IEP-1376: Linux ESP-IDF Manager buttons issue #1092
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (2)
Line range hint
144-156: Remove redundant MeasureItem listenerThere are two MeasureItem listeners with different height values (25px and 30px). The second one overwrites the first, making the first listener redundant.
Table table = tableViewer.getTable(); table.setHeaderVisible(true); table.setLinesVisible(true); -table.addListener(SWT.MeasureItem, event -> { - event.height = 25; -}); comparator = new ColumnViewerComparator(); tableViewer.setComparator(comparator); setupColumns(); table.addListener(SWT.MeasureItem, e -> { e.height = 30; });
473-486: Optimize multiple layout and redraw callsMultiple layout and redraw calls might impact performance. Consider consolidating them.
removeButton.setSize(cellBounds.width, buttonHeight); -removeButton.redraw(); editor.grabHorizontal = true; editor.grabVertical = true; editor.horizontalAlignment = SWT.CENTER; editor.verticalAlignment = SWT.CENTER; editor.minimumHeight = removeButton.getSize().y; editor.minimumWidth = removeButton.getSize().x; editor.setEditor(buttonComposite, item, cell.getColumnIndex()); -buttonComposite.layout(true, true); -buttonComposite.redraw(); -editor.layout(); -tableViewer.getTable().layout(true, true); +// Single layout call at the table level should be sufficient +tableViewer.getTable().layout(true, true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java(8 hunks)
🔇 Additional comments (4)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/tools/manager/pages/ESPIDFMainTablePage.java (4)
25-25: LGTM: Rectangle import is required for button dimension management
The addition of Rectangle import is appropriate for managing button dimensions within table cells.
274-274: LGTM: Increased column weight improves button visibility
The increased column weight (3) provides more space for the buttons, which helps with the Linux GTK visibility issues.
439-440: LGTM: Button height calculation prevents truncation
The button height calculation ensures buttons fit within cell bounds while maintaining a minimum height of 30 pixels.
Line range hint 424-482: Verify button appearance across platforms
While the changes improve button visibility on Linux, we should verify that they don't negatively impact other platforms.
✅ Verification successful
Let me analyze the search results. The code changes in question involve SWT button styling and layout in a table cell editor. From the search results, I can see that:
- There are no platform-specific UI adjustments for this particular component in the codebase.
- The button styling changes (SWT.PUSH | SWT.FLAT) and layout code are using standard SWT APIs that are well-supported across all platforms.
- The changes made (setting bounds, using FillLayout, setting minimum sizes) are platform-agnostic SWT operations.
Let me generate the final response:
Button styling changes are platform-agnostic and safe
The UI changes use standard SWT APIs and layout mechanisms that are consistently supported across all platforms. The modifications to button appearance and cell layout don't involve any platform-specific code paths or styling that could cause issues on different operating systems.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for platform-specific UI adjustments or known issues
rg -i "gtk|linux|windows|macos" --type java
Length of output: 17428
|
@alirana01 hi ! Tested: able to see and use "Delete" / "Refresh" buttons ✅ LGTM 👍 |
fix for Linux GTK
AndriiFilippov
force-pushed
the
IEP-1376
branch
from
867d9c1 to
08eeedc
Compare
Description
ESP-IDF Manager buttons issue on Linux is due to the GTK issues on Linux, the only workaround is to show the buttons with bounds of cells that works or rather looks better on windows but on Linux the buttons still appear a bit truncated and there is no fix for that yet. This is better in a sense that the user can at least see the buttons.
See the image for linux
for windows
Fixes # (IEP-1376)
Type of change
Please delete options that are not relevant.
How has this been tested?
Use the jira ticket for more information.
Test Configuration:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor