-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add Library on QML #14514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add Library on QML #14514
Conversation
cc8df70
to
d9097f6
Compare
I tried this. The QML GUI opens, but the track list is empty. If I click on a sidebar element like Computer it crashs:
|
That's strange. Looking at |
I can confirm that breaking change - see this change. Sadly, Ubuntu 24.04 is stuck at 6.4.2, which doesn't include the replacement method. Could you please run with the envvar diff --git a/res/qml/LibraryBrowser.qml b/res/qml/LibraryBrowser.qml
index a55206034d..7e994b2bce 100644
--- a/res/qml/LibraryBrowser.qml
+++ b/res/qml/LibraryBrowser.qml
@@ -59,7 +59,7 @@ Rectangle {
required property int row
required property int column
required property bool current
- readonly property var index: treeView.modelIndex(column, row) // FIXME this seems to become "row,column" in future version of Qt (after 6.4). Update to `index` when upgradeing
+ readonly property var index: treeView.index(row, column)
implicitWidth: treeView.width
implicitHeight: depth == 0 ? 42 : 35
@@ -89,13 +89,11 @@ Rectangle {
hoverEnabled: true
acceptedButtons: Qt.LeftButton | Qt.RightButton
onClicked: {
- treeView.selectionModel.select(treeView.selectionModel.model.index(row, 0), ItemSelectionModel.Rows | ItemSelectionModel.Select | ItemSelectionModel.Clear | ItemSelectionModel.Current);
+ treeView.selectionModel.setCurrentIndex(index, ItemSelectionModel.NoUpdate)
treeView.model.activate(index)
- console.log(treeView.modelIndex(column, row))
if (isTreeNode && hasChildren) {
treeView.toggleExpanded(row)
}
- event.accepted = true
}
}
I tried with Qt 6.8 and Also, please note that if you have no playlist or crate, it is expected that the sidebar appears empty, as there is no feedback on this state yet |
I consider this PR mature now. There is missing feature to be added later, such as context menu integration on the sidebar, list action such as playlist or crate creation, but this can be split and added later. Regarding the breaking changes issue raised by @JoergAtGithub (to be confirmed), I suggest we dynamically set the |
d9097f6
to
f78d8ff
Compare
This is quite a big PR. any chance of splitting it? |
I could move out crate and playlist features in a follow up PR? This should reduce the diff by about 500 lines. Would that be sufficient? |
f78d8ff
to
3fab7ee
Compare
thats still 2,2k lines. Most of the C++ looks pretty boilerplate-y, so this might be feasible. |
ded2c6b
to
0fbdcf6
Compare
0fbdcf6
to
8a83d82
Compare
Thanks for your feedback @Eve00000 !
Yep, there are currently performance limitation with the model. I'm hoping this can be fixed later, as the model gets cleaned up a little bit more
Yes, this gets implemented later, in this PR
Yep, this will be implemented later. The reason for that is, more recent version of Qt support this built-in so I didn't want to implement it manually now
Yep, that also a know "limitation", which is likely due to me not understanding well yet how the focus works on QML! Probably can also be revisited next? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some thoughts on qml_owned_ptr
since it was brought to my attention again in #14597
I could run the latest commit on Windows 11:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't take a thorough look at the qml, but I did notice some stuff in the C++ that needs a little more polish. I'm sorry for dragging this out.
// This is a workaround to support Qt 6.4.2, currently shipped on | ||
// Ubuntu 24.04 See | ||
// https://github.com/mixxxdj/mixxx/pull/14514#issuecomment-2770811094 | ||
// for further details | ||
qputenv("QT_QUICK_TABLEVIEW_COMPAT_VERSION", "6.4"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Can you raise an issue so we don't forget about this once Qt 6.5 is baseline?
|
||
class LibraryTableModel; | ||
class TreeItemModel; | ||
class AllTrackLibraryFeature final : public LibraryFeature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk, didn't know qt imposed that directory structure limitation on us. no opinion.
some of my concerns about ptr casts where bogus, I deleted them. |
Thanks for the feedback @JoergAtGithub !
Noted, I have added vertical scrollbar. I will add the horizontal later, once I have made the update to Qt 6.8 as there was quite some changes around TableView and I suspect I will need to refactor all the column sizing/horizontal sizing policy.
No, it's all currently done on the UI thread! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, no complaints. Basically LGTM from my side
tableView.selectionModel.selectRow(row); | ||
tableView.loadSelectedTrackIntoNextAvailableDeck(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works for now, though in the future it would be nice if this didn't have to go through the implicit state of the tableView
.
// FIXME: WaveformOverview is currently disabled due to performance limitation. Like for the legacy UI, a cache likely needs to be implemented to help | ||
// Mixxx.TrackListColumn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this FIXME still apply? Does the overview have perf issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it does, it basically suffers for the same impacting covers, which @JoergAtGithub raised - the fact that the data model is fully operating in the QML thread is creating slow down, but due the fairly hacky state of the model, I don't feel comfortable adding thread yet, and would prefer starting up clean, as I do the 6.8 or 6.9 migration in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, this is also an issue in the legacy GUI then, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the legacy library is already leveraging a worker thread - at least I have not noticed any significant performance issue!
In general this works on Windows. Im fine with merging this as it is, as first step. |
1490e8b
to
67f4125
Compare
Squashed the fixups! Lets get this in for now, and iterate on the async waveform overview/cover, tracked into #15376 |
Just checked this out, works surprisingly well on the surface. Good job. Only surprising thing was that the text in the header doesn't seem to be clipped/stacked quite right. Idk if this is our bug or Qt. Screencast.From.2025-09-19.13-48-41.mp4Also got a bunch of qml warnings related to
Got this one spammed 30 times in a row.
|
2b9ea64
to
9ec873a
Compare
Thanks for your test! I should have fixed all those warning hopefully as well as the clipping issue. Only one remaining is:
This was introduced by the Saved Jump feature - this comes to the fact that we later on brought capability to have the jump icon to have a backward and forward version, but the QML interface has this now redundant early checks I need to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I can confirm the warnings I complained about are gone (there are still some, but they're unrelated to this PR)
9ec873a
to
426c3e5
Compare
This API might be interesting for the future: https://www.qt.io/blog/model-to-sort-and-filter-the-data-on-the-fly |
Oh great ! I know there was improvement already on 6.5, but I could wait for 6.10 to add that, though this probably means we might struggle to support native Ubuntu 26.04 (not sure if they would stick to 6.8 LTS) |
Depends of #14516
Alternative to #12897 and #4594
Demo
Track menu (basic)
Column sorting
Library feature (Playlist)
Library feature (Computer)
(Note: resizable column is only supported in Qt 6.5 - might need to implement our own to keep support to Ubuntu 24.04 )
Bonus: file drag visual
Screencast.From.2025-03-24.14-38-13.mp4