Library-layout fix GUI lag #1740

jmigual · 2018-07-05T18:34:49Z

Fix bug re-introduced in #1714

However there's a problem when adding tracks as the signal tracksAdded(QSet<TrackId>) from TrackDAO does not provide all the ids for the new added tracks and only a small subsample.

Does anyone have an idea of why this may happen? Am I using the proper signal to see if new tracks have been added?

jmigual · 2018-07-06T06:40:45Z

The build fails because it does not find the method intersects from QSet. Which version of Qt 5 are we using for POSIX builds? Qt 5.6 already has this method in its documentation.

Be-ing · 2018-07-06T16:52:07Z

The build fails because it does not find the method intersects from QSet. Which version of Qt 5 are we using for POSIX builds? Qt 5.6 already has this method in its documentation.

It looks like Travis is using an ancient version of Ubuntu stuck on Qt 5.2. Edit .travis.yml to update it.

Travis' macOS builds are using Qt 5.11. I am not sure why those are failing, but it is not related to this PR. I attempted to fix that in #1722.

Be-ing

Thank you for continuing to work on this.

Please move all SQL code into a separate class. There should be zero SQL in GUI classes. Refer to #1304 for an example.

However there's a problem when adding tracks as the signal tracksAdded(QSet) from TrackDAO does not provide all the ids for the new added tracks and only a small subsample.

I can confirm that adding tracks to the tree is not working. I tried rescanning my library to add new tracks. While they appeared in the track table, they did not appear in the tree.

Be-ing · 2018-07-06T16:30:23Z

src/library/treeitem.h

@@ -86,8 +86,8 @@ class TreeItem final {
        m_label = label;
    }

-    inline const QString& getLabel() const {
-        count = getTrackCount();


Please do not make commits that do not compile. This makes it more cumbersome to use git bisect later. If you realize after you make a commit that it does not compile, use git commit --amend or git rebase -i to fix it.

Oh sorry I won't do it again

Be-ing · 2018-07-06T16:30:58Z

src/library/treeitem.h

@@ -86,8 +86,8 @@ class TreeItem final {
        m_label = label;
    }

-    inline const QString& getLabel() const {
-        count = getTrackCount();
+    inline const QString getLabel() const {


Why doesn't this return a reference anymore?

Be-ing · 2018-07-06T16:58:11Z

src/library/treeitem.cpp

@@ -102,6 +102,14 @@ TreeItem* TreeItem::appendChild(
    return pChild;
 }

+TreeItem*TreeItem::insertChild(int row, const QString& label, const QVariant& data) {


"TreeItem*TreeItem::insertChild"? Does this commit compile?

I am amazed that this does compile in fact. I will check it again and "fix" it just to avoid possible problems.

daschuer · 2018-07-06T21:26:50Z

The build fails because it does not find the method intersects from QSet. Which version of Qt 5 are we using for POSIX builds? Qt 5.6 already has this method in its documentation.

Ubuntu Trusty, supported until April 2019 has Qt5.2.
Ubuntu Xenial, supported until April 2021 has QT5.5

The libraryredesign release 2.3 is scheduled for 2019-03-31 so I am afraid we should head for a alternative solution.

I propose to add a wrapper into util/compatibility.h and re-implement the function
https://github.com/qt/qtbase/blob/a37dd93defd91b79fb6730d0ff0515a66a0d3972/src/corelib/tools/qset.h#L313

Be-ing · 2018-07-06T22:09:56Z

Ubuntu Trusty, supported until April 2019 has Qt5.2.

Supported by Canonical, which is a company that pays people to continue to backport critical fixes to old packages. Applications have zero obligation to ensure their new versions work on old operating systems, nor is this common practice. Qt 5.2 was released in December 2013. Following the logic that new versions of Mixxx must support the oldest OS supported by Canonical, Mixxx cannot use new Qt features until 6 years after they have been released! I find that ridiculous. This isn't even what Canonical does. They don't ship major new versions of applications for LTS releases; they only make minimal fixes. Ubuntu's Trusty repositories still ship Mixxx 1.10.1.

I propose to add a wrapper into util/compatibility.h and re-implement the function

No. Qt has already implemented the function. We should not duplicate code. It is not applications' responsibility to overcompensate for a users' out of date system.

Be-ing · 2018-07-06T22:38:05Z

From Ubuntu's LTS policy:

we define the LTS to be:
Enterprise Focused: We are targeting server and multiple desktop installations, where the average user is moderately risk averse.
...
clearly state that it is not:
A Feature-Based Release: We will focus on hardening functionality of existing features, versus introducing new ones
Cutting Edge

This is the opposite of new Mixxx releases, which are focused on providing new features for use on personal laptops.

daschuer · 2018-07-07T06:48:55Z

In the history of Mixxx we have always supported all supported Ubuntu version if possible with reasonable effort.
Here we are talking about just a few lines wrapping code, no reason to break Xenial for this, which is still the most used distribution.
I agree, that we can breake this strategy for really hard to solve issues.

jmigual · 2018-07-07T08:36:15Z

Please move all SQL code into a separate class. There should be zero SQL in GUI classes. Refer to #1304 for an example.

Ok, I will move the code as soon as I can work on it again (until August)

Regarding the Qt issue I think that we should discuss it in the mailing list or Zulip and decide in a Qt version for the next release.
Can't we just deploy a different Qt version in Ubuntu with our binaries?

daschuer · 2018-07-07T13:58:28Z

Can't we just deploy a different Qt version in Ubuntu with our binaries?

This might be get ripped off by the package maintainer. Here It is IMHO straight forward to backport the Qt 5.6 function. No need to ditch Xenial compatibility here.

Be-ing · 2018-07-07T15:14:56Z

Zulip discussion: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/minimum.20requirements.20policy

In the history of Mixxx we have always supported all supported Ubuntu version if possible with reasonable effort.

And here we are publishing our first Qt5 release as upstream is starting to plan for Qt6.

Ok, I will move the code as soon as I can work on it again (until August)

Okay thank you. I recognize that will take a lot of work. It doesn't necessarily have to be done in this pull request. We'll never get database transactions out of the GUI thread or move to QML with such tight coupling of GUI classes and SQL code.

Can't we just deploy a different Qt version in Ubuntu with our binaries?

Statically linking a small library may be reasonable, but Qt is huge.

uklotzde · 2018-07-09T18:40:41Z

Did you ~~know~~notice that LibraryScanner hosts its own instance of TrackDAO in a separate thread? It forwards added tracks one by one to the main TrackDAO via databaseTrackAdded(). Unfortunately these tracks do not result in any tracksAdded() signal from the main TrackDAO, as far as I understand.

nikmartin · 2018-08-31T17:01:07Z

FYI, because of this PR I built https://github.com/nikmartin/mixxxLibraryBuilder to build a large library of very small tagged, valid mp3 files to test this branch with. A 1,000,000 mp3 library is only .5gb

github-actions · 2020-10-20T00:21:56Z

This PR is marked as stale because it has been open 90 days with no activity.

Holzhaus · 2021-04-02T13:10:48Z

The minimum requirements issue should have fixed itself by now. This this PR still relevant or should we close this?

ronso0 · 2021-04-02T19:57:40Z

The target branch is stale and unmergable IIRC due to major perfomrmance issues.
Can be closed IMO.

jmigual added 12 commits July 2, 2018 13:38

Removed unnecessary duplicated code

2672483

Add set to store TrackIds

6ab3588

Fix compilation issues

8c4c7b3

Change TrackTreeModel to use TrackId instead of the track count

cba1aa6

Remove some unnecessary commented code

2dd10c3

Use TrackId when creating the tree with folders

01b65f0

Add code for removed tracks

d44a964

Continue working in changing the tree when needed

00f19a6

Add method to TreeItem to insert a child at a row

6b226d2

Finish methods for TracksTreeModel

9e39b88

Finish methods for LibraryFolderModel

6462d60

Fix bug when removing tracks

f617bf3

Be-ing suggested changes Jul 6, 2018

View reviewed changes

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 20, 2020

ronso0 marked this pull request as draft February 14, 2021 21:56

ronso0 closed this Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library-layout fix GUI lag #1740

Library-layout fix GUI lag #1740

jmigual commented Jul 5, 2018

jmigual commented Jul 6, 2018

Be-ing commented Jul 6, 2018 •

edited

Loading

Be-ing left a comment •

edited

Loading

Be-ing Jul 6, 2018

jmigual Jul 7, 2018

Be-ing Jul 6, 2018

Be-ing Jul 6, 2018

jmigual Jul 7, 2018

daschuer commented Jul 6, 2018

Be-ing commented Jul 6, 2018 •

edited

Loading

Be-ing commented Jul 6, 2018

daschuer commented Jul 7, 2018

jmigual commented Jul 7, 2018

daschuer commented Jul 7, 2018

Be-ing commented Jul 7, 2018

uklotzde commented Jul 9, 2018 •

edited

Loading

nikmartin commented Aug 31, 2018

github-actions bot commented Oct 20, 2020

Holzhaus commented Apr 2, 2021

ronso0 commented Apr 2, 2021

Library-layout fix GUI lag #1740

Library-layout fix GUI lag #1740

Conversation

jmigual commented Jul 5, 2018

jmigual commented Jul 6, 2018

Be-ing commented Jul 6, 2018 • edited Loading

Be-ing left a comment • edited Loading

Choose a reason for hiding this comment

Be-ing Jul 6, 2018

Choose a reason for hiding this comment

jmigual Jul 7, 2018

Choose a reason for hiding this comment

Be-ing Jul 6, 2018

Choose a reason for hiding this comment

Be-ing Jul 6, 2018

Choose a reason for hiding this comment

jmigual Jul 7, 2018

Choose a reason for hiding this comment

daschuer commented Jul 6, 2018

Be-ing commented Jul 6, 2018 • edited Loading

Be-ing commented Jul 6, 2018

daschuer commented Jul 7, 2018

jmigual commented Jul 7, 2018

daschuer commented Jul 7, 2018

Be-ing commented Jul 7, 2018

uklotzde commented Jul 9, 2018 • edited Loading

nikmartin commented Aug 31, 2018

github-actions bot commented Oct 20, 2020

Holzhaus commented Apr 2, 2021

ronso0 commented Apr 2, 2021

Be-ing commented Jul 6, 2018 •

edited

Loading

Be-ing left a comment •

edited

Loading

Be-ing commented Jul 6, 2018 •

edited

Loading

uklotzde commented Jul 9, 2018 •

edited

Loading