-
Notifications
You must be signed in to change notification settings - Fork 116
Individual colors on navtabs #3680
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
base: master
Are you sure you want to change the base?
Individual colors on navtabs #3680
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements individual tab color customization for the NavigationTabsWidget, allowing each tab to have its own selected and deselected colors. This feature enables visual status indication through tab colors (e.g., showing device status like "up", "ramping", or "fault" states).
Key Changes:
- Added
individual_selected_colorandindividual_deselected_colorproperties to each tab in the TabProperty structure - Introduced
enable_per_tab_colorswidget property to toggle between global and per-tab colors (defaults to false for backward compatibility) - Renamed
setTabs()method tosetTabNames()for better clarity about what the method does
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 23 comments.
| File | Description |
|---|---|
| NavigationTabsWidget.java | Adds new color property descriptors and updates TabProperty structure with individual color fields; defines default color constants |
| NavigationTabsRepresentation.java | Updates listeners to handle new color properties; adds logic to collect and pass individual colors from model to UI component |
| NavigationTabs.java | Implements per-tab color rendering logic in updateTabs() and handleTabSelection(); adds fields for color lists and enable flag; renames tabs field to tab_names |
| NavigationTabsDemo.java | Updates demo to use renamed setTabNames() method instead of setTabs() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pressed.setStyle("-fx-color: " + JFXUtil.webRGB(selected)); | ||
| // If the per-tab colors are enabled, the color to apply is to be found in the tab_selected_colors list | ||
| if (enable_per_tab_colors == true) { | ||
| if(i < tab_selected_colors.size()) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after 'if' keyword. The code uses 'if(i' instead of 'if (i'. This is inconsistent with the Java code style used elsewhere in this file and should have a space after 'if'.
| tmpColor = deselected; | ||
| // If the per-tab colors are enabled, the color to apply is to be found in the tab_deselected_colors list | ||
| if (enable_per_tab_colors == true) { | ||
| if(i < tab_deselected_colors.size()) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after 'if' keyword. The code uses 'if(i' instead of 'if (i'. This is inconsistent with the Java code style used elsewhere in this file and should have a space after 'if'.
| if(i < tab_deselected_colors.size()) { | |
| if (i < tab_deselected_colors.size()) { |
| final List<WidgetColor> tab_selected_colors = new ArrayList<>(); | ||
| final List<WidgetColor> tab_deselected_colors = new ArrayList<>(); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra whitespace before the closing semicolon. Line 400 has two spaces after 'tab_selected_colors' and line 401 has two spaces after 'tab_deselected_colors'. This should be a single space for consistency.
| final List<WidgetColor> tab_selected_colors = new ArrayList<>(); | |
| final List<WidgetColor> tab_deselected_colors = new ArrayList<>(); | |
| final List<WidgetColor> tab_selected_colors = new ArrayList<>(); | |
| final List<WidgetColor> tab_deselected_colors = new ArrayList<>(); |
| // Set color to global "selected" color value | ||
| tmpColor = selected; | ||
| // If the per-tab colors are enabled, the color to apply is to be found in the tab_selected_colors list | ||
| if (enable_per_tab_colors == true) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid explicit boolean comparisons like 'enable_per_tab_colors == true'. Since enable_per_tab_colors is already a boolean, this should be simplified to just 'enable_per_tab_colors' for better readability and consistency with Java best practices.
| // Highlight active tab by setting it to the 'selected' color | ||
| pressed.setStyle("-fx-color: " + JFXUtil.webRGB(selected)); | ||
| // If the per-tab colors are enabled, the color to apply is to be found in the tab_selected_colors list | ||
| if (enable_per_tab_colors == true) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid explicit boolean comparisons like 'enable_per_tab_colors == true'. Since enable_per_tab_colors is already a boolean, this should be simplified to just 'enable_per_tab_colors' for better readability and consistency with Java best practices.
| final List<String> tab_names = new ArrayList<>(); | ||
| final List<WidgetColor> tab_selected_colors = new ArrayList<>(); | ||
| final List<WidgetColor> tab_deselected_colors = new ArrayList<>(); | ||
|
|
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace at the end of line 402. This should be removed.
| // Set color to global "deselected" color value | ||
| tmpColor = deselected; | ||
| // If the per-tab colors are enabled, the color to apply is to be found in the tab_deselected_colors list | ||
| if (enable_per_tab_colors == true) { |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid explicit boolean comparisons like 'enable_per_tab_colors == true'. Since enable_per_tab_colors is already a boolean, this should be simplified to just 'enable_per_tab_colors' for better readability and consistency with Java best practices.
| jfx_node.setTabNames(tab_names); | ||
| jfx_node.setTabSelectedColors(tab_selected_colors); | ||
| jfx_node.setTabDeselectedColors(tab_deselected_colors); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling setTabNames(), setTabSelectedColors(), and setTabDeselectedColors() sequentially causes updateTabs() to be called three times, recreating all tab buttons unnecessarily on each call. Consider creating a single method that accepts all three lists to avoid redundant updates, or consider batching these updates more efficiently.
| // Set color to global "deselected" color value | ||
| tmpColor = deselected; | ||
| // If the per-tab colors are enabled, the color to apply is to be found in the tab_deselected_colors list | ||
| if (enable_per_tab_colors == true) { | ||
| if(i < tab_deselected_colors.size()) { | ||
| tmpWidgetColor = tab_deselected_colors.get(i); | ||
| tmpColor = JFXUtil.convert(tmpWidgetColor); | ||
| } | ||
| } | ||
| button.setStyle("-fx-color: " + JFXUtil.webRGB(tmpColor)); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation: these lines use tabs mixed with spaces while the rest of the file uses spaces consistently. Java code should consistently use spaces for indentation (typically 4 spaces per level) to match the existing code style.
| { | ||
| final ToggleButton button = new ToggleButton(tabs.get(i)); | ||
| Color tmpColor = deselected; | ||
| WidgetColor tmpWidgetColor = null; |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable tmpWidgetColor is declared but never used in a meaningful way. It's assigned values but those values are never read. This variable should be removed and you can directly pass the list element to JFXUtil.convert().
This PR implements an option to allow each tab from the
NavigationTabswidget to have its own colors.Here is the use case that drives me to suggest this feature :
I want to be able to use rules to apply a specific color to a tab, depending on the status of the device associated to this tab.
Let's say I manage several power supply units and I want the tab color to show me which one is up, which one is ramping up, which one is in fault.
What it looks like when I'm looking into
Tab0but something happened inTab2:Of course, I also want to know that I'm looking into
Tab0, even when everything goes wrong :So I implemented :
individual_selected_colorindividual_deselected_colorenable_individual_tab_colorsFalse: the usualselected_coloranddeselected_colorare usedTrue: the newindividual_selected_colorandindividual_deselected_colorare usedfalse, avoiding any breaking changeI also changed the name of
tabsattribute and ofsetTabsmethod to clarify the fact that they refer to the tab names.Here is a .bob with various configurations of NavigationTabs that use this feature :
nav_tabs_colors.tar.gz
Let me know if you think such a feature is relevant of if you think this not the right way to implement it.
Checklist
Testing:
Documentation: