Skip to content
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

QML: Library Improvements #4567

Merged
merged 14 commits into from
Jan 14, 2022
Merged

Conversation

Holzhaus
Copy link
Member

Currently unfinisehd because the developer tools are missing in the QML skin since #4327.

@ywwg
Copy link
Member

ywwg commented Dec 14, 2021

do you want to revert that change, or would it be better to try to re-add the developer tools separately? My understanding is that because of the qt6 change, QML is our top priority right now, so it seems like making it easy to develop for QML is important. I haven't been following the qml work closely so I don't know how big a deal it would be to re-add developer tools

@Swiftb0y Swiftb0y self-assigned this Dec 15, 2021
@Holzhaus Holzhaus force-pushed the qml-library-improvements branch from f98435c to afd0fd8 Compare December 15, 2021 17:34
@github-actions github-actions bot added the build label Dec 15, 2021
@Holzhaus Holzhaus force-pushed the qml-library-improvements branch 3 times, most recently from 9c36464 to cdd5ff1 Compare December 22, 2021 20:46
@Holzhaus
Copy link
Member Author

If you want to try this out: The MoveVertical/SelectTrackKnob/LoadSelectedTrack COs already work, which makes the QML TrackList usable from a controller. To make this possible, I reimplemented LibraryControl in pure QML. Please let me know if you have any comments regarding the implementation, unsure if this is the best way.

@Holzhaus Holzhaus force-pushed the qml-library-improvements branch 2 times, most recently from df763ca to 5d25756 Compare December 23, 2021 23:12
@Swiftb0y
Copy link
Member

Please rebase to eliminate conflicts because of #4582

res/qml/LibraryControl.qml Outdated Show resolved Hide resolved
res/qml/LibraryControl.qml Show resolved Hide resolved
@Holzhaus Holzhaus force-pushed the qml-library-improvements branch 2 times, most recently from dca16bf to 22bd247 Compare December 25, 2021 16:44
@ronso0
Copy link
Member

ronso0 commented Dec 26, 2021

Do you intend to 'duplicate' librarycontrol so that it's not only responsible of the tracks table but also of managing controls/signals for the sidebar, feature bar and search bar?

Reason I'm asking is I have a WIP branch where I'm moving stuff out of LibraryControl::emitKeyEvent into the respective slots connected to the controls like GoToItem, MoveVertical etc. by making use of the FocusWidget enum. All that with QML and future multi-pane library design in mind which can re-use FocusWidget to some degree (don't want to make the transition to QML more complicated but hopefully easier).

@Holzhaus
Copy link
Member Author

Do you intend to 'duplicate' librarycontrol so that it's not only responsible of the tracks table but also of managing controls/signals for the sidebar, feature bar and search bar?

Reason I'm asking is I have a WIP branch where I'm moving stuff out of LibraryControl::emitKeyEvent into the respective slots connected to the controls like GoToItem, MoveVertical etc. by making use of the FocusWidget enum. All that with QML and future multi-pane library design in mind which can re-use FocusWidget to some degree (don't want to make the transition to QML more complicated but hopefully easier).

Well, ideally the LibraryControl c++ would be independent of QWidgets and not invoke any methods on widgets at all. Instead it could just emit signals that the Library widget, the sidebar widget, etc. connect to. Then we wouldn't need the qml LibraryControl implementation at all.

@ronso0
Copy link
Member

ronso0 commented Dec 26, 2021

So librarycontrol will also be unaware of 'widget' focus and always fire signals to all connected components?

@Holzhaus Holzhaus force-pushed the qml-library-improvements branch from 22bd247 to cfcb1aa Compare December 26, 2021 23:17
@Holzhaus
Copy link
Member Author

So librarycontrol will also be unaware of 'widget' focus and always fire signals to all connected components?

Not necessarily. It should be possible to check the focused_widget CO value and take that into account. But there aren't that many widgets anyway, right?

@ronso0
Copy link
Member

ronso0 commented Dec 27, 2021

But there aren't that many widgets anyway, right?

Not yet ;) I was just thinking of the library redesign, and that there will probably 2+ sets of [tracks, side/feature bar, list view and searchbar], and to me it feels more effective making librarycontrol the switch for library controls (where all present library widgets are registered when the skin is parsed), instead of letting all widgets deal with the signals independently depending on widget focus
Furthermore I still like to make all library controls work without requiring the Mixxx window having focus but with an alternative pseudo-focus property (see lp:1771611)
So my questions are simply ment to remind of that and that we consider it while setting the new library base in QML.

@Holzhaus Holzhaus marked this pull request as ready for review December 30, 2021 23:55
res/qml/Library.qml Outdated Show resolved Hide resolved
res/qml/LibraryControl.qml Outdated Show resolved Hide resolved
res/qml/LibraryControl.qml Outdated Show resolved Hide resolved
res/qml/LibraryControl.qml Outdated Show resolved Hide resolved
res/qml/LibraryControl.qml Outdated Show resolved Hide resolved
res/qml/LibraryControl.qml Outdated Show resolved Hide resolved
@Holzhaus Holzhaus force-pushed the qml-library-improvements branch 3 times, most recently from 160cb7e to 712f90f Compare January 10, 2022 22:31
@Holzhaus Holzhaus force-pushed the qml-library-improvements branch from 712f90f to 3542753 Compare January 10, 2022 23:03
@Holzhaus
Copy link
Member Author

I had to rebase on #4616 because for some reason CI complained about MathUtils.mjs.

@Holzhaus Holzhaus marked this pull request as draft January 10, 2022 23:06
res/qml/Library.qml Outdated Show resolved Hide resolved
@Holzhaus Holzhaus force-pushed the qml-library-improvements branch from 3542753 to 1ed44fe Compare January 11, 2022 18:57
@Holzhaus Holzhaus marked this pull request as ready for review January 11, 2022 18:58
group: root.group
key: "LoadSelectedTrack"
onValueChanged: {
if (value == 0 || !root.enabled)
Copy link
Member

@Swiftb0y Swiftb0y Jan 11, 2022

Choose a reason for hiding this comment

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

A QtObject does not have an enabled property. Receiving keyboard and mouse events is a QtQuick feature and thus managed through the QtQuick qml base type Item. This is probably why the QtObject version breaks. Maybe because its breaking on evaluating !undefined since root.enabled is undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Also if removing the Item dependency still didn't fix it, maybe we're using QtObject wrong and the base should be a Component instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Item doesn't have an enabled property either, I was declared here. And in any case, it should throw a warning, not cause a segfault. Component doesn't work either, same issue as with QtObject.

Copy link
Member

@Swiftb0y Swiftb0y Jan 11, 2022

Choose a reason for hiding this comment

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

It was declared here

Sorry, I missed that, still Item does have an enabled property. I guess the declared one shadows the Items one. https://doc.qt.io/qt-5/qml-qtquick-item.html#enabled-prop

And in any case, it should throw a warning, not cause a segfault.

Yes, of course. The crash is still a bug, though I thought I might have found the issue thats triggering the bug and we could work around it, but I was wrong.

@Holzhaus
Copy link
Member Author

Merge?

@Swiftb0y
Copy link
Member

Currently does not work at all with touchscreens so we should address that soon. Other than that, LGTM

@Swiftb0y Swiftb0y merged commit f1c7217 into mixxxdj:main Jan 14, 2022
@Holzhaus
Copy link
Member Author

Currently does not work at all with touchscreens so we should address that soon. Other than that, LGTM

I don't have a Touchscreen, but clicking on a row should work?

@Swiftb0y
Copy link
Member

Yes that does work, but the library is not flickable as you would expect from a touch-enabled UI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants