Skip to content

Slight rework of App Settings list#13708

Merged
DonLakeFlyer merged 1 commit intomasterfrom
AppSettings
Dec 7, 2025
Merged

Slight rework of App Settings list#13708
DonLakeFlyer merged 1 commit intomasterfrom
AppSettings

Conversation

@DonLakeFlyer
Copy link
Copy Markdown
Collaborator

  • Add divider support for spacing between groups
  • Alphabetize center group

Screenshot 2025-12-06 at 6 54 53 PM

Copilot AI review requested due to automatic review settings December 7, 2025 02:59
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 refactors the App Settings UI to add visual dividers between setting groups and alphabetizes the middle group of settings. The changes introduce a Loader-based pattern to conditionally render either dividers or setting buttons, replacing the previous direct Repeater approach.

Key Changes:

  • Added divider support using a Loader component pattern to create visual spacing between setting groups
  • Reorganized settings in SettingsPagesModel.qml to alphabetize the middle group (ADSB Server through Telemetry)
  • Refactored button rendering to use Components within Loaders instead of direct Repeater delegates

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/UI/AppSettings/SettingsPagesModel.qml Added three "Divider" entries and reordered middle group settings alphabetically; moved Telemetry and Logging items to new positions
src/QmlControls/AppSettings.qml Introduced dividerComponent and buttonComponent with Loader pattern; added model property proxies; removed column spacing in favor of divider-based spacing
src/QmlControls/SettingsButton.qml Removed extraneous blank lines

Comment on lines +135 to +137
property var modelIconUrl: iconUrl
property var modelUrl: url
property var modelPageVisible: pageVisible
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Property naming inconsistency: The Loader creates properties prefixed with "model" (e.g., modelName, modelIconUrl) to pass data to the loaded component. However, these properties will be undefined for dividers since dividers in the ListModel only have a name property and no iconUrl, url, or pageVisible.

This will cause QML warnings when dividers are loaded. Consider checking for dividers before creating these properties or providing default values:

property var modelName: name
property var modelIconUrl: name === "Divider" ? "" : iconUrl
property var modelUrl: name === "Divider" ? "" : url
property var modelPageVisible: name === "Divider" ? function() { return false } : pageVisible
Suggested change
property var modelIconUrl: iconUrl
property var modelUrl: url
property var modelPageVisible: pageVisible
property var modelIconUrl: name === "Divider" ? "" : iconUrl
property var modelUrl: name === "Divider" ? "" : url
property var modelPageVisible: name === "Divider" ? function() { return false } : pageVisible

Copilot uses AI. Check for mistakes.
Component {
id: dividerComponent

Item { height: ScreenTools.defaultFontPixelHeight / 2 }
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The divider component should have a width constraint to match the button layout. Currently it only sets height, which may cause layout issues. Consider adding:

Component {
    id: dividerComponent
    
    Item { 
        Layout.fillWidth: true
        height: ScreenTools.defaultFontPixelHeight / 2 
    }
}
Suggested change
Item { height: ScreenTools.defaultFontPixelHeight / 2 }
Item {
Layout.fillWidth: true
height: ScreenTools.defaultFontPixelHeight / 2
}

Copilot uses AI. Check for mistakes.
Comment on lines +126 to +139
Repeater {
id: buttonRepeater
model: settingsPagesModel

Loader {
Layout.fillWidth: true
sourceComponent: name === "Divider" ? dividerComponent : buttonComponent

property var modelName: name
property var modelIconUrl: iconUrl
property var modelUrl: url
property var modelPageVisible: pageVisible
}
}
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The showSettingsPage function (lines 36-44) is broken by this refactoring. It accesses buttonRepeater.itemAt(i) expecting a SettingsButton with text property and clicked() method, but now returns a Loader. This will cause runtime errors when showSettingsPage is called (e.g., from MainWindow.qml line 157).

Fix by accessing the loaded item and skipping dividers:

function showSettingsPage(settingsPage) {
    for (var i=0; i<buttonRepeater.count; i++) {
        var loader = buttonRepeater.itemAt(i)
        if (loader && loader.item && loader.item.text === settingsPage) {
            loader.item.clicked()
            break
        }
    }
}

Copilot uses AI. Check for mistakes.
* Add divider support for spacing between groups
* Alphabetize center group
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@DonLakeFlyer DonLakeFlyer merged commit 97ab51f into master Dec 7, 2025
27 checks passed
@DonLakeFlyer DonLakeFlyer deleted the AppSettings branch December 7, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants