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

Crate archives #10997

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions res/schema.xml
Original file line number Diff line number Diff line change
Expand Up @@ -584,4 +584,12 @@ reapplying those migrations.
UPDATE library SET filetype='aiff' WHERE filetype='aif';
</sql>
</revision>
<revision version="40" min_compatible="3">
<description>
Support for crates being Archived.
</description>
<sql>
Copy link
Member

Choose a reason for hiding this comment

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

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.

ALTER TABLE crates ADD COLUMN archived INTEGER DEFAULT 0;
</sql>
</revision>
</schema>
2 changes: 1 addition & 1 deletion src/database/mixxxdb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
const QString MixxxDb::kDefaultSchemaFile(":/schema.xml");

//static
const int MixxxDb::kRequiredSchemaVersion = 39;
const int MixxxDb::kRequiredSchemaVersion = 40;

namespace {

Expand Down
9 changes: 9 additions & 0 deletions src/library/trackset/crate/crate.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class Crate : public DbNamedEntity<CrateId> {
explicit Crate(CrateId id = CrateId())
: DbNamedEntity(id),
m_locked(false),
m_archived(false),
m_autoDjSource(false) {
}
~Crate() override = default;
Expand All @@ -19,6 +20,13 @@ class Crate : public DbNamedEntity<CrateId> {
m_locked = locked;
}

bool isArchived() const {
return m_archived;
}
void setArchived(bool archived = true) {
m_archived = archived;
}

bool isAutoDjSource() const {
return m_autoDjSource;
}
Expand All @@ -28,5 +36,6 @@ class Crate : public DbNamedEntity<CrateId> {

private:
bool m_locked;
bool m_archived;
bool m_autoDjSource;
};
94 changes: 85 additions & 9 deletions src/library/trackset/crate/cratefeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ void CrateFeature::initActions() {
&QAction::triggered,
this,
&CrateFeature::slotToggleCrateLock);
m_pArchiveCrateAction = make_parented<QAction>(tr("Archive"), this);
connect(m_pArchiveCrateAction.get(),
&QAction::triggered,
this,
&CrateFeature::slotToggleCrateArchived);

m_pAutoDjTrackSourceAction = make_parented<QAction>(tr("Auto DJ Track Source"), this);
m_pAutoDjTrackSourceAction->setCheckable(true);
Expand Down Expand Up @@ -207,9 +212,9 @@ QString CrateFeature::formatRootViewHtml() const {
html.append(QStringLiteral("<p>%1</p>").arg(cratesSummary));
html.append(QStringLiteral("<p>%1</p>").arg(cratesSummary2));
html.append(QStringLiteral("<p>%1</p>").arg(cratesSummary3));
//Colorize links in lighter blue, instead of QT default dark blue.
//Links are still different from regular text, but readable on dark/light backgrounds.
//https://github.com/mixxxdj/mixxx/issues/9103
// Colorize links in lighter blue, instead of QT default dark blue.
// Links are still different from regular text, but readable on dark/light backgrounds.
// https://github.com/mixxxdj/mixxx/issues/9103
html.append(
QStringLiteral("<a style=\"color:#0496FF;\" href=\"create\">%1</a>")
.arg(createCrateLink));
Expand Down Expand Up @@ -392,6 +397,7 @@ void CrateFeature::onRightClickChild(
m_pAutoDjTrackSourceAction->setChecked(crate.isAutoDjSource());

m_pLockCrateAction->setText(crate.isLocked() ? tr("Unlock") : tr("Lock"));
m_pArchiveCrateAction->setText(crate.isArchived() ? tr("Unarchive") : tr("Archive"));

QMenu menu(m_pSidebarWidget);
menu.addAction(m_pCreateCrateAction.get());
Expand All @@ -400,6 +406,7 @@ void CrateFeature::onRightClickChild(
menu.addAction(m_pDuplicateCrateAction.get());
menu.addAction(m_pDeleteCrateAction.get());
menu.addAction(m_pLockCrateAction.get());
menu.addAction(m_pArchiveCrateAction.get());
menu.addSeparator();
menu.addAction(m_pAutoDjTrackSourceAction.get());
menu.addSeparator();
Expand Down Expand Up @@ -538,6 +545,33 @@ void CrateFeature::slotToggleCrateLock() {
}
}

void CrateFeature::slotToggleCrateArchived() {
Crate crate;
if (readLastRightClickedCrate(&crate)) {
bool archiving = !crate.isArchived();
// If we are archiving the crate, select the crate below rather than following
// the crate into the archive subtree.
int reselectRow = 0;
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);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

} else if (archiving) {
const auto reselectIndex = m_pSidebarModel->index(reselectRow, 0);
const auto crateId = crateIdFromIndex(reselectIndex);
if (crateId.isValid()) {
activateCrate(crateId);
}
}
} else {
qDebug() << "Failed to toggle archive status of selected crate";
}
}

void CrateFeature::slotAutoDjTrackSourceChanged() {
Crate crate;
if (readLastRightClickedCrate(&crate)) {
Expand All @@ -560,28 +594,48 @@ QModelIndex CrateFeature::rebuildChildModel(CrateId selectedCrateId) {
m_pSidebarModel->removeRows(0, pRootItem->childRows());

std::vector<std::unique_ptr<TreeItem>> modelRows;
modelRows.reserve(m_pTrackCollection->crates().countCrates());
modelRows.reserve(m_pTrackCollection->crates().countCrates(true) + 1);

auto archiveItem = std::make_unique<TreeItem>(tr("Archived"), -1);
bool selectedIsArchived = false;

int selectedRow = -1;
CrateSummarySelectResult crateSummaries(
m_pTrackCollection->crates().selectCrateSummaries());
CrateSummary crateSummary;
while (crateSummaries.populateNext(&crateSummary)) {
modelRows.push_back(newTreeItemForCrateSummary(crateSummary));
if (crateSummary.isArchived()) {
archiveItem->appendChild(formatLabel(crateSummary), crateSummary.getId().toVariant());
} else {
modelRows.push_back(newTreeItemForCrateSummary(crateSummary));
}
if (selectedCrateId == crateSummary.getId()) {
// save index for selection
selectedRow = static_cast<int>(modelRows.size()) - 1;
if (crateSummary.isArchived()) {
selectedIsArchived = true;
selectedRow = archiveItem->childRows() - 1;
} else {
selectedRow = static_cast<int>(modelRows.size()) - 1;
}
}
}

modelRows.push_back(std::move(archiveItem));
// Save the size of the model now because it gets cleared when the items are inserted
// into the model.
const int rowCount = modelRows.size();
// Append all the newly created TreeItems in a dynamic way to the childmodel
m_pSidebarModel->insertTreeItemRows(std::move(modelRows), 0);

// Update rendering of crates depending on the currently selected track
slotTrackSelected(m_selectedTrackId);

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

But why expand the Archive item?

} else {
return m_pSidebarModel->index(selectedRow, 0);
}
} else {
return QModelIndex();
}
Expand Down Expand Up @@ -634,6 +688,16 @@ QModelIndex CrateFeature::indexFromCrateId(CrateId crateId) const {
(CrateId(pTreeItem->getData()) == crateId)) {
return index;
}
// Assume single sublevel of tree.
if (pTreeItem->hasChildren()) {
for (int childRow = 0; childRow < pTreeItem->childRows(); childRow++) {
TreeItem* pChild = pTreeItem->child(childRow);
if (!pChild->hasChildren() && CrateId(pChild->getData()) == crateId) {
QModelIndex childIndex = m_pSidebarModel->index(childRow, 0, index);
return childIndex;
}
}
}
}
qDebug() << "Tree item for crate not found:" << crateId;
return QModelIndex();
Expand Down Expand Up @@ -931,6 +995,18 @@ void CrateFeature::slotTrackSelected(TrackId trackId) {
sortedTrackCrates.end(),
CrateId(pTreeItem->getData()));
pTreeItem->setBold(crateContainsSelectedTrack);
if (pTreeItem->hasChildren()) {
for (auto subItem : pTreeItem->children()) {
Copy link
Member

Choose a reason for hiding this comment

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

pSubItem?

DEBUG_ASSERT(subItem != nullptr);
bool crateContainsSelectedTrack =
m_selectedTrackId.isValid() &&
std::binary_search(
sortedTrackCrates.begin(),
sortedTrackCrates.end(),
CrateId(subItem->getData()));
subItem->setBold(crateContainsSelectedTrack);
}
}
}

m_pSidebarModel->triggerRepaint();
Expand Down
2 changes: 2 additions & 0 deletions src/library/trackset/crate/cratefeature.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class CrateFeature : public BaseTrackSetFeature {
void slotDuplicateCrate();
void slotAutoDjTrackSourceChanged();
void slotToggleCrateLock();
void slotToggleCrateArchived();
void slotImportPlaylist();
void slotImportPlaylistFile(const QString& playlistFile, CrateId crateId);
void slotCreateImportCrate();
Expand Down Expand Up @@ -119,6 +120,7 @@ class CrateFeature : public BaseTrackSetFeature {
parented_ptr<QAction> m_pDeleteCrateAction;
parented_ptr<QAction> m_pRenameCrateAction;
parented_ptr<QAction> m_pLockCrateAction;
parented_ptr<QAction> m_pArchiveCrateAction;
parented_ptr<QAction> m_pDuplicateCrateAction;
parented_ptr<QAction> m_pAutoDjTrackSourceAction;
parented_ptr<QAction> m_pImportPlaylistAction;
Expand Down
24 changes: 18 additions & 6 deletions src/library/trackset/crate/cratestorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ const mixxx::Logger kLogger("CrateStorage");

const QString CRATETABLE_LOCKED = "locked";
Copy link
Member

Choose a reason for hiding this comment

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

This can become a QStringLiteral


const QString CRATETABLE_ARCHIVED = "archived";

const QString CRATE_SUMMARY_VIEW = "crate_summary";

const QString CRATESUMMARY_TRACK_COUNT = "track_count";
Expand Down Expand Up @@ -67,6 +69,9 @@ class CrateQueryBinder final {
void bindLocked(const QString& placeholder, const Crate& crate) const {
m_query.bindValue(placeholder, crate.isLocked());
}
void bindArchived(const QString& placeholder, const Crate& crate) const {
m_query.bindValue(placeholder, crate.isArchived());
}
void bindAutoDjSource(const QString& placeholder, const Crate& crate) const {
m_query.bindValue(placeholder, crate.isAutoDjSource());
}
Expand Down Expand Up @@ -101,6 +106,7 @@ CrateQueryFields::CrateQueryFields(const FwdSqlQuery& query)
: m_iId(query.fieldIndex(CRATETABLE_ID)),
m_iName(query.fieldIndex(CRATETABLE_NAME)),
m_iLocked(query.fieldIndex(CRATETABLE_LOCKED)),
m_iArchived(query.fieldIndex(CRATETABLE_ARCHIVED)),
m_iAutoDjSource(query.fieldIndex(CRATETABLE_AUTODJ_SOURCE)) {
}

Expand All @@ -110,6 +116,7 @@ void CrateQueryFields::populateFromQuery(
pCrate->setId(getId(query));
pCrate->setName(getName(query));
pCrate->setLocked(isLocked(query));
pCrate->setArchived(isArchived(query));
pCrate->setAutoDjSource(isAutoDjSource(query));
}

Expand Down Expand Up @@ -237,9 +244,10 @@ void CrateStorage::createViews() {
}
}

uint CrateStorage::countCrates() const {
uint CrateStorage::countCrates(bool excludeArchived) const {
const QString whereClause = excludeArchived ? "WHERE archived != 0" : "";
FwdSqlQuery query(m_database,
QStringLiteral("SELECT COUNT(*) FROM %1").arg(CRATE_TABLE));
QStringLiteral("SELECT COUNT(*) FROM %1 %2").arg(CRATE_TABLE, whereClause));
if (query.execPrepared() && query.next()) {
uint result = query.fieldValue(0).toUInt();
DEBUG_ASSERT(!query.next());
Expand Down Expand Up @@ -482,7 +490,7 @@ CrateSummarySelectResult CrateStorage::selectCratesWithTrackCount(
QStringLiteral("SELECT *, "
"(SELECT COUNT(*) FROM %1 WHERE %2.%3 = %1.%4 and "
"%1.%5 in (%9)) AS %6, "
"0 as %7 FROM %2 ORDER BY %8")
"0 as %7 FROM %2 WHERE archived=0 ORDER BY %8")
.arg(
CRATE_TRACKS_TABLE,
CRATE_TABLE,
Expand Down Expand Up @@ -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)")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

.arg(
CRATE_TABLE,
CRATETABLE_NAME,
CRATETABLE_LOCKED,
CRATETABLE_ARCHIVED,
CRATETABLE_AUTODJ_SOURCE));
VERIFY_OR_DEBUG_ASSERT(query.isPrepared()) {
return false;
}
CrateQueryBinder queryBinder(query);
queryBinder.bindName(":name", crate);
queryBinder.bindLocked(":locked", crate);
queryBinder.bindArchived(":archived", crate);
queryBinder.bindAutoDjSource(":autoDjSource", crate);
VERIFY_OR_DEBUG_ASSERT(query.execPrepared()) {
return false;
Expand All @@ -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 "
Copy link
Member

Choose a reason for hiding this comment

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

This can become a QStringLiteral

"WHERE %6=:id")
.arg(
CRATE_TABLE,
CRATETABLE_NAME,
CRATETABLE_LOCKED,
CRATETABLE_ARCHIVED,
CRATETABLE_AUTODJ_SOURCE,
CRATETABLE_ID));
VERIFY_OR_DEBUG_ASSERT(query.isPrepared()) {
Expand All @@ -622,6 +633,7 @@ bool CrateStorage::onUpdatingCrate(
queryBinder.bindId(":id", crate);
queryBinder.bindName(":name", crate);
queryBinder.bindLocked(":locked", crate);
queryBinder.bindArchived(":archived", crate);
queryBinder.bindAutoDjSource(":autoDjSource", crate);
VERIFY_OR_DEBUG_ASSERT(query.execPrepared()) {
return false;
Expand Down
7 changes: 6 additions & 1 deletion src/library/trackset/crate/cratestorage.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ class CrateQueryFields {
bool isLocked(const FwdSqlQuery& query) const {
return query.fieldValueBoolean(m_iLocked);
}
bool isArchived(const FwdSqlQuery& query) const {
return query.fieldValueBoolean(m_iArchived);
}
bool isAutoDjSource(const FwdSqlQuery& query) const {
return query.fieldValueBoolean(m_iAutoDjSource);
}
Expand All @@ -38,6 +41,7 @@ class CrateQueryFields {
DbFieldIndex m_iId;
DbFieldIndex m_iName;
DbFieldIndex m_iLocked;
DbFieldIndex m_iArchived;
DbFieldIndex m_iAutoDjSource;
};

Expand Down Expand Up @@ -264,7 +268,7 @@ class CrateStorage : public virtual /*implements*/ SqlStorage {
// Crate read operations (read-only, const)
/////////////////////////////////////////////////////////////////////////

uint countCrates() const;
uint countCrates(bool excludeArchived = true) const;

// Omit the pCrate parameter for checking if the corresponding crate exists.
bool readCrateById(
Expand Down Expand Up @@ -310,6 +314,7 @@ class CrateStorage : public virtual /*implements*/ SqlStorage {
CrateId crateId) const;
CrateTrackSelectResult selectTrackCratesSorted(
TrackId trackId) const;
// Returns crates for the given trackids that are not archived.
CrateSummarySelectResult selectCratesWithTrackCount(
const QList<TrackId>& trackIds) const;
CrateTrackSelectResult selectTracksSortedByCrateNameLike(
Expand Down