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

Basic QML Sidebar placeholder #4037

Merged
merged 3 commits into from
Jul 9, 2021
Merged

Basic QML Sidebar placeholder #4037

merged 3 commits into from
Jul 9, 2021

Conversation

Holzhaus
Copy link
Member

Follow-up to #4034.

This now exposes the SidebarModel to QML and adds an actual treeview to the QML skin.
Top level items are displayed, including icons.

For now, those items are not clickable (nothing happens) and the lazy loading mechanic (e.g. browse feature) is not working. For stuff like that to work, some more serious refactoring is needed. The same goes for child item icons.

Unfortunately, Qt Quick Control 2 does not provide a TreeView (they made that a premium feature), so this still uses the old Qt Quick Controls 1 TreeView component. We may evaluate existing solutions or roll our own in the future.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 30, 2021

Qt does have a TreeView for QQC2 now, but obnoxiously it is in the "Qt marketplace" instead of packaged with Qt. It is licensed GPL to pressure companies using Qt as LGPL to pay for proprietary licenses.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 30, 2021

There is some discussion about packaging difficulties with the Qt Marketplace thing on this bug report. Looking at the Git repository, it seems they recently added or are currently working on Qt6 support with CMake.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 30, 2021

Kirigami Addons also has a TreeView.

@Holzhaus
Copy link
Member Author

Hah, I linked the marketplace tree view above, but I didn't notice the "Also available:GPLv3" and thought it was premium only.

@Holzhaus
Copy link
Member Author

Kirigami Addons also has a TreeView.

I'd like to avoid pulling in all kinds of KDE dependencies. I'll check the marketplace tree view first.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 30, 2021

Kirigami does not depend on other KDE Frameworks libraries.

@uklotzde
Copy link
Contributor

uklotzde commented Jul 6, 2021

An MVP with limited functionality is fine. Let's first explore how far we can get with the available components before we pull in new libraries. Maybe we discover a more appropriate solution for the side bar functionality along the way.

I like the lightweight approach of first mimicking the status quo by reusing as much of the existing functionality as possible and to determine any functional or conceptual gaps.

@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 6, 2021

Let's first explore how far we can get with the available components before we pull in new libraries.

I agree. The issue here is that Qt Quick Controls 1 is deprecated and removed in Qt6. So if we want to get it working with Qt6, we need to get rid of the Qt Quick Controls 1 TreeView first.

I already tried to get the Qt Quick 2 tree view working, but there is no cmakelists for Qt5, just Qt 6. I'm looking into it.

Q_INVOKABLE QAbstractItemModel* getSidebarModel();

private:
Library* m_pLibrary;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using QPointer or mixxx::SafeQPointer to prevent dangling pointers?

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Considered mergeable.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

In the long run, we should think about a Sidebar-less library as already mentioned on zulip but I consider this approach fine as well for now.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM, I'll go ahead and merge

@Swiftb0y Swiftb0y merged commit fce26e2 into mixxxdj:main Jul 9, 2021
Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Oct 13, 2021
This reverts commit fce26e2, reversing
changes made to 787b8aa.

Superseedes mixxxdj#4410.
Be-ing added a commit that referenced this pull request Oct 13, 2021
Revert "Merge pull request #4037 from Holzhaus/qml-sidebar"
JaviVilarroig pushed a commit to JaviVilarroig/mixxx that referenced this pull request Oct 26, 2021
This reverts commit fce26e2, reversing
changes made to 787b8aa.

Superseedes mixxxdj#4410.
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