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

Mac: Support Dark Mode in Advanced View #5267

Merged
merged 11 commits into from
Jun 19, 2023
Merged

Mac: Support Dark Mode in Advanced View #5267

merged 11 commits into from
Jun 19, 2023

Conversation

CharlieFenton
Copy link
Contributor

Fixes #3115 (at least for macOS)

Implementing Dark Mode support for macOS and possibly for Windows and Linux. I am initially not guarding my changes with #ifdef __APPLE__ or #ifdef __WXMAC__ to allow testing of theWindows and Linux CI build artifacts to see how well my Mc changes work on those platforms. I expect to add those guards before submitting this PR for merging.

So far, I have Dark Mode working on the Projects, Tasks, Transfers and Disk tabs in Advanced View, as well as the Event Log and various Advanced View dialogs. I have not been able to work on the Notices tab because my attached BOINC projects currently have no notices.

I still need to implement Dark Mode for the Simple View.

@CharlieFenton CharlieFenton marked this pull request as draft June 12, 2023 12:51
@AenBleidd
Copy link
Member

@CharlieFenton, thank you for this implementation.
if you don't mind, please don't rush with those guards. There is a high chance that it might work on Linux and Windows as well.
I'll check this week om Windows and linux, and will try to provide fixes for aby possible UI issues.

@CharlieFenton
Copy link
Contributor Author

@AenBleidd Please let me finish it on the Mac before you get too deeply into texting on other platforms. If this can be made to work on other platforms with a reasonable number of fixes, I would prefer that be done in a separate PR after this one has been merged.

@AenBleidd
Copy link
Member

@CharlieFenton, for sure I'll wait for you.
What I wanted to say, is that I don't see necessary adding those guards even to this PR before the merge, because since we don't plan to release it right away and still have time for work on possible fixes for Windows and/or linux.

@Vulpine05
Copy link
Contributor

So far, I have Dark Mode working on the Projects, Tasks, Transfers and Disk tabs in Advanced View, as well as the Event Log and various Advanced View dialogs. I have not been able to work on the Notices tab because my attached BOINC projects currently have no notices.

I still need to implement Dark Mode for the Simple View.

@CharlieFenton, if you have an app_config file for a project, just make a typo under one of the headers. That should create a notice.

@AenBleidd
Copy link
Member

I made a quick test on Windows.
Since the latest released version of wxWidgets we are using to build BOINC doesn't support dark mode, only the self-drawn elements change their color.
For now dark mode should be disabled for Windows.
I'll keep and eye on wxWidgets, and when they will implement this feature, I'll give another try.
Later will check also the Linux version.
Hopefully at least there is will work fine.

@CharlieFenton
Copy link
Contributor Author

@AenBleidd Thank you for testing. Once I hear your Linux results, I will add guards to the code so it does nothing on Windows, and perhaps also Linux if needed.

@@ -1980,6 +1980,55 @@ void CAdvancedFrame::OnSelectAll(wxCommandEvent& WXUNUSED(event)) {
}


// On the Mac, we must destroy and recreate each wxListCtrl
// to properly transition between dark mode and regular mode
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank comment line after this line.


StopTimers();
SaveState();
// Get the list copntrol's current scroll position
Copy link
Contributor

Choose a reason for hiding this comment

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

Spell check: "control's"

RestoreState();

// The last tab label ("Disk") is ellipsed here unless the window is resized
// TODO: figure out how to replace this hack with a proper fix
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank comment line after this line.

m_noticesBody = wxT("<html><head></head><body></body></html>");
}

// In Dark Mode, paint the windoe black immediately
Copy link
Contributor

Choose a reason for hiding this comment

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

Spell check: window

@CharlieFenton
Copy link
Contributor Author

@AenBleidd Dark Mode in Advanced View on Mac is now fully working; I have not finished implementing it for Simple View. Please test Dark Mode Advanced View on Linux today if possible.

@AenBleidd
Copy link
Member

@CharlieFenton, sure, will try to do that. I'll let you know when I get the results

@CharlieFenton
Copy link
Contributor Author

@AenBleidd I have set it up so that the selection of which platforms use the Dark Mode code requires changes in only one line: clientgui/BOINCGUIApp.h line 44. I have temporarily set that line to allow both MacOS and Linux:
#if (defined(__WXMAC__) || defined(__WXGTK__))

Again, Advanced Mode is fully implemented for macOS, but I am still working on changes for Simple Mode. Please test Advanced Mode on Linux.

@CharlieFenton
Copy link
Contributor Author

@AenBleidd @davidpanderson: I have updated the copyright dates on all source files I have modified except common/wxPieCtrl.cpp. The files common/wxPieCtrl.cpp and common/wxPieCtrl..h have copyrights from wxWidgets, though I suspect they may have been modified for BOINC in the past. I don't know what to do about these. Should we add the BOINC copyright after the wxWidgets copyright, or perhaps in front of it?

@CharlieFenton
Copy link
Contributor Author

@AenBleidd I think I have now completed implementing Dark Mode for the Macintosh. The only thing left is for me to remove || defined(__WXGTK__) from line 44 of clientgui/BOINCGUIApp.h unless you find that it works on Linux. Please test this as soon as possible and let me know so I can change this from draft.

@KeithMyers
Copy link

Just wanted to also provide some error messages from this boincmgr version when executed.

./boincmgr
Gtk-Message: 14:52:09.356: Failed to load module "appmenu-gtk-module"
Gtk-Message: 14:52:09.391: Failed to load module "canberra-gtk-module"
Gtk-Message: 14:52:09.393: Failed to load module "canberra-gtk-module"

Already have both of the supposed missing libraries loaded.

$ apt search appmenu-gtk-module
Sorting... Done
Full Text Search... Done
appmenu-gtk-module-common/jammy,jammy,now 0.7.6-2 all [installed,automatic]
Common files for GtkMenuShell D-Bus exporter

$ apt search libcanberra-gtk-module
Sorting... Done
Full Text Search... Done
libcanberra-gtk-module/jammy-updates,now 0.30-10ubuntu1.22.04.1 amd64 [installed]
translates GTK+ widgets signals to event sounds

@KeithMyers
Copy link

'Notices' is the web page. Not sure it's reasonable to convert it to dark mode manually. For me it's completely fine to have it as-is

Ok, didn't realize that Notices was an actual web page pulled from the projects.

@davidpanderson
Copy link
Contributor

Notices is part of the manager. It's displayed using an HTML renderer that's part of WxWidgets. In dark mode it would ideally be dark too.

@AenBleidd
Copy link
Member

@davidpanderson, not sure wxWidgets 'smart enough' to convert colors on the html page.
In any case, I'd say that this is not something that should be fixed on our side, since it might require quite a huge effort (I'll check if this component of wxWidgets supports dark mode at all, and probably will talk to the wxWidgets maintainers about it)

@CharlieFenton
Copy link
Contributor Author

On the Mac, Notices uses wxWebView and my code does shift its display to whit text on black in Dark Mode. On at least some Linux versions, Notices uses wxHtmlWindow because wxWebView is unavailable or doesn't work. I h ave no way to test on Linux, but I will see if I can get it working with wxHtmlWindow by temporarily modifying the Mac code to use wxHtmlWindow.

@CharlieFenton
Copy link
Contributor Author

@AenBleidd I noticed one other small thing I need to fix on the Mac before it is ready to merge. I wi ll also try to get it working with wxHtmlWindow for the Notices.

@CharlieFenton
Copy link
Contributor Author

Here is what the Notices tab looks like on the Mac in Dark mode:

Screenshot 2023-06-16 at 5 07 54 PM

Mac: Ensure that the 3 buttons at bottom of Simple View are legible
@CharlieFenton
Copy link
Contributor Author

@KeithMyers I believe I have fixed Dark Mode for the Notices tab under Advanced View and Notices under Simple View in Linux. Please test it and let us know the result. Also, please check the various dialogs (Select Computer, Computing Preferences (both Simple and Advanced Views), Other Options, Event Log Options, Exclusive Applications, Select Columns, Add Project / Account Manager, etc.)

@codecov
Copy link

codecov bot commented Jun 18, 2023

Codecov Report

Merging #5267 (6a0ea0c) into master (c520ed6) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5267      +/-   ##
============================================
- Coverage     10.86%   10.86%   -0.01%     
  Complexity     1064     1064              
============================================
  Files           279      279              
  Lines         35969    35973       +4     
  Branches       8275     8313      +38     
============================================
  Hits           3909     3909              
- Misses        31668    31672       +4     
  Partials        392      392              

see 1 file with indirect coverage changes

@KeithMyers
Copy link

KeithMyers commented Jun 18, 2023

New artifact works on all tabs of the Manager now.
I've gone through all the menus and selected various options. All choices give me Dark Mode view.
Looks like you've done it.

@KeithMyers
Copy link

Screenshot from 2023-06-18 07-12-17
Screenshot from 2023-06-18 07-11-39
Screenshot from 2023-06-18 07-02-46
Screenshot from 2023-06-18 07-01-02
Screenshot from 2023-06-18 07-00-23
Screenshot from 2023-06-18 06-56-56

@AenBleidd
Copy link
Member

Looks awesome. Thanks, @CharlieFenton, @KeithMyers!
I'll make a review tonight then.

@davidpanderson
Copy link
Contributor

Great work! Small suggestion:
in the striped areas (tasks, projects) make the light gray darker.

@KeithMyers
Copy link

KeithMyers commented Jun 18, 2023

Great work! Small suggestion: in the striped areas (tasks, projects) make the light gray darker.

Not too much though. I think it's fine in my opinion.

Some commentary from teammates about the shade of blue in the tasks, projects percentage areas. . . . too dark a shade of blue. I'm fine with it as it. Think this color is hard-coded. Doesn't respond to the color palette in Ubuntu Settings. Only the Event Log highlighting follows the Desktop color palette.

Copy link
Member

@AenBleidd AenBleidd left a comment

Choose a reason for hiding this comment

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

Looks good to me

@KeithMyers
Copy link

I too am fine with the color choices as is. Niggles only for any commentary from other users.

@CharlieFenton
Copy link
Contributor Author

Our code is not setting the colors and doesn't have control over the choices. wxWidgets sets the Light Mode colors and the OS (or perhaps code in wxWidgets) decides what to change them to for Dark Mode. All we do to create the alternating row colors (which wxWidgets calls "Zebra striping") is call EnableAlternateRowColours(true); .

@CharlieFenton
Copy link
Contributor Author

I've gone through all the menus and selected various options. All choices give me Dark Mode view.

@KeithMyers Does this mean that you were able to confirm that the dialogs I listed here also display properly in Dark Modern Linux? If so, this PR is ready to be merged.

@CharlieFenton CharlieFenton marked this pull request as ready for review June 18, 2023 23:27
@CharlieFenton
Copy link
Contributor Author

@KeithMyers Thank you for your help with this.

@KeithMyers
Copy link

I've gone through all the menus and selected various options. All choices give me Dark Mode view.

@KeithMyers Does this mean that you were able to confirm that the dialogs I listed here also display properly in Dark Modern Linux? If so, this PR is ready to be merged.

Yes, I went through every choice you mentioned and confirmed they all were in Dark Mode.

@AenBleidd AenBleidd merged commit f425696 into master Jun 19, 2023
@CharlieFenton CharlieFenton deleted the mac_dark_mode branch June 19, 2023 07:18
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.

[Manager] Support dark theme mode on Linux/MacOS
6 participants