-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Only child creation and leaf deletion avaliable so far
This is looking way too ugly, due to the previous commits from the redesign. The new library is pretty functional as it is, so it seems only logical (since it's such a big PR) to have it's own branch in upstream |
// 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 comment
The 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.
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.
Just did a quick test and I confirm it works as you describe. I haven't looked at the code in detail yet.
Ignore the first entry on the hierarchy marked "recursion" it's for a later use.
What will that be used for?
I see a lot of assertions in my log but I don't know how to reproduce this:
|
There is an issue with the crate filter. When I use any other filter in the main Tracks feature, then switch to another feature and switch back, the filter last used on the Tracks feature is restored. If the last filter was a crate filter, the tracks shown when switching back to the Tracks feature are the tracks that would be shown with that filter, but the text of the filter is not restored to the search bar. |
protected: | ||
|
||
private: | ||
bool findParentChildFromPath(Crate& parent, |
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.
This function could have a better name. At first glance it seems like it is finding a "ParentChild" from the given path and the bool return type doesn't make sense with that idea. How about "findParentAndChildFromPath"? It's a long name, but it is clear what the function does. There should be a comment here in the header file explaining what the returned bool indicates.
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.
Wait, why is this a member of CrateTreeModel? Wouldn't it make more sense as a member of CrateStorage or CrateHierarchy?
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.
This looks like it's off to a pretty good start. I haven't looked closely at the SQL or the modifications to the database structure yet, so I have no comment on that (yet). I think we should come up with a better organization for the relationships between the classes. Currently lots of implementation details about handling the database are leaked into the GUI code. I don't think CrateFeature should be aware of CrateHierarchy's existence. I think CrateStorage should take care of all that behind the scenes. Combining CrateStorage and CrateHierarchy would create a pretty large and unwieldy class. How about instantiating CrateHierarchy as a private member of CrateStorage and modifying CrateStorage to make calls to CrateHierarchy where appropriate? That way CrateFeature does not need to make calls to CrateStorage then to CrateHierarchy. I think this refactoring should be taken care of before continuing to expand the current functionality. If this is not taken care of now, it will be more work later to disentangle the GUI from the DB logic.
CrateTreeModel::CrateTreeModel(LibraryFeature* pFeature, | ||
TrackCollection* pTrackCollection, | ||
CrateHierarchy* pCrateHierarchy) | ||
:m_pFeature(pFeature), |
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.
nitpick: space between : and m_pFeature
CrateHierarchy* pCrateHierarchy); | ||
//virtual ~CrateTreeModel(); | ||
|
||
// QVariant data(const QModelIndex& index, int role) const override; |
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.
If the data and setData functions from the TreeItemModel base class work for your purposes, you do not need to reimplement them. If at some point you realize you need to modify those functions specifically for CrateTreeModel in a way that wouldn't be applicable to other TreeItemModels, then reimplement them for CrateTreeModel.
public slots: | ||
void reloadTree() override; | ||
|
||
protected: |
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.
There won't be any child classes of CrateTreeModel, will there? If there won't be, then you can get rid of this line.
TrackCollection* m_pTrackCollection; | ||
CrateHierarchy* m_pCrateHierarchy; | ||
|
||
parented_ptr<TreeItem> m_pRecursion; |
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.
How about renaming this m_pRecursionButton or m_pRecursionItem?
if (!findParentChildFromPath(parent, child, idPath)) { | ||
parent.setId(rootId); | ||
} | ||
if (child.getName() == "") { |
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.
Use QString::isEmpty
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.
Actually, this shouldn't require a string operation. Would child.getId().isValid() work?
namePath = namePath + "/" + crate.getName(); | ||
idPath = idPath + "/" + crate.getId().toString(); | ||
|
||
if(!writeCratePaths(crate.getId(), namePath, idPath)) { |
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.
This can be simplified to return writeCratePaths(crate.getId(), namePath, idPath);
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, you are right, I'll do that
m_pTrackCollection(pTrackCollection){ | ||
} | ||
|
||
uint countCratesOnClosure() const; |
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.
Would "countCratesInClosure" work a bit better for the name?
if (m_pTrackCollection->deleteCrate(crate.getId())) { | ||
m_crateHierarchy.deleteCrate(crate.getId()); |
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.
The GUI code shouldn't have to be concerned with this. Calling TrackCollection::deleteCrate should take care of it automatically.
@@ -65,6 +87,10 @@ void CrateFeature::initActions() { | |||
connect(m_pCreateCrateAction.get(), SIGNAL(triggered()), | |||
this, SLOT(slotCreateCrate())); | |||
|
|||
m_pCreateChildCrateAction = std::make_unique<QAction>(tr("Create Child Crate Here"),this); |
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.
No need for the word "Here" at the end. "Create Child Crate" is clear.
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.
I'll rename it to "Create Subcrate", I think it's nice.
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 comment
The 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.
Merge the master branch to bring in #1309 |
Eliminate GUIs interaction with the backend stuff
kCrateSummaryViewSelect, | ||
kLibraryTracksJoin, | ||
CRATE_TABLE, | ||
CRATETABLE_ID); |
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.
I think it would be easier to maintain if these SQL string constants were put back into cratestorage.cpp (inside the anonymous namespace). Keep the helper classes in the new files.
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.
But I see them as helper strings, that's why I put them there
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.
I also don't like that these constants are exposed to the whole world in a header file.
return newCrateId; | ||
} | ||
|
||
CrateId CrateFeatureHelper::createEmptySubrate(const Crate& parent) { |
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.
typo: createEmptySubcrate
m_pTrackCollection->crates().initClosureForCrate(newCrateId); | ||
m_pTrackCollection->crates().generateCratePaths(newCrate); | ||
qDebug() << "Created new crate" << newCrate; | ||
} else { |
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.
CrateFeatureHelper::createEmptyCrate only makes top-level crates, right? If so, it should be renamed to clarify that.
@@ -74,6 +74,68 @@ CrateId CrateFeatureHelper::createEmptyCrate() { | |||
if (m_pTrackCollection->insertCrate(newCrate, &newCrateId)) { | |||
DEBUG_ASSERT(newCrateId.isValid()); | |||
newCrate.setId(newCrateId); | |||
m_pTrackCollection->crates().initClosureForCrate(newCrateId); | |||
m_pTrackCollection->crates().generateCratePaths(newCrate); |
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.
Calling TrackCollection::insertCrate above should take care of this. TrackCollection shouldn't have to be concerned with these details either. How about moving this logic to CrateStorage?
CrateFeature does not make use of the returned CrateId from CrateFeatureHelper, so you can change the return type to void.
DEBUG_ASSERT(newCrateId.isValid()); | ||
newCrate.setId(newCrateId); | ||
m_pTrackCollection->crates().initClosureForCrate(newCrateId); | ||
if (m_pTrackCollection->crates().insertIntoClosure(parent.getId(), newCrateId)) { |
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.
Likewise, this logic should be moved into CrateStorage. You could add another parameter to TrackCollection::insertCrate to pass it the parent Crate.
pTrackCollection->crates().resetClosure(); | ||
pTrackCollection->crates().initClosure(); | ||
pTrackCollection->crates().resetPath(); | ||
pTrackCollection->crates().generateAllPaths(); |
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.
A single call to TrackCollection should take care of this, including checking closureIsValid. How about calling it TrackCollection::checkCrateHierarchy? I don't think there should be any mention of "closure" in CrateFeature; that's an implementation detail.
I think we need an additional level of control between TrackCollection and CrateStorage/CrateHierarchy that handles all interactions and workflows with crates. CrateStorage already has too many responsibilities, namely managing both crates and a crate tracks. Task: Create a new crate
Task: Delete existing crate
|
Yeah, I have. I guess I'm asking how likely it is to destroy the DB so that I know whether to avoid doing much meaningful work on my data while using it, if that makes sense. If the risk is low, I don't mind running it a lot more. I guess a better question is, what will happen if I try to re-organise crates, and then try to go back to the jmigual-library-redesign banch? |
@naught101 The way I implemented the nesting functionality in the db leaves the old crates table untouched. So if you add nested crates using this branch and then use another build without them, all your crates will be there without the nesting. I haven't had an issue when switching back and forth from versions. All the existing crates are converted to lvl 1 crates when you first launch mixxx with nested crates for the first time. So changing crates and then going back doesn't affect anything, you just lose the nested functionality. On the other hand, if you create some nested crates and then you go and delete them in jmigual-lib-redesign, the functions that take care of the hierarchy stuff wont get called for deletion and then next time you try to add a nested crate in the same DB you might have a problem. |
I don't like this at all. deleting a crate is something people rarely do outside of testing purposes. makes no sense for me to bind this to any key even with a warning dialog it seems wrong to me.
kind regards
poelzi
…On 10 May 2018 05:48:55 CEST, naught101 ***@***.***> wrote:
> - It would be nice if pressing the delete key when a crate is
highlighted in the tree deleted the crate.
Sounds dangerous. It's easy to bump the delete key. If you do this,
please add a confirmation dialogue.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#1304 (comment)
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Awesome, thanks gramanas, that's a very useful summary. I'll start testing more intensely :) |
I think these things are probably post-merge feature requests, but I'll put them here in case it makes sense to do it beforehand:
|
Ahh.. I think the sorting is going off the crate name, instead of the entire crate path. |
|
|
I believe there is a crate:= filter that doesn't match with recursion but I don't remember if I did that or not. Gonna have to check. The way crates are displayed in the right click menu is realy suboptimal, but I didn't think of anythink that worked better. Maybe this #1656 crate widget could be a solution. As for the whole wrefolding of the tree structure on crate change, I can't really do anything unless we fix the underlying tree representation to support saving and restoring states. You are right about the scroll bar. I didn't touch it at all last summer, this is one of the nitpicks that have to be fixed throught the new library. |
A merge conflict has developed. |
Merge conflict and database upgrade issue fixed in gramanas#3 |
Nested crates updates
do you have an idea why |
Add tables to support nested crates. | ||
</description> | ||
<sql> | ||
CREATE TABLE IF NOT EXISTS crateClosure ( |
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?
namePath TEXT | ||
); | ||
|
||
ALTER TABLE crates RENAME TO old_crates; |
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.
Ping!
|
||
CREATE TABLE IF NOT EXISTS cratePath ( | ||
crateId INTEGER, | ||
idPath TEXT, |
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.
please add a comment like
"This contains a / separated string of crate Ids"
CREATE TABLE IF NOT EXISTS cratePath ( | ||
crateId INTEGER, | ||
idPath TEXT, | ||
namePath TEXT |
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.
"This contains a / separated string of crate names"
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.
By the way: how do you treat names which includes a "/"?
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.
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?
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.
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.
This PR is still open, but the feature is good. I personally would like to have a "House" crate with all sub-genres as sub crates. I could not build the PR with cmake, because the code is old and it was build using a legacy way (which I don't know). If there is not much left to do, it would be great to have this feature in 2.3 stable or 2.3.x |
Unfortunately this branch was built on top of the jmigual-library-redesign branch which still has major performance problems and no one is working on that branch anymore. This was a mistake of project management on our part to build this feature on top of another that still had a lot of work to get to a mergeable state. Instead of this feature, we have decided to take the more flexible approach of user defined faceted tags in #2656. |
Hello,
This is a work in progress for the crate enchantments for GSoC 2017.
It's build on top of #1117
Full story here .
In order to test this do the following:
And then
Create new Crate
to create a level 1 crate.Create Subcrate
for subcrates.Rename
to rename the crate (rules apply)Duplicate
to create a duplicate of this crate and it's tracks at lvl 1 (currently it is not recursive, we'll see)move subtree to
will present you with a list of places whare you can move the crate and all it's children to. (You can't move a crate while leaving it's children there, it doesn't make sense since the contents of a child crate are also the contents of the parent one)Remove
. If the crate has children they will get deleted as well. (after a warning)The first entry in the tree is the recursion switch marked as
Show tracks is subcrates
. It defaults to on.Clicking the tree item toggles it.
Crates are displayed using
CrateTableModel
. A view is generated (one for recursion and one for simple view) for each crate when you click at it depending on the above toggle switch. Everything is in that SQL statement so the view updates live with new tracks even new child crates with new tracks (in the case of recursion)CrateStorage
2.2 update