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

Nested crates #1304

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
f3ad479
Add cratetreemodel.h
gramanas Jun 7, 2017
1fba5bc
Merge branch 'jmigual-library-redesign' of https://github.com/daschue…
gramanas Jun 15, 2017
3c09995
Nested Crate Insertion with bad code
gramanas Jun 20, 2017
1635ac3
Merge branch 'jmigual-library-redesign' of https://github.com/daschue…
gramanas Jun 20, 2017
2720d6c
Add nested crates
gramanas Jun 21, 2017
f3e474f
Disguise sql columns as variables
gramanas Jul 6, 2017
bc97923
Merge branch 'jmigual-library-redesign' of https://github.com/daschue…
gramanas Jul 6, 2017
8242c67
Add 'Add new crate' that adds a crate to the top level
gramanas Jul 7, 2017
596df84
Merge branch 'jmigual-library-redesign' of https://github.com/mixxxdj…
gramanas Jul 15, 2017
8d187b5
Move hierarchy functions into CrateHierarchy
gramanas Jul 16, 2017
7cf56f3
Split CrateStorage file
gramanas Jul 16, 2017
f9946ec
Moved crateHierarchy to crateStorage
gramanas Jul 16, 2017
d137704
Merge branch 'jmigual-library-redesign' of https://github.com/mixxxdj…
gramanas Jul 18, 2017
1d48fa5
Fix small mistakes and typos
gramanas Jul 19, 2017
5587e1c
Refactor CrateStorage
gramanas Jul 23, 2017
06d9cbe
Select crates usinc the crate filter
gramanas Jul 27, 2017
bf5cd72
First attempt at a recursive crate select via filter
gramanas Jul 27, 2017
9948c55
Duplicate crate, small fixes and typos, privacy at crateHierarchy
gramanas Jul 29, 2017
6fa0f83
Rename now avaliable. Fix typos
gramanas Jul 31, 2017
add5459
insertCrate is now the only thing gui knows when inserting a crate
gramanas Aug 1, 2017
b0ca91d
Add recursive deletion, no filter in crate select anymore
gramanas Aug 4, 2017
6a7ac10
Toggle recursion in crate selection + small fixes
gramanas Aug 4, 2017
9d74122
Fix bug with crateSelect, typos & small fixes
gramanas Aug 5, 2017
d380b51
More small changes
gramanas Aug 5, 2017
f2fc75a
Fix renaming issue and small changes
gramanas Aug 7, 2017
e7f6781
Introduce new more stable crate filter
gramanas Aug 7, 2017
5f3daab
RichtClick > add to crate on a track now shows paths, fix filter bug
gramanas Aug 9, 2017
82c3d45
Add crate summary
gramanas Aug 12, 2017
3bdecdb
small code cleanup
gramanas Aug 15, 2017
774ba58
Change naming conventions to work more like a file system
gramanas Aug 20, 2017
79ee638
Merge branch 'NestedCrates' of https://github.com/gramanas/mixxx into…
gramanas Aug 20, 2017
aa4d06f
Add moving capabilities to crates via right click option
gramanas Aug 22, 2017
d0591a9
Enforce naming conventions
gramanas Aug 22, 2017
41d2e81
Correct crate filter tests
gramanas Aug 23, 2017
59af9a9
Don't move a crate to it's parent
gramanas Aug 23, 2017
e8417d5
Fix bug...
gramanas Oct 11, 2017
8996708
Wrap moveCrate in a transaction
gramanas Oct 12, 2017
14c2cfc
Merge branch 'NestedCrates' of https://github.com/gramanas/mixxx into…
gramanas Oct 12, 2017
311e22b
Merge branch 'jmigual-library-redesign' of https://github.com/mixxxdj…
gramanas Oct 12, 2017
23c2c9d
Fix bugs with move crate and with the merge problem
gramanas Oct 14, 2017
de6cd1b
Merge branch 'jmigual-library-redesign' of https://github.com/mixxxdj…
gramanas Jan 5, 2018
a73cbc0
Merge commit '9df14fa6f3706df6b3563ad2cd8f66ae56ca99db' into NestedCr…
gramanas Apr 20, 2018
47157bb
Merge branch 'jmigual-library-redesign' into NestedCrates
gramanas Apr 20, 2018
a26ff55
Merge branch 'jmigual-library-redesign' into NestedCrates
gramanas May 7, 2018
e2cf708
Merge remote-tracking branch 'upstream/jmigual-library-redesign' into…
Be-ing Jun 23, 2018
e4da311
fix database schema upgrade for nested crates
Be-ing Jun 23, 2018
cdb08fc
show context menu to add crate when right clicking empty area
Be-ing Jun 23, 2018
a5c5adc
Merge pull request #3 from Be-ing/nested-crates
gramanas Jun 24, 2018
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
7 changes: 6 additions & 1 deletion build/depends.py
Original file line number Diff line number Diff line change
Expand Up @@ -1027,9 +1027,14 @@ def sources(self, build):

"library/features/crates/cratefeature.cpp",
"library/features/crates/cratetablemodel.cpp",
"library/features/crates/cratetracks.cpp",
"library/features/crates/cratehierarchy.cpp",
"library/features/crates/cratestoragehelpers.cpp",
"library/features/crates/cratestorage.cpp",
"library/features/crates/cratetreemodel.cpp",
"library/features/crates/cratefeaturehelper.cpp",

"library/features/crates/cratemanager.cpp",

"library/features/history/historyfeature.cpp",
"library/features/history/historytreemodel.cpp",

Expand Down
33 changes: 33 additions & 0 deletions res/schema.xml
Original file line number Diff line number Diff line change
Expand Up @@ -458,4 +458,37 @@ METADATA
);
</sql>
</revision>
<revision version="30" min_compatible="3">
<description>
Add tables to support nested crates.
</description>
<sql>
CREATE TABLE IF NOT EXISTS crateClosure (
Copy link
Contributor

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

Copy link
Member

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?

Copy link
Contributor

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:

  • if you need to store the ID somewhere else outside of the database and you must guarantee that an ID still identifies the same record when coming back later at any time
  • if you are using the ID to enforce a strict ordering of records, i.e. for a persistent FIFO task scheduler

parentId INTEGER,
childId INTEGER,
depth INTEGER
);

CREATE TABLE IF NOT EXISTS cratePath (
crateId INTEGER,
idPath TEXT,
Copy link
Member

Choose a reason for hiding this comment

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

please add a comment like
"This contains a / separated string of crate Ids"

namePath TEXT
Copy link
Member

Choose a reason for hiding this comment

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

"This contains a / separated string of crate names"

Copy link
Member

Choose a reason for hiding this comment

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

By the way: how do you treat names which includes a "/"?

Copy link
Member

Choose a reason for hiding this comment

The 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.
How is guaranteed that this is always correct?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

);

ALTER TABLE crates RENAME TO old_crates;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apparently you can't alter a column's unique status, so I had to create the crates table again this time with the name not unique

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about removing the unique constraint from the name column. The SQL code does not reveal this fact.

Copy link
Member

Choose a reason for hiding this comment

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

Ping!


CREATE TABLE crates (
id integer PRIMARY KEY AUTOINCREMENT,
name varchar(48) NOT NULL,
count integer DEFAULT 0,
show integer DEFAULT 1,
locked integer DEFAULT 0,
autodj_source integer DEFAULT 0
);

INSERT INTO crates SELECT * FROM old_crates;

DROP TABLE old_crates;
</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 @@ -11,7 +11,7 @@
const QString MixxxDb::kDefaultSchemaFile(":/schema.xml");

//static
const int MixxxDb::kRequiredSchemaVersion = 28;
const int MixxxDb::kRequiredSchemaVersion = 30;

namespace {

Expand Down
4 changes: 4 additions & 0 deletions src/library/basesqltablemodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,10 @@ void BaseSqlTableModel::setTable(const QString& tableName,
m_bInitialized = true;
}

QString BaseSqlTableModel::getTableName() {
return m_tableName;
}

const QString BaseSqlTableModel::currentSearch() const {
return m_currentSearch;
}
Expand Down
1 change: 1 addition & 0 deletions src/library/basesqltablemodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ class BaseSqlTableModel : public QAbstractTableModel, public TrackModel {
void setTable(const QString& tableName, const QString& trackIdColumn,
const QStringList& tableColumns,
QSharedPointer<BaseTrackCache> trackSource);
QString getTableName();
void initHeaderData();

QSet<TrackId> getTrackIdsFromIndices(const QModelIndexList& list) const;
Expand Down
9 changes: 9 additions & 0 deletions src/library/crate/crateschema.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

#define CRATE_TABLE "crates"
#define CRATE_TRACKS_TABLE "crate_tracks"
#define CRATE_CLOSURE_TABLE "crateClosure"
#define CRATE_PATH_TABLE "cratePath"

const QString CRATETABLE_ID = "id";
const QString CRATETABLE_NAME = "name";
Expand All @@ -24,5 +26,12 @@ const QString CRATETABLE_AUTODJ_SOURCE = "autodj_source";
const QString CRATETRACKSTABLE_CRATEID = "crate_id";
const QString CRATETRACKSTABLE_TRACKID = "track_id";

const QString CLOSURE_PARENTID = "parentId";
const QString CLOSURE_CHILDID = "childId";
const QString CLOSURE_DEPTH = "depth";

const QString PATHTABLE_CRATEID = "crateId";
const QString PATHTABLE_ID_PATH = "idPath";
const QString PATHTABLE_NAME_PATH = "namePath";

#endif // MIXXX_CRATESCHEMA_H
15 changes: 8 additions & 7 deletions src/library/dao/autodjcratesdao.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ AutoDJCratesDAO::AutoDJCratesDAO(
UserSettingsPointer pConfig)
: m_iAutoDjPlaylistId(iAutoDjPlaylistId),
m_pTrackCollection(pTrackCollection),
m_pCrates(pTrackCollection->crates()),
m_database(pTrackCollection->database()),
m_pConfig(pConfig),
// The database has not been created yet.
Expand Down Expand Up @@ -198,13 +199,13 @@ void AutoDJCratesDAO::createAndConnectAutoDjCratesDatabase() {
this, SLOT(slotTrackDirty(TrackId)));

// Be notified when the status of crates changes.
connect(m_pTrackCollection, SIGNAL(crateInserted(CrateId)),
connect(m_pCrates, SIGNAL(crateInserted(CrateId)),
this, SLOT(slotCrateInserted(CrateId)));
connect(m_pTrackCollection, SIGNAL(crateDeleted(CrateId)),
connect(m_pCrates, SIGNAL(crateDeleted(CrateId)),
this, SLOT(slotCrateDeleted(CrateId)));
connect(m_pTrackCollection, SIGNAL(crateUpdated(CrateId)),
connect(m_pCrates, SIGNAL(crateUpdated(CrateId)),
this, SLOT(slotCrateUpdated(CrateId)));
connect(m_pTrackCollection, SIGNAL(crateTracksChanged(CrateId,QList<TrackId>,QList<TrackId>)),
connect(m_pCrates, SIGNAL(crateTracksChanged(CrateId,QList<TrackId>,QList<TrackId>)),
this, SLOT(slotCrateTracksChanged(CrateId,QList<TrackId>,QList<TrackId>)));

// Be notified when playlists are added/removed.
Expand Down Expand Up @@ -720,7 +721,7 @@ void AutoDJCratesDAO::slotTrackDirty(TrackId trackId) {
void AutoDJCratesDAO::slotCrateInserted(CrateId crateId) {
// If this newly-added crate is in the auto-DJ queue, add it to the list.
Crate crate;
if (m_pTrackCollection->crates().readCrateById(crateId, &crate)) {
if (m_pCrates->storage().readCrateById(crateId, &crate)) {
if (crate.isAutoDjSource()) {
updateAutoDjCrate(crateId);
}
Expand All @@ -729,7 +730,7 @@ void AutoDJCratesDAO::slotCrateInserted(CrateId crateId) {

void AutoDJCratesDAO::slotCrateUpdated(CrateId crateId) {
Crate crate;
if (m_pTrackCollection->crates().readCrateById(crateId, &crate)) {
if (m_pCrates->storage().readCrateById(crateId, &crate)) {
if (crate.isAutoDjSource()) {
updateAutoDjCrate(crateId);
} else {
Expand Down Expand Up @@ -875,7 +876,7 @@ void AutoDJCratesDAO::slotCrateTracksChanged(
const QList<TrackId>& removedTrackIds) {
// Skip this if it's not an auto-DJ crate.
Crate crate;
if (!m_pTrackCollection->crates().readCrateById(crateId, &crate)
if (!m_pCrates->storage().readCrateById(crateId, &crate)
|| !crate.isAutoDjSource()) {
return;
}
Expand Down
2 changes: 2 additions & 0 deletions src/library/dao/autodjcratesdao.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "preferences/usersettings.h"
#include "library/crate/crateid.h"
#include "library/features/crates/cratemanager.h"
#include "track/track.h"
#include "util/class.h"

Expand Down Expand Up @@ -103,6 +104,7 @@ class AutoDJCratesDAO : public QObject {
const int m_iAutoDjPlaylistId;

TrackCollection* m_pTrackCollection;
CrateManager* m_pCrates;

// The SQL database we interact with.
QSqlDatabase m_database;
Expand Down
21 changes: 11 additions & 10 deletions src/library/features/autodj/autodjfeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#include "mixer/playermanager.h"
#include "library/trackcollection.h"
#include "library/treeitem.h"
#include "library/features/crates/cratestorage.h"
#include "library/features/crates/cratemanager.h"
#include "controllers/keyboard/keyboardeventfilter.h"
#include "sources/soundsourceproxy.h"
#include "util/dnd.h"
Expand Down Expand Up @@ -52,7 +52,8 @@ AutoDJFeature::AutoDJFeature(UserSettingsPointer pConfig,
m_iAutoDJPlaylistId(findOrCrateAutoDjPlaylistId(m_playlistDao)),
m_pAutoDJProcessor(nullptr),
m_pAutoDJView(nullptr),
m_autoDjCratesDao(m_iAutoDJPlaylistId, pTrackCollection, pConfig) {
m_autoDjCratesDao(m_iAutoDJPlaylistId, pTrackCollection, pConfig),
m_pCrates(pTrackCollection->crates()) {

qRegisterMetaType<AutoDJProcessor::AutoDJState>("AutoDJState");
m_pAutoDJProcessor = new AutoDJProcessor(
Expand All @@ -72,11 +73,11 @@ AutoDJFeature::AutoDJFeature(UserSettingsPointer pConfig,
m_childModel.setRootItem(std::move(pRootItem));

// Be notified when the status of crates changes.
connect(m_pTrackCollection, SIGNAL(crateInserted(CrateId)),
connect(m_pCrates, SIGNAL(crateInserted(CrateId)),
this, SLOT(slotCrateChanged(CrateId)));
connect(m_pTrackCollection, SIGNAL(crateUpdated(CrateId)),
connect(m_pCrates, SIGNAL(crateUpdated(CrateId)),
this, SLOT(slotCrateChanged(CrateId)));
connect(m_pTrackCollection, SIGNAL(crateDeleted(CrateId)),
connect(m_pCrates, SIGNAL(crateDeleted(CrateId)),
this, SLOT(slotCrateChanged(CrateId)));

// Create context-menu items to allow crates to be added to, and removed
Expand Down Expand Up @@ -197,18 +198,18 @@ bool AutoDJFeature::dragMoveAccept(QUrl url) {

// Add a crate to the auto-DJ queue.
void AutoDJFeature::slotAddCrateToAutoDj(int iCrateId) {
m_pTrackCollection->updateAutoDjCrate(CrateId(iCrateId), true);
m_pCrates->updateAutoDjCrate(CrateId(iCrateId), true);
}

void AutoDJFeature::slotRemoveCrateFromAutoDj() {
CrateId crateId(m_pRemoveCrateFromAutoDj->data());
DEBUG_ASSERT(crateId.isValid());
m_pTrackCollection->updateAutoDjCrate(crateId, false);
m_pCrates->updateAutoDjCrate(crateId, false);
}

void AutoDJFeature::slotCrateChanged(CrateId crateId) {
Crate crate;
if (m_pTrackCollection->crates().readCrateById(crateId, &crate) && crate.isAutoDjSource()) {
if (m_pCrates->storage().readCrateById(crateId, &crate) && crate.isAutoDjSource()) {
// Crate exists and is already a source for AutoDJ
// -> Find and update the corresponding child item
for (int i = 0; i < m_crateList.length(); ++i) {
Expand Down Expand Up @@ -287,7 +288,7 @@ void AutoDJFeature::slotAddRandomTrack() {

void AutoDJFeature::constructCrateChildModel() {
m_crateList.clear();
CrateSelectResult autoDjCrates(m_pTrackCollection->crates().selectAutoDjCrates(true));
CrateSelectResult autoDjCrates(m_pCrates->storage().selectAutoDjCrates(true));
Crate crate;
while (autoDjCrates.populateNext(&crate)) {
// Create the TreeItem for this crate.
Expand All @@ -304,7 +305,7 @@ void AutoDJFeature::onRightClickChild(const QPoint& globalPos,
// Bring up the context menu.
QMenu crateMenu;
crateMenu.setTitle(tr("Add Crate as Track Source"));
CrateSelectResult nonAutoDjCrates(m_pTrackCollection->crates().selectAutoDjCrates(false));
CrateSelectResult nonAutoDjCrates(m_pCrates->storage().selectAutoDjCrates(false));
Crate crate;
while (nonAutoDjCrates.populateNext(&crate)) {
auto pAction = std::make_unique<QAction>(crate.getName(), &crateMenu);
Expand Down
2 changes: 2 additions & 0 deletions src/library/features/autodj/autodjfeature.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ class AutoDJFeature : public LibraryFeature {
// How we access the auto-DJ-crates database.
AutoDJCratesDAO m_autoDjCratesDao;

CrateManager* m_pCrates;

// A context-menu item that allows crates to be removed from the
// auto-DJ list.
QAction *m_pRemoveCrateFromAutoDj;
Expand Down
Loading