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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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
@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.

@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-3014-9f91c62609bfa4028007d1ccd7f3db6d0d27c459-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.

@sonarcloud
Copy link

sonarcloud bot commented Nov 15, 2021

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.

Signed-off-by: Elsie Hupp <9206310+elsiehupp@users.noreply.github.com>
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.

3 participants