-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Nested crates #1304
Nested crates #1304
Changes from 11 commits
f3ad479
1fba5bc
3c09995
1635ac3
2720d6c
f3e474f
bc97923
8242c67
596df84
8d187b5
7cf56f3
f9946ec
d137704
1d48fa5
5587e1c
06d9cbe
bf5cd72
9948c55
6fa0f83
add5459
b0ca91d
6a7ac10
9d74122
d380b51
f2fc75a
e7f6781
5f3daab
82c3d45
3bdecdb
774ba58
79ee638
aa4d06f
d0591a9
41d2e81
59af9a9
e8417d5
8996708
14c2cfc
311e22b
23c2c9d
de6cd1b
a73cbc0
47157bb
a26ff55
e2cf708
e4da311
cdb08fc
a5c5adc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -446,4 +446,22 @@ METADATA | |
); | ||
</sql> | ||
</revision> | ||
<revision version="29" min_compatible="3"> | ||
<description> | ||
Add tables to support nested crates. | ||
</description> | ||
<sql> | ||
CREATE TABLE IF NOT EXISTS crateClosure ( | ||
parentId INTEGER, | ||
childId INTEGER, | ||
depth INTEGER | ||
); | ||
|
||
CREATE TABLE IF NOT EXISTS cratePath ( | ||
crateId INTEGER, | ||
idPath TEXT, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add a comment like |
||
namePath TEXT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "This contains a / separated string of crate names" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way: how do you treat names which includes a "/"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it smart to store the namePath in the Database? This is a redundant information which can be wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Storing redundant information for optimizing queries is not an issue as long as you are able to guarantee consistency, either immediately or at least eventually. The method of choice for immediate consistency is encapsulation with hooks that are triggered when needed and everything enclosed in a transaction scope. I remember that the my last review revealed that consistency cannot be guaranteed by the current implementation, because the transaction scoping was missing. Considering the fact that the whole hierarchy is abandoned and flattened when any inconsistencies are detected, this is a no-go for production and needs to be fixed. |
||
); | ||
</sql> | ||
</revision> | ||
</schema> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,13 +45,35 @@ CrateFeature::CrateFeature(UserSettingsPointer pConfig, | |
m_cratesIcon(":/images/library/ic_library_crates.png"), | ||
m_lockedCrateIcon(":/images/library/ic_library_locked.png"), | ||
m_pTrackCollection(pTrackCollection), | ||
m_pCrateTableModel(nullptr) { | ||
m_pCrateTableModel(nullptr), | ||
m_crateHierarchy(pTrackCollection) { | ||
|
||
|
||
initActions(); | ||
|
||
// construct child model | ||
m_childModel.setRootItem(std::make_unique<TreeItem>(this)); | ||
rebuildChildModel(); | ||
// m_childModel.setRootItem(std::make_unique<TreeItem>(this)); | ||
// rebuildChildModel(); | ||
|
||
// if closure does not have the same number of crates as the crates table | ||
// this means that the user just started mixxx with nested crates for the | ||
// first time, so we have to fill the closure table with (self,self,0) | ||
if (!m_crateHierarchy.closureIsValid()) { | ||
QMessageBox::warning( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no need for this dialog box to be shown. Please remove it. |
||
nullptr, | ||
tr("Nested Crates"), | ||
tr("Crates now support hierarchical structure." | ||
"All your crates have been converted to Level 1 crates")); | ||
|
||
m_crateHierarchy.resetClosure(); | ||
m_crateHierarchy.initClosure(); | ||
m_crateHierarchy.resetPath(); | ||
m_crateHierarchy.generateAllPaths(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you anticipate needing these 4 functions in any other context? If not, it may make sense to make them private members of CrateHierarchy and create a public wrapper function (maybe called CrateHierarchy::initializeHierarchy or something like that) around them to call here. I think this exposes too much of the implementation of the database storage to the GUI class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, you are right, I'll do that |
||
} | ||
|
||
m_pChildModel = std::make_unique<CrateTreeModel>(this, | ||
m_pTrackCollection, &m_crateHierarchy); | ||
m_pChildModel->reloadTree(); | ||
|
||
connectLibrary(pLibrary); | ||
connectTrackCollection(); | ||
|
@@ -65,6 +87,10 @@ void CrateFeature::initActions() { | |
connect(m_pCreateCrateAction.get(), SIGNAL(triggered()), | ||
this, SLOT(slotCreateCrate())); | ||
|
||
m_pCreateChildCrateAction = std::make_unique<QAction>(tr("Create Subcrate"),this); | ||
connect(m_pCreateChildCrateAction.get(), SIGNAL(triggered()), | ||
this, SLOT(slotCreateChildCrate())); | ||
|
||
m_pDeleteCrateAction = std::make_unique<QAction>(tr("Remove"),this); | ||
connect(m_pDeleteCrateAction.get(), SIGNAL(triggered()), | ||
this, SLOT(slotDeleteCrate())); | ||
|
@@ -204,7 +230,7 @@ void updateTreeItemForTrackSelection( | |
pTreeItem->setBold(crateContainsSelectedTrack); | ||
} | ||
|
||
} | ||
} // anonymus namespace | ||
|
||
bool CrateFeature::dragMoveAccept(QUrl url) { | ||
return SoundSourceProxy::isUrlSupported(url) || | ||
|
@@ -273,7 +299,7 @@ parented_ptr<QWidget> CrateFeature::createPaneWidget(KeyboardEventFilter* pKeybo | |
} | ||
|
||
QPointer<TreeItemModel> CrateFeature::getChildModel() { | ||
return &m_childModel; | ||
return m_pChildModel.get(); | ||
} | ||
|
||
void CrateFeature::activate() { | ||
|
@@ -380,6 +406,7 @@ void CrateFeature::onRightClickChild(const QPoint& globalPos, const QModelIndex& | |
|
||
QMenu menu(NULL); | ||
menu.addAction(m_pCreateCrateAction.get()); | ||
menu.addAction(m_pCreateChildCrateAction.get()); | ||
menu.addSeparator(); | ||
menu.addAction(m_pRenameCrateAction.get()); | ||
menu.addAction(m_pDuplicateCrateAction.get()); | ||
|
@@ -401,8 +428,29 @@ void CrateFeature::onRightClickChild(const QPoint& globalPos, const QModelIndex& | |
void CrateFeature::slotCreateCrate() { | ||
CrateId crateId = CrateFeatureHelper( | ||
m_pTrackCollection, m_pConfig).createEmptyCrate(); | ||
Crate newCrate; | ||
m_pTrackCollection->crates().readCrateById(crateId, &newCrate); | ||
m_crateHierarchy.initClosureForCrate(crateId); | ||
m_crateHierarchy.generateCratePaths(newCrate); | ||
if (crateId.isValid()) { | ||
activateCrate(crateId); | ||
m_pChildModel->reloadTree(); | ||
} | ||
} | ||
|
||
void CrateFeature::slotCreateChildCrate() { | ||
Crate parent; | ||
if (readLastRightClickedCrate(&parent)) { | ||
CrateId newCrate = CrateFeatureHelper( | ||
m_pTrackCollection, m_pConfig).createEmptyCrate(); | ||
if (newCrate.isValid()) { | ||
m_crateHierarchy.initClosureForCrate(newCrate); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CrateHierarchy should be responsible for this workflow (atomically):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A single function call to CrateStorage should take care of all this DB logic. |
||
if (m_crateHierarchy.insertIntoClosure(parent.getId(), newCrate)) { | ||
Crate child; | ||
m_pTrackCollection->crates().readCrateById(newCrate, &child); | ||
m_crateHierarchy.generateCratePaths(child); | ||
m_pChildModel->reloadTree(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
@@ -413,8 +461,16 @@ void CrateFeature::slotDeleteCrate() { | |
qWarning() << "Refusing to delete locked crate" << crate; | ||
return; | ||
} | ||
// better deletion requeried | ||
if (m_crateHierarchy.hasChildern(crate.getId())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo |
||
qWarning() << "Can't delete " << crate; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please only log a single warning message |
||
qWarning() << "Currently only delete leaf crates"; | ||
return; | ||
} | ||
if (m_pTrackCollection->deleteCrate(crate.getId())) { | ||
m_crateHierarchy.deleteCrate(crate.getId()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deletion of crates must be performed atomically in a single transaction There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe m_crateHierarchy as a special kind of DAO should better be added to TrackCollection (our library controller) instead of some GUI component. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The GUI code shouldn't have to be concerned with this. Calling TrackCollection::deleteCrate should take care of it automatically. |
||
qDebug() << "Deleted crate" << crate; | ||
m_pChildModel->reloadTree(); | ||
return; | ||
} | ||
} | ||
|
@@ -502,11 +558,11 @@ void CrateFeature::slotAutoDjTrackSourceChanged() { | |
QModelIndex CrateFeature::rebuildChildModel(CrateId selectedCrateId) { | ||
qDebug() << "CrateFeature::rebuildChildModel()" << selectedCrateId; | ||
|
||
TreeItem* pRootItem = m_childModel.getRootItem(); | ||
TreeItem* pRootItem = m_pChildModel->getRootItem(); | ||
VERIFY_OR_DEBUG_ASSERT(pRootItem != nullptr) { | ||
return QModelIndex(); | ||
} | ||
m_childModel.removeRows(0, pRootItem->childRows()); | ||
m_pChildModel->removeRows(0, pRootItem->childRows()); | ||
|
||
QList<TreeItem*> modelRows; | ||
modelRows.reserve(m_pTrackCollection->crates().countCrates()); | ||
|
@@ -526,13 +582,13 @@ QModelIndex CrateFeature::rebuildChildModel(CrateId selectedCrateId) { | |
} | ||
|
||
// Append all the newly created TreeItems in a dynamic way to the childmodel | ||
m_childModel.insertTreeItemRows(modelRows, 0); | ||
m_pChildModel->insertTreeItemRows(modelRows, 0); | ||
|
||
// Update rendering of crates depending on the currently selected track | ||
slotTrackSelected(m_pSelectedTrack); | ||
|
||
if (selectedRow >= 0) { | ||
return m_childModel.index(selectedRow, 0); | ||
return m_pChildModel->index(selectedRow, 0); | ||
} else { | ||
return QModelIndex(); | ||
} | ||
|
@@ -549,8 +605,8 @@ void CrateFeature::updateChildModel(const QSet<CrateId>& updatedCrateIds) { | |
VERIFY_OR_DEBUG_ASSERT(crateStorage.readCrateSummaryById(crateId, &crateSummary)) { | ||
continue; | ||
} | ||
updateTreeItemForCrateSummary(m_childModel.getItem(index), crateSummary); | ||
m_childModel.triggerRepaint(index); | ||
updateTreeItemForCrateSummary(m_pChildModel->getItem(index), crateSummary); | ||
m_pChildModel->triggerRepaint(index); | ||
} | ||
if (m_pSelectedTrack) { | ||
// Crates containing the currently selected track might | ||
|
@@ -566,7 +622,7 @@ CrateId CrateFeature::crateIdFromIndex(const QModelIndex& index) const { | |
bool ok = false; | ||
int id = index.data(AbstractRole::RoleData).toInt(&ok); | ||
if (ok) { | ||
return CrateId(id); | ||
return CrateId(id); | ||
} | ||
return CrateId(); | ||
} | ||
|
@@ -575,9 +631,9 @@ QModelIndex CrateFeature::indexFromCrateId(CrateId crateId) const { | |
VERIFY_OR_DEBUG_ASSERT(crateId.isValid()) { | ||
return QModelIndex(); | ||
} | ||
for (int row = 0; row < m_childModel.rowCount(); ++row) { | ||
QModelIndex index = m_childModel.index(row, 0); | ||
TreeItem* pTreeItem = m_childModel.getItem(index); | ||
for (int row = 0; row < m_pChildModel->rowCount(); ++row) { | ||
QModelIndex index = m_pChildModel->index(row, 0); | ||
TreeItem* pTreeItem = m_pChildModel->getItem(index); | ||
DEBUG_ASSERT(pTreeItem != nullptr); | ||
if (!pTreeItem->hasChildren() && // leaf node | ||
(CrateId(pTreeItem->getData()) == crateId)) { | ||
|
@@ -793,17 +849,19 @@ void CrateFeature::slotExportTrackFiles() { | |
} | ||
|
||
void CrateFeature::slotCrateTableChanged(CrateId crateId) { | ||
if (m_lastRightClickedIndex.isValid() && | ||
(crateIdFromIndex(m_lastRightClickedIndex) == crateId)) { | ||
// Preserve crate selection | ||
m_lastRightClickedIndex = rebuildChildModel(crateId); | ||
if (m_lastRightClickedIndex.isValid()) { | ||
activateCrate(crateId); | ||
} | ||
} else { | ||
// Discard crate selection | ||
rebuildChildModel(); | ||
} | ||
Q_UNUSED(crateId) | ||
// if (m_lastRightClickedIndex.isValid() && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't comment out code. Delete it or keep it. Otherwise no one will know how to deal with this code snippets. |
||
// (crateIdFromIndex(m_lastRightClickedIndex) == crateId)) { | ||
// // Preserve crate selection | ||
// m_lastRightClickedIndex = rebuildChildModel(crateId); | ||
// if (m_lastRightClickedIndex.isValid()) { | ||
// activateCrate(crateId); | ||
// } | ||
// } else { | ||
// // Discard crate selection | ||
// rebuildChildModel(); | ||
// } | ||
m_pChildModel->reloadTree(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rebuilding the tree should preserve the current selection. Otherwise I always need to manually navigate through the hierarchy back to the crate that I had selected. |
||
} | ||
|
||
void CrateFeature::slotCrateContentChanged(CrateId crateId) { | ||
|
@@ -827,7 +885,7 @@ void CrateFeature::htmlLinkClicked(const QUrl& link) { | |
void CrateFeature::slotTrackSelected(TrackPointer pTrack) { | ||
m_pSelectedTrack = pTrack; | ||
|
||
TreeItem* pRootItem = m_childModel.getRootItem(); | ||
TreeItem* pRootItem = m_pChildModel->getRootItem(); | ||
VERIFY_OR_DEBUG_ASSERT(pRootItem != nullptr) { | ||
return; | ||
} | ||
|
@@ -849,7 +907,7 @@ void CrateFeature::slotTrackSelected(TrackPointer pTrack) { | |
updateTreeItemForTrackSelection(pTreeItem, selectedTrackId, sortedTrackCrates); | ||
} | ||
|
||
m_childModel.triggerRepaint(); | ||
m_pChildModel->triggerRepaint(); | ||
} | ||
|
||
void CrateFeature::slotResetSelectedTrack() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly specify a primary key id column for each table. Otherwise it will be created implicitly as ROWID. But omit the keyword AUTOINCREMENT:
https://sqlite.org/autoinc.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uklotzde: Why is that an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whith AUTOINCREMENT SQLite needs to keep track of the greatest ID value that has ever been used in a separate table. Without the keyword it can simply leverage the primary key index of the table itself and reuse any currently unused ID value:
https://www.sqlite.org/autoinc.html
Two use cases for AUTOCOMMIT: