Crate archives #10997

ywwg · 2022-10-24T15:47:22Z

This is a small feature that creates an Archive subfolder of the crate menu, much like the history playlist now has per-year folders. Users can move a crate to the archive folder if they don't want it to appear in the crate submenu. This requires a small change to the db (the addition of a bool to determine if the crate is in the archive) and a small amount of qt for the new submenu.

This is a draft because it needs more testing and UX improvement

ywwg · 2022-10-24T15:54:47Z

src/library/trackset/crate/cratefeature.cpp

+        if (selectedIsArchived) {
+            QModelIndex archiveIndex = m_pSidebarModel->index(rowCount - 1, 0);
+            m_pSidebarWidget->setExpanded(archiveIndex, true);
+            return m_pSidebarModel->index(selectedRow, 0, archiveIndex);


In my use, I find it not very convenient to keep the archived crate selected. Instead I think it might be nicer to select the crate below where it used to be in the hierarchy. This is because archiving a crate is kind of like "please make this go away" so having the selection follow the archive process doesn't quite make sense.

But why expand the Archive item?

github-actions · 2023-01-23T00:07:07Z

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

…crate into the archive folder Also, make sure to highlight archived crates when selecting tracks.

src/library/trackset/crate/cratefeature.cpp

Co-authored-by: ronso0 <[email protected]>

daschuer

Thank you. This looks like a nice feature.

daschuer · 2023-05-22T12:34:36Z

src/library/trackset/crate/cratestorage.cpp

@@ -569,18 +577,20 @@ bool CrateStorage::onInsertingCrate(
    FwdSqlQuery query(m_database,
            QStringLiteral(
                    "INSERT INTO %1 (%2,%3,%4) "
-                    "VALUES (:name,:locked,:autoDjSource)")
+                    "VALUES (:name,:locked,:archived,:autoDjSource)")


I got a violated Debug Assertion:

warning [Main] QString::arg: 1 argument(s) missing in INSERT INTO %1 (%2,%3,%4) VALUES (:name,:locked,:archived,:autoDjSource) critical [Main] FwdSqlQuery - Failed to prepare statement "INSERT INTO crates (name,locked,archived) VALUES (:name,:locked,:archived,:autoDjSource)" : QSqlError("1", "Unable to execute statement", "4 values for 3 columns") critical [Main] DEBUG ASSERT: "query.isPrepared()" in function bool CrateStorage::onInsertingCrate(const Crate&, CrateId*) at ../../src/library/trackset/crate/cratestorage.cpp:587

ronso0 · 2023-05-22T14:26:14Z

The debug assert in CrateFeature::activateChild needs to just return for crateId = -1 (Archived item)
27e625a

It would be a nice addon if the table view is cleared when clicking the Archived item, currently it sticks to the previous view (which might be any item of any sidebar feature). Just fyi, for the YEAR items in History I created a temporary, locked playlist. Dunno if that's too much of a hack, or if CrateTableModel could use a clearView method, or if we just call BaseTrackSetFeature::activate() to show the root view when Archived is selected.

ywwg · 2023-05-23T16:36:50Z

yeah I can clear the view. I'll try the different options you suggest

treeitems can either be crates or have children, not both.

ronso0 · 2023-05-25T11:51:50Z

Actually I wanted something like archiving for playlists (don't use crates that much anymore).
I think, now that you did the work for crates I can easily adopt if for playlists.
However, that would require another schema upgrade. I'm not much into that, do we want to keep schema upgrades to a minimum or is this irrelevant?
In case a minimum of upgrades is preferred, could you add the archived column to the playlist table as well in this PR?

ronso0 · 2023-05-25T11:53:22Z

Btw CrateStorageTest.persistentLifecycle is failing on all OS except macOS arm64, and it passes locally here 🤷‍♂️

ronso0 · 2023-05-27T21:26:20Z

#11208 was merged, so now the sidebar selection should remain unchanged if the right-clicked item wasn't also selected. This is mostly done in slotCrateTableChanged.

With this branch however, if I

have Tracks selected
expand Crates > right-click any crate > click Archive
= crate above the now archived crate is selected

I think the reselection part can be simplified if we use a dedicated crate archive method in TrackCollection, as well as a new archiveChanged signal. Connect that to an equivalent of slotCrateTableChanged to takes care of reselect if required.
Coding that proposal was faster than writing it down as review, so here you go ywwg#12

daschuer

Thank you. I have left some comments.

daschuer · 2023-07-19T07:15:08Z

res/schema.xml

+    <description>
+      Support for crates being Archived.
+    </description>
+    <sql>


This uses an integer column for a bool value.

Does it make sense to make the use of this column extensible? Something like "type" or even "parent". We did the same "mistake" with the hidden column for playlists.

daschuer · 2023-07-19T16:29:50Z

src/library/trackset/crate/cratefeature.cpp

+        if (archiving) {
+            const auto index = indexFromCrateId(crate.getId());
+            // Constrain reselection between first and last valid crate positions.
+            reselectRow = std::min(std::max(index.row(), 0), m_pSidebarModel->rowCount() - 3);


Why is this required? Why -3? Can you make it a constant?

daschuer · 2023-07-19T16:35:53Z

src/library/trackset/crate/cratefeature.cpp

+        }
+        crate.setArchived(archiving);
+        if (!m_pTrackCollection->updateCrate(crate)) {
+            qDebug() << "Failed to toggle archive status of crate" << crate;


What is the scenario for this?
Can you give more details to also desringush this message from the one below?

daschuer · 2023-07-19T16:49:18Z

src/library/trackset/crate/cratefeature.cpp

-                        CrateId(pTreeItem->getData()));
-        pTreeItem->setBold(crateContainsSelectedTrack);
+        if (pTreeItem->hasChildren()) {
+            for (auto subItem : pTreeItem->children()) {


daschuer · 2023-07-19T16:51:25Z

src/library/trackset/crate/cratestorage.cpp

@@ -607,12 +617,13 @@ bool CrateStorage::onUpdatingCrate(
    FwdSqlQuery query(m_database,
            QString(
                    "UPDATE %1 "
-                    "SET %2=:name,%3=:locked,%4=:autoDjSource "
-                    "WHERE %5=:id")
+                    "SET %2=:name,%3=:locked,%4=:archived,%5=:autoDjSource "


This can become a QStringLiteral

daschuer · 2023-07-19T16:53:09Z

src/library/trackset/crate/cratestorage.cpp

@@ -14,6 +14,8 @@ const mixxx::Logger kLogger("CrateStorage");

 const QString CRATETABLE_LOCKED = "locked";


This can become a QStringLiteral

ywwg · 2024-02-18T18:22:38Z

I'll be picking this back up and addressing comments

daschuer · 2024-05-08T05:54:25Z

This has unfortunately build errors. I will remove it from the 2.5-beta for now.

ywwg · 2024-05-08T13:51:52Z

no problem. Hopefully I will find time for this in the future

ywwg · 2024-05-09T20:24:33Z

this seems to be a desired feature so I'm going to try to pick it back up and also add the same thing for playlists. One thing I am tempted by is instead of using a bool, use a string, and call it "Archive". Then in the future this could become a more generic feature of named folders (one layer deep, no subfolders allowed!) or perhaps a csv tag system, where one crate could have multiple tags. It's really just a matter of UX decisions.

ywwg added 4 commits October 22, 2022 12:06

Initial work to add Archived property to crates

661f043

formatting

59ca189

Crate Archive: finish basic implementation

a4e3d03

note to self

ec60c70

github-actions bot added the library label Oct 24, 2022

ywwg added 2 commits October 24, 2022 11:49

Merge remote-tracking branch 'origin/main' into crate-archives

2ab2388

revert qml changes

70f3d87

ywwg commented Oct 24, 2022

View reviewed changes

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 23, 2023

ywwg added 2 commits May 20, 2023 14:43

Merge remote-tracking branch 'origin/main' into crate-archives

b26b384

When archiving, select the next crate down rather than following the …

a6e13fd

…crate into the archive folder Also, make sure to highlight archived crates when selecting tracks.

ywwg marked this pull request as ready for review May 20, 2023 19:35

ywwg removed the stale Stale issues that haven't been updated for a long time. label May 20, 2023

ronso0 reviewed May 20, 2023

View reviewed changes

src/library/trackset/crate/cratefeature.cpp Outdated Show resolved Hide resolved

archive crates: only create index if we need it

fd3b63b

Co-authored-by: ronso0 <[email protected]>

ywwg requested a review from ronso0 May 21, 2023 18:49

Merge remote-tracking branch 'origin/main' into crate-archives

7778f64

daschuer requested changes May 22, 2023

View reviewed changes

Crate Archives: missing query format param

fb6d072

ywwg added 2 commits May 23, 2023 12:35

Merge remote-tracking branch 'origin/main' into crate-archives

1d148ab

Crate Archives: don't need to assert on invalid crate

c08cb30

Crate Archives: don't try to get crateid of Archive treeitem

354d684

treeitems can either be crates or have children, not both.

JoergAtGithub requested a review from daschuer July 15, 2023 12:30

JoergAtGithub added the needs review label Jul 15, 2023

daschuer requested changes Jul 19, 2023

View reviewed changes

daschuer removed the needs review label Jul 20, 2023

ywwg added 2 commits November 16, 2023 18:55

Merge remote-tracking branch 'origin/main' into crate-archives

72c984c

crate archives: fix build

8ef183e

ronso0 mentioned this pull request Jan 16, 2024

Past Performance Hidden Crate/Playlist #10707

Open

ywwg added this to the 2.5-beta milestone Feb 18, 2024

daschuer removed this from the 2.5-beta milestone May 8, 2024

Merge remote-tracking branch 'origin/main' into crate-archives

7c4bd2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crate archives #10997

Crate archives #10997

ywwg commented Oct 24, 2022

ywwg Oct 24, 2022

ronso0 May 27, 2023

github-actions bot commented Jan 23, 2023

daschuer left a comment

daschuer May 22, 2023

ywwg May 22, 2023

ronso0 commented May 22, 2023 •

edited

Loading

ywwg commented May 23, 2023

ronso0 commented May 25, 2023

ronso0 commented May 25, 2023

ronso0 commented May 27, 2023

daschuer left a comment

daschuer Jul 19, 2023

daschuer Jul 19, 2023

daschuer Jul 19, 2023

daschuer Jul 19, 2023

daschuer Jul 19, 2023

daschuer Jul 19, 2023

ywwg commented Feb 18, 2024

daschuer commented May 8, 2024

ywwg commented May 8, 2024

ywwg commented May 9, 2024

		@@ -14,6 +14,8 @@ const mixxx::Logger kLogger("CrateStorage");

		const QString CRATETABLE_LOCKED = "locked";

Crate archives #10997

Are you sure you want to change the base?

Crate archives #10997

Conversation

ywwg commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2023

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented May 22, 2023 • edited Loading

ywwg commented May 23, 2023

ronso0 commented May 25, 2023

ronso0 commented May 25, 2023

ronso0 commented May 27, 2023

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Feb 18, 2024

daschuer commented May 8, 2024

ywwg commented May 8, 2024

ywwg commented May 9, 2024

ronso0 commented May 22, 2023 •

edited

Loading