waydabber / BetterDisplay

Unlock your displays on your Mac! Flexible HiDPI scaling, XDR/HDR extra brightness, virtual screens, DDC control, extra dimming, PIP/streaming, EDID override and lots more!
https://betterdisplay.pro
18.69k stars 331 forks source link

Update popover style menu (expanding menu disabled) to act more like traditional NSMenu (+ fix popover related crash on Sonoma) #3282

Closed KrzysztofPrzygoda closed 3 weeks ago

KrzysztofPrzygoda commented 1 month ago

See attached video: https://github.com/user-attachments/assets/ae3c5c2e-48ce-41b2-9c37-b8f1db932018

BD v3.0.2 macOS Sonoma 14.6.1 MBP16 M1 Max


UPDATE: Pre-release available with a fix for popover induced crash - get the latest one here: https://github.com/waydabber/BetterDisplay/releases/tag/pre

waydabber commented 1 month ago

Does it only crashes like this when overloaded this way or does it crash normally?

Note: nice to have confirmation that at least somebody uses the old popup style menus - I honestly thought everybody is using the default expanding style so was contemplating removing this feature entirely. I'll now have to fix it instead. ;)

KrzysztofPrzygoda commented 1 month ago

Sometimes as fast as on third submenu and on the very slow browse.

waydabber commented 1 month ago

Ok, that's not good. I was not aware of this as I practically never used the app this way since first adding the option. But it's like this with many features, so these feedbacks are really helpful. Many live with bugs instead of reporting them.

KrzysztofPrzygoda commented 1 month ago

Note: nice to have confirmation that at least somebody uses the old popup style menus - I honestly thought everybody is using the default expanding style so was contemplating removing this feature entirely. I'll now have to fix it instead. ;)

Can you elaborate on "the new popup style menus"? I thought they are "new". Probably I'm not aware of something :)

KrzysztofPrzygoda commented 1 month ago

Do you mean "expandable menus"? I find floating ones a way better option - zero-click effort to browse them.

waydabber commented 1 month ago

I mean the popup style menu is old in a way that it was how v1.x worked. From v2.x the app uses an in-place expanding menu system by default, except when it is disabled. I gave the option as I feared some would complain about the new system. Actually nobody ever complained, but it's good to know some actually use the popups.

Do you see any particular reason why the app crashes? In Activity Manager maybe memory usage goes up? I was not able to quickly reproduce the issue although trying to overload the app this way. I might need to add some slight delay as a minimum time threshold between switching between two popups.

waydabber commented 1 month ago

Hmm. Maybe it's Sonoma specific (I am on Sequoia). Apple is constantly tweaking SwiftUI, fixing bugs (+introducing new ones :)), changing things - it's great on one hand but also makes development a bit difficult - SwiftUI is a somewhat interpreted thing so one can never know that the code that a UI code that works on a particular version of macOS might not be broken on an other (maybe sub)version. Will try to reproduce this on a Sonoma system later on.

KrzysztofPrzygoda commented 1 month ago

My observations:

  1. Memory alloc at start is around 44 MB. While browsing submenus rises up to 90 MB.
  2. Last log line on crash: [2024-08-11 19:13:35.883] Startup completed Fatal error: Attempted to read an unowned reference but object 0x60000340cea0 was already deallocatedzsh: abort /Applications/BetterDisplay.app/Contents/MacOS/BetterDisplay -v
  3. No crash report invoked by macOS.
  4. Sometimes crashes fast after 2 secs of browsing. The other time I needed a minute.
KrzysztofPrzygoda commented 1 month ago
  1. It happens on reopening last opened submenu.

So, it looks like app is trying to open (reference) already deallocated previous submenu.

waydabber commented 1 month ago

Can you show me a video (if it's not a too big task to ask) how to reproduce this with 5. in mind? I am trying to reproduce it by reopening last opened menu items and can't seem to succeed.

waydabber commented 1 month ago

I see the crash and the detailed crashlog in Sentry. 19 users were affected so far (which is a low number in terms of overall usage but can build up over timre) and the issue is indeed new in 3.0.x. Not only Sonoma is affected, I have some Sequoia reports as well.

Referencing/dereferencing an object in a SwiftUI context is not something I need to do manually and sadly the crash seems to come from somewhere deep in SwiftUI environment territory, well outside of the scope of app code (the last app command the system seems to be able to tie the issue is app.run() which basically means that it can't tie to any specific app cause. This is further validated by the message Crashed in non-app libsystem_kernel).

I am afraid the problem might come from the fact that the version is using a Swift 6 which is rather new and still considered beta.

I think I'll need to file this under "Known issues" for now and hope the next Xcode/Swift version will fix this particular issue.

KrzysztofPrzygoda commented 1 month ago

Can you show me a video (if it's not a too big task to ask) how to reproduce this with 5. in mind? I am trying to reproduce it by reopening last opened menu items and can't seem to succeed.

The video is quite short this time :)

https://github.com/user-attachments/assets/14cdf0bd-cce0-4b62-a19f-1a4e502ed981

waydabber commented 1 month ago

Is this a way to reliably reproduce the issue? I will try to recreate the same problem with the same number of display modes as normally I can't seem to reproduce it no matter what. The issue seems to be related to the Display Mode menu, right? At least both videos show this one crashing.

KrzysztofPrzygoda commented 1 month ago

I can reproduce it in every run on any submenu. Crash time is inconsistent and a matter of determination but inevitable. I suspect asynchronous (de)allocation (multithreaded), that's why it is time based issue.

Here is the run that takes longer to crash:

https://github.com/user-attachments/assets/8cfd9bee-b9fd-4cff-b5cf-fa89f0ef6946

waydabber commented 1 month ago

All right, thank you. I am now filing this under a mystery for now as I simply can't reproduce the problem. However I can see that the issue is real. An other reason might be that it's fixed in the recent Sequoia build (I only see Sonoma and older Sequoia betas with this issue, those from 3.0.0 and 3.0.1).

waydabber commented 1 month ago

Note: about deallocation/multithreading issue, the code that actually shows the popover is so abstract and far from any actual reference and memory management - as it's pure SwiftUI code - that I simply have no way to influence any of that - SwiftUI is doing a lot of multithreading and is reactive by nature, but that is several layers below - mostly abstracted away at the level the actual code is written, and difficult or even impossible to access or interfere with (this is by design). The actual SwiftUI code that shows menu content in popovers actually did not change at all from v2.3.6 to v3.0.2. There are of course various other changes in the app so I am not claiming there is no issue somewhere that might cause this, but if there is, it is not clear at all what it possibly could be. With regards to the display menu I at least introduced some added multithreaded code so I was thinking that since the issue seemed to be localized to that part, I might have some chance of figuring it out (as it might not be a SwiftUI issue then) - but this does not seem to be the case.

waydabber commented 1 month ago

Note: was able to reproduce the issue easily on a Sonoma Mac. Not at all on Ventura or the latest Sequoia. I think this might be a bug that was introduced at some point in the runtime but now got fixed. The bug is the "Send to Apple" kind so Apple must have collected and received a number of similar issues from other apps. I'll continue to look out in Sentry for crashes from other macOS versions or the latest Sequoia beta as that might put things into a different perspective again. The issue only happens with expandable menus disabled so that explains the low reported issue count.

KrzysztofPrzygoda commented 1 month ago

Great news for me as Sequoia will be released soon. Worse news for those who will stuck with Sonoma due to Apple policy. Hopefully, not many.

waydabber commented 1 month ago

Since this issue was not present in v2.3.6 which used the same code, it might be that the problem that triggers the issue on Sonoma is somehow introduced by XCode 16. There is a chance that it will get fixed, eliminating the problem for Sonoma users as well. I have to note that based on the error telemetry (if this problem is a proper indication), only a small number of users are using the traditional popover style menus on affected systems (according to the calculations - 0.11%).

waydabber commented 1 month ago

I added this to a known issues section in the latest release description.

https://github.com/waydabber/BetterDisplay/releases/tag/v3.0.2

PuzzledUser commented 1 month ago

Note: nice to have confirmation that at least somebody uses the old popup style menus - I honestly thought everybody is using the default expanding style so was contemplating removing this feature entirely. I'll now have to fix it instead. ;)

Just for balance. I hated the new way and swapped back to the old way with glee. Please don't remove it! 🤣

PS I have seen at least two crashes (in 10 minutes) since updating to V3. But not tied down a trigger. I can't reproduce https://github.com/KrzysztofPrzygoda's crash.

PuzzledUser commented 1 month ago

Note: was able to reproduce the issue easily on a Sonoma Mac. Not at all on Ventura or the latest Sequoia. I think this might be a bug that was introduced at some point in the runtime but now got fixed.

Just a thought. I bet it's a conflict between the fade out of the old menu and the fade in of the new. You could test my hypothesis by setting the delay of one (or both) to zero. Might also make use of the menus less fussy too.

waydabber commented 1 month ago

@PuzzledUser - it has no logical reason from app code perspective, it's just a SwiftUI bug. Honestly I actually don't like at all how the popover looks by default, the silly popup animation, the anchor etc. But SwiftUI can't do anything other. I am just experimenting rewriting it with a fully custom implementation instead of SwiftUI popover:

Screenshot 2024-08-13 at 19 14 05
KrzysztofPrzygoda commented 1 month ago

I am just experimenting rewriting it with a fully custom implementation instead of SwiftUI popover.

Fantastic news! Why cannot it look like regular menu's submenu (without any fancy animations or speech bubble shape)? A good, old and well-known UI paradigm.

image
waydabber commented 1 month ago

I'll try to mimic it, but this is not available in SwiftUI this way (Apple's UI ecosystem is not unified actually in many ways, there are various solutions from various generations of the OS, all with different limitations and presentations. For some of the newest stuff even Apple does not use any of the standardized components - see Control Center for example which internally uses a lot of hacks to work as it does, much like how BetterDisplay tries to patch together various technologies and workarounds to present an UI that at the end looks like as intended :)).

waydabber commented 1 month ago

Ok, I kinda achieved what I wanted. This will need testing as although it should look much like native menus, nothing is native about it - actually everything (timings, fadeouts, positioning etc) is meticulously and artificially coded to mimic the native NSMenu feel as much as possible and the code is somewhat complex to do so.

waydabber commented 1 month ago

I updated a pre-release, let me know if you find any serious issues with the implementation so I can try fixing it. Thanks!

waydabber commented 1 month ago

Latest one here: https://github.com/waydabber/BetterDisplay/releases/tag/pre

waydabber commented 1 month ago

The implementation still has some issues - these are what I found so far:

PuzzledUser commented 1 month ago

I noticed that sometimes a menu item doesn't highlight and so its sub menu doesn't activate. It took me a while to reproduce. It seems that if mouse enters a region just as a previous submenu disappears, then the highlight disappears too. It doesn't return until there is a new enter over the menu item. See 33s into the video.

https://github.com/user-attachments/assets/8bd84858-3512-4020-ae2d-d71a2cd14c0f

waydabber commented 1 month ago

Yeah, you are right. OnHover is not 100% effective in this context, much like 99.5% - generally there are some uncertainties because of the declarative nature of the UI, heavy multithreading and interaction of various parts of the UI elements. This level of uncertainty is generally present in most modern complex UI apps and is unlikely to adversely affect normal daily use. The issue is not strictly popover related, happens with normal menu items as well from time-to-time. I am actually impressed that it could only be reproduced only after 30-40 seconds of intensive effort. ;)

waydabber commented 1 month ago

@PuzzledUser - I see you have a Studio Display. Can you show me what you see under Color Mode for this display? Is there any or only a sole option is available (you showed a lot of submenus but just happened to skip that one 😄)?

PuzzledUser commented 1 month ago

Sure, attached. Points taken. I actually noticed the submenu glitch much quicker initially, but frustratingly took so long to reproduce whilst recording!

image

Looks like you need some UK localisation for things such as Colour! 😉

waydabber commented 1 month ago

Well, if somebody decides to contribute with a UK localisation, I'm ready to add it! :)

KrzysztofPrzygoda commented 1 month ago

I updated a pre-release, let me know if you find any serious issues with the implementation so I can try fixing it. Thanks!

This is way better than popovers! It's faster and cleaner. One thing is only not clear to me: why Display Mode rolls out (modes render is visible), while any other just shows up fully rendered?

KrzysztofPrzygoda commented 1 month ago

Some long submenus renders outside of the screen and despite of scrolling content ability, latest list items are not reachable (here the Color Profile submenu).

https://github.com/user-attachments/assets/3f560c34-7174-4f45-b180-084e2ddfd5de

Moreover, parent menu item should stay highlighted. Now it is dehighlighted while leaving menu item to its submenu. And I'd prefer that highlight color comes from OS defaults - there are two states: highlighted (Accent Color - mine is blue) while pointer is over and selected (gray) while pointer enters NSMenu submenu.

https://github.com/user-attachments/assets/1ce5882d-0a52-4678-a16b-5f1a5b385be1

waydabber commented 1 month ago

I'll try to address the long submenus issue (if feasible).

Regarding no highlight color, the current gray is better I think. This is the format Apple itself uses for detached Control Center menus and other newer menubar extra items - this is where BetterDisplay belongs more in terms of style + probably that is generally the way forward for macOS (don't know exactly of course). Right now macOS is not universal in this and uses both styles - older built-in macOS menubar extra items (using legacy code) the highlight color is the accent color, for newer ones it's gray (as accent color would obviously clash with the rounded icons in unexpected ways). Control Center and newer items use an entirely custom way to present things (it's also SwiftUI with various hacks to act more like NSMenu, including rendering things on a transparent window etc).

Example:

Screenshot 2024-08-14 at 14 20 45
waydabber commented 1 month ago

Note that for newer style menus Apple actually stopped using popover style submenus, opted for expandable things with various differing styles, as it was made by different teams:

Screenshot 2024-08-14 at 14 23 55 Screenshot 2024-08-14 at 14 30 09

(note the glitch in the XDR preset dropdown, the right side of the expandable area is rendered incorrectly)

waydabber commented 1 month ago

Anyway, just noting these to see that macOS itself is an amalgamation of styles, Apple does not really follow it's own design guidebooks and things are not as precise, so it is not fully obvious what style a developer should follow who wants to abide by the rules. This, combined with the fact that existing ready-made APIs do not even have the functionality most modern apps need, creates an environment in which every developer will come up with something. I mostly try to mimic the macOS look&feel while taking some liberties where Apple itself does. :)

waydabber commented 1 month ago

Made it so that when a submenu is open, the parent menu has a lighter background.

Screenshot 2024-08-14 at 17 12 44
KrzysztofPrzygoda commented 1 month ago

Made it so that when a submenu is open, the parent menu has a lighter background.

May I get the latest prerelease with these changes?

waydabber commented 1 month ago

Sure. I also added a a protection to the window delegate so the popover submenu does not get out of the safe area of the screen in Y direction.

Screenshot 2024-08-14 at 19 13 33
KrzysztofPrzygoda commented 1 month ago

BTW, how to understand that Retina built-in display at 10bit Display Mode has 8bit Color Mode only available?

image
waydabber commented 1 month ago

Pushed an update.

No, the MBP panel and the connection is 8-bit, the framebuffer is 10 bit (see the Display Mode menu) - it uses (GPU side) temporal dithering to create 10-bit effect.

KrzysztofPrzygoda commented 1 month ago

No, the MBP panel and the connection is 8-bit, the framebuffer is 10 bit (see the Display Mode menu) - it uses (GPU side) temporal dithering to create 10-bit effect.

Display Mode says "10bit":

image
waydabber commented 1 month ago

Yes. That is the framebuffer (video memory) pixel color depth. The connection bit depth is 8-bit. The uses GPU temporal dithering (alternates two neighboring color values in even/odd frames) to create interim color levels + also uses spatial dithering and various other algorithms. Acutally an insane amount of image processing is going on with a lot of things to play with. But this is off-topic here. :)

KrzysztofPrzygoda commented 1 month ago

Pushed an update.

It's perfect now!

One thing left: why Display Mode rolls out while other submenus render at once (ready made)?

https://github.com/user-attachments/assets/b8f25e80-9548-43ed-907a-e31e970e5f0f

waydabber commented 1 month ago

Yes, that is intentional - the system has trouble rendering the resolution list in time so it's now staged so it does not stutter when opening the menu item. Might try to optimize or at least figure out the size beforehand so no size change is needed.

waydabber commented 1 month ago

I somewhat optimized that so when the expandable menu items are disabled, the presentation won't be staged (only when expanding style is used - in that case the menu is more overloaded due to all the other pre-existing layout).

waydabber commented 1 month ago

An update will be available in about 5 minutes. Check it out and let me know if you have any other issues with the new submenu implementation. Did not think I'll spend 2 full days on just this, but at least it now works great. :)