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

Show/Hide Menubar and Dock Icon on macOS #3014

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Conversation

elsiehupp
Copy link
Member

@elsiehupp elsiehupp commented Mar 17, 2021

Branched from #2959.

This pull request adds code to make it so that the onboarding wizard and the settings dialogue both behave like foreground windows on macOS. When either window opens, the application spawns a dock icon and displays a menubar, and when the window is closed, the dock icon despawns, and the menubar is replaced with whatever other application is now in the foreground.

On Linux (or, at least GNOME), this behavior is automatically implemented when the Qt::Window flag is used. The behavior is especially important on macOS, because a Qt::Window can be maximized, but without a menubar, the application is unable to show the titlebar containing the “stoplight” buttons, so it is very difficult to un-maximize it. (The dock icon and menubar are a package deal in Cocoa, so you can’t have one without the other. While the dock icon isn’t as immediately necessary as the menubar on macOS, implementing a temporary dock icon on macOS has the added benefit of being consistent with the existing behavior on Linux.) The fact that the Qt::Window flag does not implement this behavior on macOS by default should probably be rightly considered a bug in Qt, but I digress.

I was unable to get a working copy of Windows installed on my computer, so I was unable to test whether the behavior of Qt::Window on Windows is more like Linux or more like macOS. The way the ForegroundBackground class I created is structured makes it easy to add a Windows implementation after the fact, and indeed my original intention was to include implementations for multiple different platforms in this pull request.

Ultimately, because the behavior did not require a custom implementation on Linux (or, at least GNOME), and because I was unable to set up a Windows development environment, I decided to make this pull request solely include the macOS implementation. I also haven’t been able to test the behavior on KDE or any other Linux shells aside from GNOME, but there aren’t any other open issues about the existing behavior of the application’s dock icon, so further shell-specific implementations can also safely wait.

It’s worth noting that the onboarding wizard already uses the Qt::Window flag, so the changes I’ve made here fix the existing issues of it being difficult to exit fullscreen and it being difficult to quit the application from this wizard. This pull request is a prerequisite for adding the Qt::Window flag to the settings dialogue, because it would have the same issues that already exist for the onboarding wizard.

There may be other places where it would be desirable for Nextcloud to temporarily spawn a dock icon and a menubar, but like the possible Windows and non-GNOME Linux implementations, I feel like they can be safely deferred to a later pull request.

@elsiehupp elsiehupp marked this pull request as draft March 17, 2021 01:26
@github-actions github-actions bot force-pushed the foreground-background branch from 724557b to 22f4da1 Compare May 7, 2021 07:14
@elsiehupp elsiehupp force-pushed the foreground-background branch 2 times, most recently from a48cc3f to 5986cd7 Compare May 8, 2021 15:56
@elsiehupp elsiehupp changed the title Show/Hide Menubar and Dock/Taskbar Icon Show/Hide Menubar and Dock Icon on macOS May 8, 2021
@elsiehupp elsiehupp marked this pull request as ready for review May 8, 2021 16:24
@elsiehupp
Copy link
Member Author

By the way, what is the 0. Needs triage label supposed to mean in this case? As far as I can tell, the numbered labels are a vestige of the GitHub Projects pseudo-agile workflow (which we aren’t really using), and 0. Needs triage is for Issues that have yet to be assigned to a project, so it doesn’t really make sense to apply to a pull request.

@elsiehupp
Copy link
Member Author

I should also emphasize that the weird structure of ForegroundBackground, with multiple layers of interfaces, etc., is primarily necessary in order to get C++ and Cocoa/Objective-C to play nice together. The fact that it’s conducive to allowing additional platform-specific implementations is kind of just gravy.

@elsiehupp
Copy link
Member Author

This is still ready for review…

@elsiehupp
Copy link
Member Author

@nextcloud/desktop can someone please review this so it can get merged? And then review both #2959 and eventually #2974? #700 continues to be a significant pain for me on a day-to-day basis, and this pull plus #2959 fixes it.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@elsiehupp
Copy link
Member Author

I went ahead and (at @FlexW’s suggestion) drastically narrowed the number of open pull requests I have. If it would be helpful, I could fairly easily combine this one back into #2959.

Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice 🙂 thanks a ton!

@claucambra claucambra force-pushed the foreground-background branch from 6b230f5 to 9ae0287 Compare October 17, 2024 08:23
@claucambra claucambra added this to the 3.14.2 milestone Oct 17, 2024
@claucambra claucambra force-pushed the foreground-background branch from 9ae0287 to 0ad55e8 Compare October 17, 2024 08:26
@claucambra
Copy link
Collaborator

/backport to stable-3.14

@claucambra claucambra merged commit 1b78915 into master Oct 17, 2024
8 of 14 checks passed
@claucambra claucambra deleted the foreground-background branch October 17, 2024 08:45
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-3014-0ad55e82c7baa2400f7425d8011a17baf5374304-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

Copy link

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

Successfully merging this pull request may close these issues.

4 participants