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

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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;
};
112 changes: 95 additions & 17 deletions src/library/trackset/crate/cratefeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@
&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 @@ -204,9 +209,9 @@
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 @@ -300,7 +305,8 @@
void CrateFeature::activateChild(const QModelIndex& index) {
qDebug() << " CrateFeature::activateChild()" << index;
CrateId crateId(crateIdFromIndex(index));
VERIFY_OR_DEBUG_ASSERT(crateId.isValid()) {
if (!crateId.isValid()) {
// happens when clicking the 'Archived' item
return;
}
m_lastClickedIndex = index;
Expand Down Expand Up @@ -389,6 +395,7 @@
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 @@ -397,6 +404,7 @@
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 @@ -535,6 +543,33 @@
}
}

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 @@ -557,28 +592,48 @@
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();

Check failure on line 622 in src/library/trackset/crate/cratefeature.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

the following warning is treated as an error

Check warning on line 622 in src/library/trackset/crate/cratefeature.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

'initializing': conversion from 'size_t' to 'int', possible loss of data

Check warning on line 622 in src/library/trackset/crate/cratefeature.cpp

View workflow job for this annotation

GitHub Actions / Windows 2019 (MSVC)

'initializing': conversion from 'size_t' to 'const int', possible loss of data
// 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 @@ -631,6 +686,16 @@
(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 @@ -923,13 +988,26 @@
// clear all the bolding).
for (TreeItem* pTreeItem : pRootItem->children()) {
DEBUG_ASSERT(pTreeItem != nullptr);
bool crateContainsSelectedTrack =
m_selectedTrackId.isValid() &&
std::binary_search(
sortedTrackCrates.begin(),
sortedTrackCrates.end(),
CrateId(pTreeItem->getData()));
pTreeItem->setBold(crateContainsSelectedTrack);
if (pTreeItem->hasChildren()) {
for (auto subItem : pTreeItem->children()) {

Check warning on line 992 in src/library/trackset/crate/cratefeature.cpp

View workflow job for this annotation

GitHub Actions / clang-tidy

'auto subItem' can be declared as 'auto *subItem' [readability-qualified-auto]
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);
}
} else {
bool crateContainsSelectedTrack =
m_selectedTrackId.isValid() &&
std::binary_search(
sortedTrackCrates.begin(),
sortedTrackCrates.end(),
CrateId(pTreeItem->getData()));
pTreeItem->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
26 changes: 19 additions & 7 deletions src/library/trackset/crate/cratestorage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,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 @@ -69,6 +71,9 @@ class CrateQueryBinder final {
void bindLocked(const QString& placeholder, const Crate& crate) const {
m_query.bindValue(placeholder, QVariant(crate.isLocked()));
}
void bindArchived(const QString& placeholder, const Crate& crate) const {
m_query.bindValue(placeholder, QVariant(crate.isArchived()));
}
void bindAutoDjSource(const QString& placeholder, const Crate& crate) const {
m_query.bindValue(placeholder, QVariant(crate.isAutoDjSource()));
}
Expand Down Expand Up @@ -103,6 +108,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 @@ -112,6 +118,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 @@ -239,9 +246,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 @@ -484,7 +492,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 @@ -570,19 +578,21 @@ bool CrateStorage::onInsertingCrate(
}
FwdSqlQuery query(m_database,
QStringLiteral(
"INSERT INTO %1 (%2,%3,%4) "
"VALUES (:name,:locked,:autoDjSource)")
"INSERT INTO %1 (%2,%3,%4,%5) "
"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 @@ -609,12 +619,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 @@ -624,6 +635,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 @@ -28,6 +28,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 @@ -40,6 +43,7 @@ class CrateQueryFields {
DbFieldIndex m_iId;
DbFieldIndex m_iName;
DbFieldIndex m_iLocked;
DbFieldIndex m_iArchived;
DbFieldIndex m_iAutoDjSource;
};

Expand Down Expand Up @@ -266,7 +270,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 @@ -312,6 +316,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
Loading