IEP-1331 Info message in CMakeMainTab is barely visible on Ubuntu #1056
Conversation
WalkthroughThe changes involve updating the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bundles/com.espressif.idf.swt.custom/src/com/espressif/idf/swt/custom/StyledInfoText.java (1)
39-43: Approved: Improved color handling, but consider internal API usageThe new color handling mechanism using
CascadingColorRegistryeffectively addresses the visibility issue mentioned in the PR objectives. By retrieving the color from the current theme's color registry, it should provide better consistency across different operating systems, including Ubuntu.However, the use of
@SuppressWarnings("restriction")suggests that you're using internal API. While this solution works, it's worth considering if there's a public API alternative to avoid potential future compatibility issues.Consider exploring public API alternatives for color registry access to ensure long-term maintainability and compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- bundles/com.espressif.idf.swt.custom/src/com/espressif/idf/swt/custom/StyledInfoText.java (3 hunks)
🔇 Additional comments (2)
bundles/com.espressif.idf.swt.custom/src/com/espressif/idf/swt/custom/StyledInfoText.java (2)
Line range hint
3-20: LGTM: Import statements updated correctlyThe new import statements for
ColorRegistry,PlatformUI, andCascadingColorRegistryare appropriately added to support the theme-based color retrieval mechanism. These changes align with the PR objectives of improving visibility across different operating systems.
Line range hint
1-180: Verify color consistency across themes and operating systemsWhile the changes look good and should address the visibility issue, it's important to ensure that the new color handling works consistently across different themes and operating systems.
Please run the following verification steps:
- Test on different operating systems (Windows, macOS, and various Linux distributions, including Ubuntu).
- Test with different Eclipse themes (light, dark, and any custom themes).
- Verify that the information background color is visible and consistent in all scenarios.
This script will help identify other areas in the codebase where colors are set or theme-related preferences are used, which may need similar updates or could be affected by this change.
✅ Verification successful
Color consistency verified successfully
The changes in
StyledInfoText.javaare localized and do not impact other parts of the codebase. All usages ofCascadingColorRegistryare confined to this file, ensuring that color handling remains consistent across different themes and operating systems.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: List all color-related preferences to help with manual verification # Test: List all color-related preferences echo "Color-related preferences:" rg --type java -i 'color|theme' . | grep -i 'preference\|setting' # Test: Find usage of CascadingColorRegistry echo "Usage of CascadingColorRegistry:" rg --type java 'CascadingColorRegistry' # Test: Find other places where background colors are set echo "Other background color settings:" rg --type java 'setBackground'Length of output: 5786
Script:
#!/bin/bash # Description: Find all usages of CascadingColorRegistry and related color settings. # Find all instances where CascadingColorRegistry is used echo "Searching for CascadingColorRegistry usages:" rg --type java 'CascadingColorRegistry' # Find all files that set background colors using ColorRegistry echo "Searching for setBackground usages with ColorRegistry:" rg --type java 'ColorRegistry.*setBackground' # Find all theme-related color settings across the codebase echo "Searching for theme-related color settings:" rg --type java 'theme.*color|ColorConstants' # Find all preferences related to color settings echo "Searching for color preference settings:" rg --type java 'getPreferenceColor|COLOR_'Length of output: 4327
|
@sigmaaa hi ! Tested: LGTM 👍 |
Description
The system background info color could be barely visible on Ubuntu and other OS:
To fix it, but keep the colors look and feel like in the OS, I've changed the system color to the information background color from the color registry which is different based on the selected theme, which is also helpful with different color themes.
The color is also changeable via eclipse preferences like this: General -> Appearance -> Colors and Fonts -> Basic -> Information background color
Fixes # (IEP-1331
Type of change
Please delete options that are not relevant.
How has this been tested?
verify background color on ubuntu
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
StyledTextcomponent to align with the current theme.Bug Fixes