Plan: More work on toolbar and other things #13756
Conversation
* Reworked buttons * Also added MissionStatus expand to ToolStrip
There was a problem hiding this comment.
Pull request overview
This PR redesigns the Plan View toolbar to improve usability by consolidating controls, adding visual icons, and introducing a hamburger menu for less-frequently-used actions. When the Mission Status panel is collapsed, a new expand button appears in the ToolStrip. The changes align with the goal of creating a cleaner, more intuitive toolbar interface while maintaining all existing functionality.
Key changes include:
- Toolbar buttons now display icons (Open, Save, Upload, Clear) with consistent visual design
- Less-frequent actions (Save as KML, Download) moved to a hamburger menu dropdown
- Mission Status panel can be expanded via a new "Stats" button in the ToolStrip when collapsed
- QGCButton component enhanced to better support icon-only and icon-with-text configurations
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/QmlControls/ToolStrip.qml | Simplified QGCFlickable anchoring from explicit edges to anchors.fill for cleaner code |
| src/QmlControls/QGCButton.qml | Refactored property ordering, adjusted icon sizing and vertical padding calculation to accommodate icons |
| src/QmlControls/PlanViewRightPanel.qml | Removed unnecessary topMargin to simplify layout |
| src/QmlControls/PlanView.qml | Added "Stats" expand button to ToolStrip, refactored Mission Status visibility logic with new hidden property and showMissionStatus function |
| src/QmlControls/PlanToolBarIndicators.qml | Major restructure: removed Vehicle/Storage section labels, added icons to buttons, moved Save as KML and Download to hamburger dropdown menu |
| resources/UploadToVehicle.svg | New SVG icon for Upload button featuring upward arrow in circle |
| resources/TrashCan.svg | New SVG icon for Clear button |
| resources/SaveToDisk.svg | New SVG icon for Save/Save As button |
| qgcresources.qrc | Registered three new SVG resources |
| property int _verticalPadding: Math.round(ScreenTools.defaultFontPixelHeight * heightFactor) - (iconSource === "" ? 0 : (_iconHeight - ScreenTools.defaultFontPixelHeight) / 2) | ||
| property real _iconHeight: text.height * 1.5 |
There was a problem hiding this comment.
The _verticalPadding calculation has a circular dependency issue. It depends on _iconHeight which depends on text.height, but text.height may not be initialized when _verticalPadding is first calculated. This could lead to incorrect padding values, especially on initial render. Consider using a fixed value or ScreenTools.defaultFontPixelHeight instead of text.height for the icon height calculation.
| visible: !hidden && _editingLayer == _layerMission && QGroundControl.corePlugin.options.showMissionStatus | ||
|
|
||
| property bool _missionStatusVisible: _planViewSettings.showMissionItemStatus.rawValue | ||
| readonly property bool hidden: _planViewSettings.showMissionItemStatus.rawValue ? false : true |
There was a problem hiding this comment.
The hidden property uses a double negation pattern (? false : true) which is unnecessarily complex. This can be simplified to: readonly property bool hidden: !_planViewSettings.showMissionItemStatus.rawValue. This makes the code more readable and maintainable.
|
|
||
| function _toggleMissionStatsVisibility() { | ||
| function _toggleMissionStatusVisibility() { | ||
| _planViewSettings.showMissionItemStatus.rawValue = _planViewSettings.showMissionItemStatus.rawValue ? false : true |
There was a problem hiding this comment.
The toggle function uses a ternary operator to toggle a boolean value, which is unnecessarily complex. This can be simplified to: _planViewSettings.showMissionItemStatus.rawValue = !_planViewSettings.showMissionItemStatus.rawValue. This makes the intent clearer and is more maintainable.
| id: uploadButton | ||
| text: qsTr("Upload") | ||
| iconSource: "/res/UploadToVehicle.svg" | ||
| enabled: _utmspEnabled ? (!_syncInProgress && UTMSPStateStorage.enableMissionUploadButton) : (!_syncInProgress && _hasPlanItems) |
There was a problem hiding this comment.
The enabled condition has redundant parentheses around each branch of the ternary operator. These can be simplified from (_utmspEnabled ? (!_syncInProgress && UTMSPStateStorage.enableMissionUploadButton) : (!_syncInProgress && _hasPlanItems)) to _utmspEnabled ? !_syncInProgress && UTMSPStateStorage.enableMissionUploadButton : !_syncInProgress && _hasPlanItems for better readability.
| QGCButton { | ||
| Layout.fillWidth: true | ||
| text: qsTr("Download") | ||
| enabled: _utmspEnabled ? !_syncInProgress && UTMSPStateStorage.enableMissionDownloadButton : !_syncInProgress |
There was a problem hiding this comment.
The enabled condition has unnecessary parentheses. Consider simplifying from _utmspEnabled ? !_syncInProgress && UTMSPStateStorage.enableMissionDownloadButton : !_syncInProgress to _utmspEnabled ? !_syncInProgress && UTMSPStateStorage.enableMissionDownloadButton : !_syncInProgress for consistency with other similar conditions.
New toolbar:
Expand Mission Status:
