Cleanup board/library manager color setting and somewhat support dark themes #9272
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, for the boards manager: - InstallerJDialog would set the "selection background" color on the table, using the "status.notice.bgcolor" the color (default blueish green). This color is not used directly, but made available for cell renderers to use. https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/ui/InstallerJDialog.java#L183 - For each cell, either a ContributedPlatformTableCellEditor or ContributedPlatformTableCellRenderer is used, depending on whether the cell is being edited (i.e. when selected). - Both of these create a ContributedPlatformTableCellJPanel, and call its `update` method, which creates the components for the cell. - The `update` method als sets the background color of the description to white, which does not actually have any effect because the description is not opaque. https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java#L271 https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java#L309 https://docs.oracle.com/javase/7/docs/api/javax/swing/JComponent.html#setBackground(java.awt.Color) - The `update` method also sets its colors of itself (JPanel) to the FG and BG color, or the selected FG and BG color of the table depending on the selected status of the cell. These seem to default to black on white for non-selected and white on blue-ish for selected cells. However, InstallJDialog has replaced the selected BG with a blueish green, as shown above. Of these, only the BG colors actually seem to take effect. The fg color of the description component is actually used (default black). https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java#L282-L288 - After calling `update`, ContributedPlatformTableCellEditor overrides the JPanel background color with a fixed grey color. Similarly, ContributedPlatformTableCellRenderer sets an alternating white and (slightly lighter) grey background color. Together, this means that the background color set by ContributedPlatformTableCellJPanel is never actually used. https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java#L132-L133 https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java#L47-L53 For the library manager, pretty much the same happens. Effectively, the only colors that were actually used were the background colors set by ContributedPlatformTableCellEditor and ContributedPlatformTableCellRenderer. This is problematic because: - There is a lot of other confusing and unused code - The foreground color is never set. This is fine when it is black or another dark color, but when the system is configured with a dark theme, the default foreground color will be white, which is problematic on a white background. This commit remove the unneeded code, setting the foreground color is left for later. It also removes the (now unused) `isSelected` from `ContributedPlatformTableCellJPanel::update`. For the library manager, the corresponding argument is still used to decide the "author" color.
Previously,`makeNewDescription` was called in the constructor and then again later in the `update` method (board manager) or later in the constructor (library manager) to recreate the description JTextPane so it can be filled with text. In all cases, the pane would be created equal, so there is no point in recreating it. Now, it is created only once and stored in an instance variable for later reference. Additionally, `makeNewDescription` now only creates the JTextPane, the constructor handles adding it (like for other components). This change slightly simplifies code, but also prepares for allowing to change the description text color externally in a later commit. For the library manager it is not currently strictly needed to have an instance variable (since the description is only used inside the constructor), but the instance variable is added for consistency and to prepare for this same upcoming change.
Previously, only the background color was changed to white or light grey. This worked well for the default theme with a black or dark text, but not for a dark theme with white or light text. This commit fixes this by also overriding the text color to be black. Since the colors are set on the JPanel table cell, but the actual text is rendered by the description JTextPane, the `setForeground` method is overridden to forward the foreground color to the description pane. Note that this commit only touches the table cell and description inside, the dropdowns and buttons have neither background nor foreground color set (thus these use both colors from the system theme). It might be more consistent to also override these, but such native UI components are typically tricky to colorize properly, so best let the system handle that normally. An alternative solution would be only use the default colors, which would actually preserve the dark theme colors in these managers as well (rather than forcing black-on-white/grey as now). There are default colors for selected and non-selected table cells that could be used, but these are different from the current colors. Additionally, the current odd/even alternating colors are then also no longer available.
|
✅ Build completed. Please test this code using one of the following: ⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-9272-BUILD-897-linux32.tar.xz ℹ️ The |
|
If you plan on going with the second version I recommend switching the links color to something brighter (maybe brighter blue or orange?) in the dark theme so it doesn't hurt the eyes as much. |
Well, this is exactly the main reasons to not use the second version, since it is tricky to know what theme is being used (light and dark could maybe be autodetected with some smartness, but there might be completely different themes too). In this example I just used the default (theme) colors for background and normal text, but it seems links use a hardcoded color, or perhaps a default link color that is not changed with the theme. Figuring out a good color depending on the used theme is probably tricky... |
matthijskooijman
added a commit
to matthijskooijman/Arduino
that referenced
this pull request
Mar 14, 2020
In commit 93581b0 (Set foreground color in library/board manager), the foreground color was set in addition to the background color, to make sure that the library and board manager would remain readable even with a non-standard color scheme (e.g. a dark theme). When that commit was created, this worked properly. However, between creating that commit and merging it as part of arduino#9272, the title rendering was changed from being part of the description (which had its color set up properly) to being part of the title border (which used default colors) in arduino#9262. This commit fixes this again by applying the foreground color also to the TitledBorder component.
matthijskooijman
added a commit
to matthijskooijman/Arduino
that referenced
this pull request
Mar 14, 2020
In commit 93581b0 (Set foreground color in library/board manager), the foreground color was set in addition to the background color, to make sure that the library and board manager would remain readable even with a non-standard color scheme (e.g. a dark theme). When that commit was created, this worked properly. However, between creating that commit and merging it as part of arduino#9272, the title rendering was changed from being part of the description (which had its color set up properly) to being part of the title border (which used default colors) in arduino#9262. This commit fixes this again by applying the foreground color also to the TitledBorder component. In ContributedPlatformTableCellJPanel this also moves the creation of the TitledBorder into the constructor, and for ContributedLibraryTableCellJPanel upwards in the constructor (where it is run unconditionally), so the property can be final.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm running with a dark system theme, which defaults interfaces to have white/light grey on black/dark grey coloring. This works pretty well in the Arduino IDE, except for the library and board managers.
As an example, here's how the board manager looks using the default Gnome light theme (adwaita):
However, when switching to a dark theme, adwaita-dark (can be done using the gnome-tweaks command), the background colors change but the foreground color stays at the (now) default light color:
While investigating, I also found that there is a lot of color-setting code that is not actually effective (because the colors are set on the wrong component, or are overwritten by other code). The first commit removes this useless code. See the commit message for a detailed analysis of how the code paths run.
The second commit does a bit of cleanup and refactoring needed for the last commit, which also sets the foreground color to black to match the background color. This results in a usable interface again:
With the default light theme, the interface is unchanged. All of the above also applies to the library manager, which is fixed in the same way.
Note that this essentially forces a light theme for the board and library manager, but leaves the dropdowns and buttons unchanged (since it is probably tricky to completely undo the dark them for those).
An alternative solution would be only use the default colors, which would actually preserve the dark theme colors in these managers as well (rather than forcing black-on-white/grey as now). There are default colors for selected and non-selected table cells that could be used, but these are different from the current colors. Additionally, the current odd/even alternating colors are then also no longer available.
For completeness, here's how that would look in a light theme:
And in a dark theme: