Use checkbox menu in crate selection. by poelzi · Pull Request #1341 · mixxxdj/mixxx

poelzi · 2017-09-09T01:02:07Z

Switch from normal QAction menu to a composition of QCheckBox and QActionWidget.
The checkbox shows in which crates the selection is in.
Changing the crates selection does not collapse the menu, which allows
much easier categorization of tracks without going through the menu from scratch.

daschuer · 2017-09-10T15:29:05Z

Thank you for this PR. Nice Idea.

It work well for a single track, unfortunately not well for multiple tracks. I think we need a three state check box or something, for the case where some of the selected tracks are in a crate and some not.

Nit: Please move the pointer asterisk next to QWidget to meet our coding style.

Be fore merge, we need your permission.
Please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ and comment here when done.

@gramanas: how does this merge with your crate hierarchy branch?

gramanas · 2017-09-11T08:56:40Z

I can't check it atm, but this is based on master and not the new library redesign. Since it only changes widget files, I don't think it'll be hard to resolve the merge conflicts, but I can definitely see some.

poelzi · 2017-09-12T21:19:13Z

I implemented the tristate when multiple tracks are selected that do not share a crate fully.
Fixed your styling comment as well (I hope ;))

daschuer

Thank you for the changes.
It works nice now.

I have added some comments most of them are to improve the readability of the code.

daschuer · 2017-09-13T06:08:12Z

+        // here. But since the coresponding FK column is indexed the impact on the
+        // performance should be negligible. By reusing an existing query we reduce
+        // the amount of code and the number of prepared SQL queries.
+        CrateTrackSelectResult crateTracks(selectTrackCratesSorted(trackId));


The plural is kind of misleading here.
Isn't it trackCrates? I am unsure any other name?

daschuer · 2017-09-13T06:10:31Z

+        while (crateTracks.next()) {
+            DEBUG_ASSERT(crateTracks.trackId() == trackId);
+            CrateId cid = crateTracks.crateId();
+            if(!trackCrates.contains(cid)) {


I do not understand this. Isn't it always false?

this if condition is redundant, because operator[] has already such a code.

daschuer · 2017-09-13T06:12:35Z

+            pAction->setDefaultWidget(pCheckBox.get());
+
+            // FIXME: we could use the tristate feature here and show grey in case of multi track selection
+            // in which not all track are in the crate


Is this comment outdated?

daschuer · 2017-09-13T19:30:29Z

+
+        const QList<TrackId> trackIds = getSelectedTrackIds();
+
+        QHash<CrateId, QSet<TrackId>> currentCrates(m_pTrackCollection->crates().collectCrateTrackListOfTracks(trackIds));


Please wrap this line around column 80. Vertical scrolling in Github is odd ;-)

daschuer · 2017-09-13T19:41:48Z

+            auto pCheckBox = std::make_unique<QCheckBox>(m_pCrateMenu);
+
+            pCheckBox->setText(crate.getName());
+            pCheckBox->setProperty("crateId", crate.getId().toInt());


use toVariant()

daschuer · 2017-09-13T19:44:20Z


-void WTrackTableView::addSelectionToCrate(int iCrateId) {
+void WTrackTableView::addRemoveSelectionInCrate(QWidget* qc) {
+    CrateId crateId = static_cast<CrateId>(qc->property("crateId"));


CrateId has a QVariant constructor, so you can omit the static cast.

daschuer · 2017-09-13T19:50:05Z

-        crateId = CrateFeatureHelper(
-                m_pTrackCollection, m_pConfig).createEmptyCrate();
+    // we need to disable tristate again as the mixed state will now be gone and can't be brought back
+    if (qc->property("tristate").toBool() == true) {


Instead of using string properties, we should use a qobject_cast at the very beginning of the function and check for null.

I tried different techniques for hours, but this was the only only I got working properly. I'm quite open if someone gets this running using more slick code, but as I think it's none critical code from performance aspects, I think it's OK.
I will write some more optimal SQL queries in a separate pull request which should be a much better optimization.

See poelzi#1

daschuer · 2017-09-13T20:53:20Z

+            // FIXME: we could use the tristate feature here and show grey in case of multi track selection
+            // in which not all track are in the crate
+            if(currentCrates.contains(crate.getId())) {
+                if(currentCrates[crate.getId()].size() != trackIds.size()) {


Ok, this works, but it took me quite a while to find out how.
I think this requires some comments.
currentCrates should also be renamed ... mmm
or can we move the size compare if condition to collectCrateTrackListOfTracks ?

My thought was, that the API maybe more useful in the future. Giving a list of tracks and getting a hash with the crates and the containing tracks. Maybe I was overthinking it. I changed the api to just count the tracks in the crate.

daschuer · 2017-09-13T20:55:08Z

+
+            // FIXME: we could use the tristate feature here and show grey in case of multi track selection
+            // in which not all track are in the crate
+            if(currentCrates.contains(crate.getId())) {


contains() and a subsequent [] is a double hash lookup. Use find();

poelzi · 2017-09-15T13:14:59Z

Thanks for the review. I haven't user QT C++ before and haven't touched C++ for a very long time.
I hope I have addressed all points. Unfortunately I haven't found a way to avoid string properties in a slick way, but I don't think it's critical in any way.

uklotzde · 2017-09-16T13:29:20Z

+        // the amount of code and the number of prepared SQL queries.
+        CrateTrackSelectResult trackCrates(selectTrackCratesSorted(trackId));
+        while (trackCrates.next()) {
+            rv[trackCrates.crateId()] += 1;


Are you sure that the integer value in each entry is explicitly initialized with 0??? The fundamental type int does not have a default constructor and QHash will leave it uninitialized when inserting a new key/value pair.

As far as I understood the QList documentation. It states that such types are initialized with 0.

uklotzde · 2017-09-16T13:46:11Z

Very nice, I like this feature 👍 Now I can delete my custom branch that allowed me to delete selected tracks from a crate. But I omitted to add visual feedback, if those tracks are actually contained in crates. That's why I never created a PR.

uklotzde · 2017-09-16T13:50:09Z

Proposal: The name of the menu entry should be changed from "Add to Crate" to just "Crates"!

uklotzde · 2017-09-16T13:54:44Z

+
+        const QList<TrackId> trackIds = getSelectedTrackIds();
+
+        QHash<CrateId, int> currentCrates(m_pTrackCollection->crates().\


Unfortunately many queries are executed even if I don't open the Crates menu entry. Each right click on selected tracks now has a noticeable delay when you have many crates. The list of crates with their state should only be populated when the corresponding sub-menu entry becomes visible.

Ok, I will look into optimizing it and change the name to crates :)

daschuer · 2017-09-16T19:23:40Z

        Crate crate;
        while (allCrates.populateNext(&crate)) {
-            auto pAction = std::make_unique<QAction>(crate.getName(), m_pCrateMenu);
+            auto pAction = std::make_unique<QWidgetAction>(m_pCrateMenu);


we have recently introduced "make_parented" to document the qt object tree ownership and to avoid the unused unique_ptr nature.

uklotzde · 2017-11-06T22:31:02Z

@poelzi Any plans to address the open comments?

I must admit that this is a really helpful feature that I merge into my private builds. I would like to see it in the next release(s). I usually have around 50+ crates and the performance penalties seem to be acceptable. We could even discuss to deliver the performance improvements in a follow-up release if (hopefully) we are able to return to a more frequent release cycle ;)

poelzi · 2017-11-07T01:43:32Z

I will hopefully get back addressing all the comments I have on my open pull requests and this one is annoying me as well ;) I had no working development machine for a while or was quite busy.

poelzi · 2017-11-10T02:34:35Z

I was able to optimize everything into one query with a subselect and reuse the CratesSummery code.
Got even slicker :)

Switch from normal QAction menu to a composition of QCheckBox and QActionWidget. The checkbox shows in which crates the selection is in. Changing the crates selection does not collapse the menu, which allows much easier categorization of tracks without going through the menu from scratch. Use tristate when multiple tracks are selected. When mulitple tracks are selected which do not share a crate, use the tristate partially selected to indicate this. Fix styling of pointers. Use QVariant to transport CrateId in checkbox Use optimized SQL query to select crates and count tracks. Unfortunatelly QSqlQuery has no way of binding QLists in WHERE x IN statements. Use make_partented as suggested by daschuer rename "Add to Crates" -> "Crates" to reflect function better

Optimize formatting of string lists for SQL queries

Be-ing · 2017-11-11T17:22:34Z

@poelzi for the future, it's easier to review changes since the last review if new commits are added instead of rebasing.

Be-ing

just some small style nitpicks

Be-ing · 2017-11-11T17:32:07Z

 }

+CrateSummarySelectResult CrateStorage::selectCratesWithTrackCount(const QList<TrackId>& trackIds) const {
+


nitpick: extra blank line here

Be-ing · 2017-11-11T17:32:32Z

+    // Using bindValue did not work, only constructing the SQL string befor.
+    // As TrackId is a int, we are safe here
+    QStringList idstrings;
+    foreach(TrackId id, trackIds) {


Use standard C++11 for loop instead of old Qt foreach macro: for (TrackId id : trackIds) {

Be-ing · 2017-11-11T17:32:40Z

+    } else {
+        return CrateSummarySelectResult();
+    }
+


extra blank line

poelzi · 2017-11-11T17:44:25Z

@Be-ing: I think your nit picks where fixed by the optimization Uwe Klotz did.

Be-ing · 2017-11-11T17:48:42Z

Oh, you're correct. Sorry for the noise.

uklotzde · 2017-11-11T21:37:30Z

The idea of lazily populating the context sub-menus is also mentioned in this bug report:
https://bugs.launchpad.net/mixxx/+bug/1395022
Executing even more synchronous database queries to obtain results that might never be needed will impact the user experience even more.

Again, I like the functionality and would accept the lag in the first place. But we should think about how this can be fixed ASAP.

poelzi · 2017-11-14T18:50:05Z

@uklotzde i totally agree, but the current iteration does not make any more SQL calls then before.

sqlite> SELECT *, (    SELECT COUNT(*) FROM crate_tracks WHERE crates.id = crate_tracks.crate_id and crate_tracks.track_id in (29, 31, 28, 19, 15,135, 267) ) AS track_count, 0 as track_duration FROM crates ORDER BY name;
6|Haha1|0|1|0|0|3|0
4|blbl|0|1|0|0|0|0
3|blubb|0|1|0|0|0|0
5|huhu|0|1|0|0|3|0
10|huhuihaenuae|0|1|0|0|0|0
1|test1|0|1|1|0|2|0
2|test2|0|1|0|0|1|0
7|turaine|0|1|0|0|2|0
8|uaierutaors|0|1|0|0|0|0
9|uiaeuaie|0|1|0|0|1|0
-------- scanstats --------
Loop  1: SCAN TABLE crates USING INDEX sqlite_autoindex_crates_1
         nLoop=1        nRow=10       estRow=1048576  estRow/Loop=1.04858e+06
-------- subquery 1 -------
Loop  1: SEARCH TABLE crate_tracks USING COVERING INDEX sqlite_autoindex_crate_tracks_1 (crate_id=? AND track_id=?)
         nLoop=10       nRow=12       estRow=80       estRow/Loop=8       
---------------------------
Run Time: real 0.000 user 0.000000 sys 0.000000
sqlite>

It's 2 loops instead of one but fully covered by indexes.

uklotzde · 2017-11-14T20:23:03Z

The lazy initialization of 2nd level actions affects multiple use cases and is a cross-cutting concern and could be solved in a separate task.

I vote for integrating this extremly useful feature before improving the performance of all context menus that require database access.

If the song changes while the current history view is open, the selection of songs is lost. Therefor all crate changes are lost. The history view saves and restores the selection through a new usefull api for selecting tracks in the wtracklistview.

Be-ing · 2017-11-19T15:39:37Z

I tested this and it works nicely. It would be cool to have this for Playlists too, but before that is implemented I think this needs to be lazy initialized or right clicking tracks will become even more terribly slow.

Be-ing · 2017-11-21T19:13:54Z

Is there anything left to do here? Ready for merge?

uklotzde · 2017-11-21T21:49:10Z

LGTM.

Some nitpicking about typos and coding style. But I don't think we need another round here. I would like to get the approval of 1 or 2 other reviewers before merging anything myself.

@daschuer How do we verify that the author has signed the contributor agreement?

daschuer · 2017-11-21T23:52:16Z

@poelzi is already a contributor.
Do you have a Launchpad account? Than I can add you to the Mixxx Contributors Group.

poelzi · 2017-11-22T10:41:52Z

@daschuer it's poelzi on launchpad :)

Be-ing · 2017-11-25T14:32:39Z

 }

-void WTrackTableView::addSelectionToCrate(int iCrateId) {
+void WTrackTableView::addRemoveSelectionInCrate(QWidget* pWidget) {


This function name is ambiguous. Does it add or does it remove?

Be-ing · 2017-11-25T14:33:01Z

+void WTrackTableView::addRemoveSelectionInCrate(QWidget* pWidget) {
+    auto pCheckBox = qobject_cast<QCheckBox*>(pWidget);
+    VERIFY_OR_DEBUG_ASSERT(pCheckBox) {
+        qWarning() << "crateId is not ef CrateId type";


"not ef"? Did you mean "not of"?

Be-ing · 2017-11-30T01:19:05Z

+    // assume that all track ids fit into 6 decimal digits and
+    // add 1 character for the list separator.
+    joinedTrackIds.reserve((6 + 1) * trackIds.size());
+    for (const auto trackId: trackIds) {


const auto& trackId to prevent unnecessary copy

Be-ing · 2017-11-30T01:37:06Z

-            connect(pAction.get(), SIGNAL(triggered()), &m_crateMapper, SLOT(map()));
+            pAction->setDefaultWidget(pCheckBox.get());
+
+            if(crate.getTrackCount() == 0) {


Be-ing · 2017-11-30T01:37:19Z

+
+            if(crate.getTrackCount() == 0) {
+                pCheckBox->setChecked(false);
+            } else if(crate.getTrackCount() == (uint)trackIds.length()) {


} else if (

Be-ing · 2017-11-30T01:40:42Z

+        return;
+    }
+
+    foreach(TrackId trackId, trackIds) {


Use modern standard C++ instead of old Qt foreach macro: for (const auto& trackId : trackIds) {

…e/crate_selectbox

uklotzde · 2017-12-10T10:58:17Z

@poelzi Do you have time to address Be's comments and resolve the pending merge conflict, Daniel? I would like to have this in 2.1, because it has proven to be a valuable feature 👍

…e/crate_selectbox

Be-ing · 2017-12-14T00:53:15Z

LGTM, thank you! @poelzi would you be interested in implementing the lazy initialization to alleviate https://bugs.launchpad.net/mixxx/+bug/1733200 for 2.1?

poelzi · 2017-12-14T01:08:29Z

@Be-ing kinda added the required infrastructure in another branch already. I added a widget (qlistview) that displays the crates of the selected track. I used a background thread to fetch the information in background and update the gui when done. If we decide to use one thread for sql queryies and gui support or one thread per component. https://ibb.co/dJsuf6

Be-ing · 2017-12-18T04:31:10Z

@uklotzde ready for merge?

uklotzde · 2017-12-18T18:49:10Z

    return trackIds;
 }

+void WTrackTableView::setSelectedTracks(QList<TrackId> trackIds) {


This parameter should be passed as const-ref to avoid a deep copy of the list. Unfortunately the range-based for loops do not work well with Qt containers. Declaring the loop variable as const-ref is not enough. The container itself must be const! There is already a link in our coding guidelines.

We have this issue at many places and I have done this wrong until recently. Checking the code with clazy could reveal those hidden performance killers.

I will fix this after merge

I already did in #1427

included in 187ec3d by @Be-ing

uklotzde · 2017-12-18T18:50:21Z

Almost ready. Just one minor issue that we should fix, now that we know better ;)

Be-ing · 2017-12-23T08:21:28Z

Thank you for this big usability improvement for crates! Let's continue this momentum to make Mixxx 2.2 the best music library management software. :)

poelzi force-pushed the feature/crate_selectbox branch from 59683aa to 4795bd9 Compare September 12, 2017 21:17

daschuer requested changes Sep 13, 2017

View reviewed changes

poelzi force-pushed the feature/crate_selectbox branch from 4795bd9 to cc8af31 Compare September 15, 2017 13:08

uklotzde reviewed Sep 16, 2017

View reviewed changes

daschuer reviewed Sep 16, 2017

View reviewed changes

uklotzde added the ui label Oct 29, 2017

poelzi force-pushed the feature/crate_selectbox branch from f257b90 to 97a0684 Compare November 10, 2017 02:32

poelzi force-pushed the feature/crate_selectbox branch from bf5080a to 0a75b38 Compare November 11, 2017 01:01

uklotzde and others added 2 commits November 11, 2017 10:14

Optimize formatting of string lists for SQL queries

de8f406

Merge pull request #2 from uklotzde/feature/crate_selectbox

7f2e1e7

Optimize formatting of string lists for SQL queries

Be-ing reviewed Nov 11, 2017

View reviewed changes

uklotzde added this to the 2.1.0 milestone Nov 25, 2017

Be-ing suggested changes Nov 30, 2017

View reviewed changes

Merge branch 'master' of https://github.com/mixxxdj/mixxx into featur…

39d609e

…e/crate_selectbox

poelzi added 2 commits December 14, 2017 01:09

Merge branch 'master' of https://github.com/mixxxdj/mixxx into featur…

6c8b9a1

…e/crate_selectbox

style fixes according to be's comments

341acf1

uklotzde reviewed Dec 18, 2017

View reviewed changes

This was referenced Dec 20, 2017

Clean up track right click context menu #1427

Merged

merge master to 2.1 branch #1438

Merged

daschuer merged commit 341acf1 into mixxxdj:master Dec 23, 2017

esbrandt mentioned this pull request Apr 14, 2018

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

This was referenced Aug 22, 2022

library context actions can block UI when many tracks selected #7669

Open

keyboard navigation appears broken in crate section of track context menu #9016

Open


		const QList<TrackId> trackIds = getSelectedTrackIds();

		QHash<CrateId, QSet<TrackId>> currentCrates(m_pTrackCollection->crates().collectCrateTrackListOfTracks(trackIds));


		const QList<TrackId> trackIds = getSelectedTrackIds();

		QHash<CrateId, int> currentCrates(m_pTrackCollection->crates().\

		}

		CrateSummarySelectResult CrateStorage::selectCratesWithTrackCount(const QList<TrackId>& trackIds) const {

Uh oh!

Conversation

poelzi commented Sep 9, 2017

Uh oh!

daschuer commented Sep 10, 2017

Uh oh!

gramanas commented Sep 11, 2017

Uh oh!

poelzi commented Sep 12, 2017

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poelzi commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uklotzde commented Sep 16, 2017

Uh oh!

uklotzde commented Sep 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uklotzde commented Nov 6, 2017

Uh oh!

poelzi commented Nov 7, 2017

Uh oh!

poelzi commented Nov 10, 2017

Uh oh!

Be-ing commented Nov 11, 2017

Uh oh!

Be-ing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Be-ing Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poelzi commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Be-ing commented Nov 11, 2017

Uh oh!

uklotzde commented Nov 11, 2017

Be-ing Nov 11, 2017 •

edited

Loading

poelzi commented Nov 11, 2017 •

edited

Loading

poelzi commented Nov 14, 2017 •

edited

Loading

Be-ing commented Nov 19, 2017 •

edited

Loading