Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements #261

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Conversation

schriftgestalt
Copy link
Contributor

No description provided.

the limitation to Big Sur is mostly from the image as it uses SF-Symbols
@yujitach
Copy link
Owner

yujitach commented Nov 2, 2021

Hi, I tried to merge many of the user-invisible changes. (The next one to do is menuitem.indentation, which I also intended to do for a long time...) Thank you for all your works in cleaning up the code.

  • I'm OK with changing the look of the pref pane, but I'm not very OK with changing how the meters themselves look. I myself is mostly OK, but it might disturb many of the users, especially those who're using old versions of macOS for a long time who don't find any need. I know making things more compact is necessary for Macbook with a notch. But this doesn't affect other users. I also know that default colors should be made system colors. But this doesn't affect users of old macOS's whose menubar doesn't change the color automatically.
  • I don't like micro-optimizations which complicate the code, such as your caching of pref entries. I don't think it really matters as far as the battery consumption is concerned. I'm happy with micro-optimizations which actually make the code more maintainable.
  • Please use NSLog instead of MMLog. They are useful when you need to study bugs which manifest only on users' Macs, not yours.
  • Please don't use CFPreference... api. It has a slightly different behavior concerning how it traverses .plist files which are symbolic links, and I was once contacted by a user who habitually makes .plist files within Library/Preferences symbolic links, who hit this very subtle feature/bug of the OS....

@yujitach
Copy link
Owner

yujitach commented Nov 2, 2021

I thought of cherrypicking your "move tallMenuBar into MenuMeterDefaults" but my Mac Mini with an external display (on Monterey) gave the menu bar height as 24. So it was identified as "Macbook with Notch" ...

@yujitach
Copy link
Owner

yujitach commented Nov 2, 2021

By the way, have you tried any of the following?

They are better looking and better maintained. You might like some of them more than MenuMeters; you might then want to contribute to that instead. Your contribution would then be felt by more. Honestly, I don't think you don't have to waste your valuable time into clearing up this old code base ... But thanks nonetheless for the contributions so far.

@schriftgestalt
Copy link
Contributor Author

I thought of cherrypicking your "move tallMenuBar into MenuMeterDefaults" but my Mac Mini with an external display (on Monterey) gave the menu bar height as 24. So it was identified as "Macbook with Notch" ...

Then we can raise the threshold. The "tall" menu is actually 37.

@yujitach
Copy link
Owner

yujitach commented Nov 2, 2021

Thanks for the info.

Another thing I just noticed while I was reading your contributions is the Dutch localization. It seems it has been totally broken since I moved from the old method of having separate xib's to the newer "base localization" in 2018.

I'm not familiar with the new tool you're using to localize. Could you point to it? Then I need to somehow revive the Dutch localization ...

@schriftgestalt
Copy link
Contributor Author

I don't like micro-optimizations which complicate the code, such as your caching of pref entries. I don't think it really matters as far as the battery consumption is concerned. I'm happy with micro-optimizations which actually make the code more maintainable.

Come one.

  • It is much faster (the access to the defaults was ~35% of the image code).
  • it doesn't change the external API
  • it is not that much more code so the time to actually maintaining this in the future is minimal, probably less time we spent arguing about it. And I improved the code in so many areas, we can afford this.

@schriftgestalt
Copy link
Contributor Author

I’m using this: www.loc-suite.org (my branch: https://github.com/schriftgestalt/LocalizationSuite) to manage the localizations for my app. And it is very easy to manage a dozen localizations, with external translators …

I’ll can have a look at the Dutch file. I checked the German files and they seemed fine.

@schriftgestalt
Copy link
Contributor Author

Please use NSLog instead of MMLog. They are useful when you need to study bugs which manifest only on users' Macs, not yours.

I would use NSLog only for the error case to not spam the console too much. But I understand if it actually useful.

@schriftgestalt
Copy link
Contributor Author

By the way, have you tried any of the following?

No, But from the look on there website I’m not intrigued. There is too much going on. What I like about MenuMeters, is it’s simplicity and that it can’t do much more than I need. I only need the CPU and the Net item, no need for widgets.

@schriftgestalt
Copy link
Contributor Author

Please don't use CFPreference... api. It has a slightly different behavior concerning how it traverses .plist files which are symbolic links, and I was once contacted by a user who habitually makes .plist files within Library/Preferences symbolic links, who hit this very subtle feature/bug of the OS....

OK, no problem with this. This was the beginning of my optimization. But it is not needed any more with the caching.

@schriftgestalt
Copy link
Contributor Author

I'm OK with changing the look of the pref pane, but I'm not very OK with changing how the meters themselves look. I myself is mostly OK, but it might disturb many of the users, especially those who're using old versions of macOS for a long time who don't find any need.

  • As you pointed out yourself, that is a very low number of people.
  • Why do you assume that they don't like that change?
  • If they really don’t like it, they simply can keep the old version. Nothing to miss in the new version.

I know making things more compact is necessary for Macbook with a notch. But this doesn't affect other users.

Every user with a smaller screen will be effected (those people are most likely have machined with only a few CPUs and will not have as big of a problem).

I also know that default colors should be made system colors. But this doesn't affect users of old macOS's whose menubar doesn't change the color automatically.

But most uses are affected. And for the others not much changes. And the people that do mind the change, I suspect have setup their own colors already.

@bitigchi
Copy link
Contributor

bitigchi commented Dec 2, 2021

Just my two cents, please do not change how meters look. I've tried above mentioned software and they all take too much space. MenuMeters is perfect in this regard, compact and all the drop-downs list on-point information, not over padded eye candy.

@schriftgestalt
Copy link
Contributor Author

On big goal for me was to reduce the needed space.

This is my version:
Bildschirmfoto 2021-12-02 um 22 44 31

and this the current release.
Bildschirmfoto 2021-12-02 um 22 43 27
The difference is much bigger on a M1 Pro with 10 cores, it gets to the notch quickly.

@bitigchi
Copy link
Contributor

bitigchi commented Dec 2, 2021

Hmm, to be fair looks better. Unless the colours are the same and more legible, it looks like it should be okay.

The same goes for the dark mode as well.

@schriftgestalt
Copy link
Contributor Author

schriftgestalt commented Dec 3, 2021

The default colors are a bit more bright and not as nice with the highlight but are OK otherwise. And work a bit better in Dark Mode.
Bildschirmfoto 2021-12-03 um 01 44 12

@jagoose
Copy link

jagoose commented Mar 23, 2022

On top of the already highly comprehensive meters, can the fan speed be shown also? It would be very helpful in troubleshooting the noise problem of Mac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants