Conversation
14e7d76 to
27cbfcc
Compare
27cbfcc to
7f77c25
Compare
|
I took the liberty of amending the commits with the requested changes @ywwg! |
|
Are we happy to merge this now @ywwg ? |
| Settings.StatsPerformance { | ||
| } |
There was a problem hiding this comment.
Can't reproduce. Can you confirm this is a regression?
There was a problem hiding this comment.
Looks to be impacting Qt 6.10 only - looks like Object.values on QML list have a new behaviour/flawed. Fixed now!
7f77c25 to
9652a65
Compare
9652a65 to
10c4070
Compare
|
Friendly ping @JoergAtGithub :) |
|
Friendly ping @mixxxdj/developers As our agreement on the operating mode for the QML project, I would like to get that merged since this is not touching on any 2.x code is part of the continuous improvement of 3.0. |
res/qml/Settings.qml
Outdated
| readonly property int selectedIndex: categoryItem && categoryItem.selectedIndex !== undefined ? categoryItem.selectedIndex : 0 | ||
| readonly property var tabs: categoryItem ? categoryItem.tabs : [] | ||
| readonly property int selectedIndex: root.activeCategory?.selectedIndex ?? 0 | ||
| readonly property var tabs: activeCategory?.tabs ?? [] |
There was a problem hiding this comment.
| readonly property var tabs: activeCategory?.tabs ?? [] | |
| readonly property var tabs: root.activeCategory?.tabs ?? [] |
res/qml/Settings.qml
Outdated
| readonly property real smallScreenWidth: 1200 | ||
| function updateActiveCategory() { | ||
| root.activeCategory?.deactivated(); | ||
| root.activeCategory = Object.values(managerItem.data)[categoryList.currentIndex] ?? null; |
There was a problem hiding this comment.
| root.activeCategory = Object.values(managerItem.data)[categoryList.currentIndex] ?? null; | |
| root.activeCategory = managerItem.data[categoryList.currentIndex] ?? null; |
Great work on migrating away from Loader!
I noticed an issue here: I think, we shouldn't use Object.values() on managerItem.data. Because data is a C++ QQmlListProperty under the hood, running standard JavaScript reflection like Object.values() on it leads to unpredictable behavior in the QML engine (it ends up pulling internal C++ properties and array length values instead of just the items themselves).
I tested this with a test file like this:
import QtQuick 2.0
Item {
id: root
Item { id: child1; objectName: "child1" }
Item { id: child2; objectName: "child2" }
Component.onCompleted: {
let vals = Object.values(root.data);
console.log("vals:", vals);
Qt.quit();
}
}which outputs:
qml: vals: [QQuickItem(0xa54c6b3a0, "child1"),QQuickItem(0xa54c6b480, "child2"),QQuickItem(0xa54c6b480, "child2")]As you can see from the trailing output, the QQuickItem(..., "child2") pointer is completely duplicated at the end of the array.
There was a problem hiding this comment.
Yeah, good point, I had also suffered from the same issue here
There was a problem hiding this comment.
ah, i missed the other convo. great, we can keep this behaviour in mind.
|
Thanks for your feedback @xARSENICx , I have updated the PR now |
|
The PR LGTM now. |
|
@mixxxdj/developers merge? |
|
there are failing tests, please squash the fixup commit. and is the android failure fixable? |
e5e9e3c to
49b1192
Compare
|
Squashed the commits.
No, this is related to this issue, which I believe is being looked at here |
|
Should be good to go now @ywwg ! |
JoergAtGithub
left a comment
There was a problem hiding this comment.
With this PR Mixxx is not starting up anymore:
warning [QQmlThread] file:///D:/mixxx/res/qml/Settings/Category.qml:10:12: Duplicate signal name: invalid override of property change signal or superclass signal
warning [Main] QQmlApplicationEngine failed to load component
warning [Main] file:///D:/mixxx/res/qml/main.qml:639:5: Type Skin.Settings unavailable
warning [Main] file:///D:/mixxx/res/qml/Settings.qml:280:21: Type Settings.Interface unavailable
warning [Main] file:///D:/mixxx/res/qml/Settings/Interface.qml:4:1: Cannot load library D:\mixxx\build\x64__portable\Qt6\qml\QtQuick\Dialogs\qtquickdialogsplugin.dll: Das angegebene Modul wurde nicht gefunden.
critical [Main] Failed to load QML file "D:/mixxx/res/qml/main.qml"


This PR refactors the QML settings core pop-up to use explicit declaration instead of using
Loader. This is done in order to fix issues with Window integration, such a file or folder dialog, as it looks like loaders seem to create strange bugs.