Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
eb5cf7e
Highlight Crates a Track is in. Still some work to do
May 28, 2015
3832e41
Rename delegate according to his function
May 28, 2015
dccb519
Highlighting crates the selected track is in works now
May 29, 2015
c48d33a
Some minor code-style fixes
May 29, 2015
a096fb6
Added comment to CrateHighlightDelegate::initStyleOption
May 29, 2015
ca3f011
Added highlighting of playlists
May 29, 2015
34ce420
Another aproach for Repainting preventing the selection to move
May 29, 2015
5d09cfa
Ignore QModelIndex's that don't have TreeItem internal pointers in Cr…
rryan May 29, 2015
915c682
Fix SidebarModel handling of child models' dataChanged signals.
rryan May 29, 2015
f2fb6bf
Merge pull request #1 from rryan/sidebar-highlight
MK-42 May 29, 2015
8f27dc3
Holding Track-Crate relation in memory for faster highlighting
May 29, 2015
4dbc7ac
Holding Track-Playlist relation in memory for faster highlighting
May 29, 2015
d9c3db3
Clean up used memory
May 29, 2015
29cd09a
fix missing signals
May 29, 2015
dc1f573
Clear selected Playlists/Crates on viewChange
May 29, 2015
dbb60d8
Remove CrateHighlightDelegate.
rryan May 30, 2015
6552667
Add bold attribute to TreeItem.
rryan May 30, 2015
d6692f0
Restore LibraryFeature::trackSelected signal.
rryan May 30, 2015
89e3ad3
Respond to Qt::FontRole in SidebarModel.
rryan May 30, 2015
986013d
Populate playlist and crate TreeItems with bold attribute when track …
rryan May 30, 2015
939428f
Populate TreeItem state when re-building the child model.
rryan May 30, 2015
4a75b85
Merge pull request #2 from rryan/sidebar-highlight
MK-42 May 31, 2015
dd64f07
Revert "Some fixes."
MK-42 Jun 10, 2015
994300c
Merge pull request #4 from MK-42/revert-2-sidebar-highlight
MK-42 Jun 10, 2015
6cf15ba
Restore LibraryFeature::trackSelected signal.
rryan May 30, 2015
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
1 change: 1 addition & 0 deletions build/depends.py
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ def sources(self, build):
"library/baseplaylistfeature.cpp",
"library/playlistfeature.cpp",
"library/setlogfeature.cpp",
"library/cratehighlightdelegate.cpp",

"library/browse/browsetablemodel.cpp",
"library/browse/browsethread.cpp",
Expand Down
26 changes: 26 additions & 0 deletions src/library/baseplaylistfeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <QFileInfo>
#include <QDesktopServices>

#include "library/library.h"
#include "library/parser.h"
#include "library/parserm3u.h"
#include "library/parserpls.h"
Expand Down Expand Up @@ -82,6 +83,12 @@ BasePlaylistFeature::BasePlaylistFeature(QObject* parent,

connect(&m_playlistDao, SIGNAL(lockChanged(int)),
this, SLOT(slotPlaylistTableChanged(int)));

Library* pLibrary = static_cast<Library*>(parent);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is redundant with library.cpp:229 -- all LibraryFeatures should already get this signal. Did you find that it wasn't being delivered?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whoops, I got that backwards.

connect(pLibrary, SIGNAL(trackSelected(TrackPointer)),
this, SLOT(slotTrackSelected(TrackPointer)));
connect(pLibrary, SIGNAL(switchToView(const QString&)),
this, SLOT(slotResetSelectedTrack()));
}

BasePlaylistFeature::~BasePlaylistFeature() {
Expand Down Expand Up @@ -585,3 +592,22 @@ QModelIndex BasePlaylistFeature::indexFromPlaylistId(int playlistId) {
}
return QModelIndex();
}

bool BasePlaylistFeature::isTrackInChildModel(const int trackId,
const QVariant dataPath) {
return m_playlistDao.isTrackInPlaylist(trackId, dataPath.toInt());
}

TrackPointer BasePlaylistFeature::getSelectedTrack() {
return m_pSelectedTrack;
}

void BasePlaylistFeature::slotTrackSelected(TrackPointer pTrack) {
m_pSelectedTrack = pTrack;
m_childModel.triggerRepaint();
}


void BasePlaylistFeature::slotResetSelectedTrack() {
slotTrackSelected(TrackPointer());
}
9 changes: 9 additions & 0 deletions src/library/baseplaylistfeature.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "library/libraryfeature.h"
#include "library/dao/playlistdao.h"
#include "library/dao/trackdao.h"
#include "trackinfoobject.h"

class WLibrary;
class MixxxKeyboard;
Expand All @@ -34,6 +35,9 @@ class BasePlaylistFeature : public LibraryFeature {
void bindWidget(WLibrary* libraryWidget,
MixxxKeyboard* keyboard);

TrackPointer getSelectedTrack();
virtual bool isTrackInChildModel(const int trackId, const QVariant dataPath);

signals:
void showPage(const QUrl& page);
void analyzeTracks(QList<int>);
Expand Down Expand Up @@ -89,11 +93,16 @@ class BasePlaylistFeature : public LibraryFeature {
QList<QPair<int, QString> > m_playlistList;
QModelIndex m_lastRightClickedIndex;
TreeItemModel m_childModel;
TrackPointer m_pSelectedTrack;

private:
virtual QString getRootViewHtml() const = 0;

QString m_rootViewName;

private slots:
void slotTrackSelected(TrackPointer pTrack);
void slotResetSelectedTrack();
};

#endif /* BASEPLAYLISTFEATURE_H */
27 changes: 25 additions & 2 deletions src/library/cratefeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,13 @@
#include "util/dnd.h"
#include "util/time.h"

CrateFeature::CrateFeature(QObject* parent,
CrateFeature::CrateFeature(Library* pLibrary,
TrackCollection* pTrackCollection,
ConfigObject<ConfigValue>* pConfig)
: m_pTrackCollection(pTrackCollection),
m_crateDao(pTrackCollection->getCrateDAO()),
m_crateTableModel(this, pTrackCollection),
m_pConfig(pConfig) {
Q_UNUSED(parent);
m_pCreateCrateAction = new QAction(tr("Create New Crate"),this);
connect(m_pCreateCrateAction, SIGNAL(triggered()),
this, SLOT(slotCreateCrate()));
Expand Down Expand Up @@ -92,6 +91,11 @@ CrateFeature::CrateFeature(QObject* parent,
TreeItem *rootItem = new TreeItem();
m_childModel.setRootItem(rootItem);
constructChildModel(-1);

connect(pLibrary, SIGNAL(trackSelected(TrackPointer)),
this, SLOT(slotTrackSelected(TrackPointer)));
connect(pLibrary, SIGNAL(switchToView(const QString&)),
this, SLOT(slotResetSelectedTrack()));
}

CrateFeature::~CrateFeature() {
Expand All @@ -116,6 +120,11 @@ QIcon CrateFeature::getIcon() {
return QIcon(":/images/library/ic_library_crates.png");
}

bool CrateFeature::isTrackInChildModel(const int trackId,
const QVariant dataPath) {
return m_crateDao.isTrackInCrate(trackId, dataPath.toInt());
}

int CrateFeature::crateIdFromIndex(QModelIndex index) {
TreeItem* item = static_cast<TreeItem*>(index.internalPointer());
if (item == NULL) {
Expand Down Expand Up @@ -705,3 +714,17 @@ QString CrateFeature::getRootViewHtml() const {
html.append("</td></tr></table>");
return html;
}

TrackPointer CrateFeature::getSelectedTrack() {
return m_pSelectedTrack;
}

void CrateFeature::slotTrackSelected(TrackPointer pTrack) {
m_pSelectedTrack = pTrack;
m_childModel.triggerRepaint();
}

void CrateFeature::slotResetSelectedTrack() {
slotTrackSelected(TrackPointer());
}

12 changes: 11 additions & 1 deletion src/library/cratefeature.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,18 @@

#include "library/libraryfeature.h"
#include "library/cratetablemodel.h"
#include "library/library.h"

#include "treeitemmodel.h"
#include "configobject.h"
#include "trackinfoobject.h"

class TrackCollection;

class CrateFeature : public LibraryFeature {
Q_OBJECT
public:
CrateFeature(QObject* parent,
CrateFeature(Library* pLibrary,
TrackCollection* pTrackCollection,
ConfigObject<ConfigValue>* pConfig);
virtual ~CrateFeature();
Expand All @@ -38,6 +40,9 @@ class CrateFeature : public LibraryFeature {

TreeItemModel* getChildModel();

TrackPointer getSelectedTrack();
virtual bool isTrackInChildModel(const int trackId, const QVariant dataPath);

signals:
void analyzeTracks(QList<int>);

Expand Down Expand Up @@ -85,6 +90,11 @@ class CrateFeature : public LibraryFeature {
QModelIndex m_lastRightClickedIndex;
TreeItemModel m_childModel;
ConfigObject<ConfigValue>* m_pConfig;
TrackPointer m_pSelectedTrack;

private slots:
void slotTrackSelected(TrackPointer pTrack);
void slotResetSelectedTrack();
};

#endif /* CRATEFEATURE_H */
45 changes: 45 additions & 0 deletions src/library/cratehighlightdelegate.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include "library/cratehighlightdelegate.h"
#include "library/treeitem.h"
#include "trackinfoobject.h"
#include "libraryfeature.h"
#include "cratefeature.h"

CrateHighlightDelegate::CrateHighlightDelegate(QObject* parent)
: QStyledItemDelegate(parent) {
}

// This will be called to by Qt before painting the TreeView-Item. Set up styles here
void CrateHighlightDelegate::initStyleOption(QStyleOptionViewItem* option,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add here a comment, when it is called?

const QModelIndex& index) const {
if (!index.isValid()) {
return;
}

QStyledItemDelegate::initStyleOption(option, index);
QStyleOptionViewItemV4 *optionV4 = qstyleoption_cast<QStyleOptionViewItemV4*>(option);

// If the item has no parent then it is a top-level sidebar item and its
// internalPointer is of type SidebarModel*, not TreeItem*.
if (!index.parent().isValid()) {
return;
}

TreeItem* item = static_cast<TreeItem*>(index.internalPointer());
if (item == NULL) {
return;
}

LibraryFeature* pFeature = item->getFeature();
if (pFeature == NULL) {
return;
}

TrackPointer pTrack = pFeature->getSelectedTrack();
if (pTrack.isNull()) {
return;
}

if (pFeature->isTrackInChildModel(pTrack->getId(), item->dataPath())){
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This leads finally to a conains() call for every crate which end up in while loop that compare key and item.
We should think about an sql Query a similar operation on an optimized hash table that is done one time in the selected track slot, preparing a list of crates that are highlighted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Of cause the "is it worth" question should be solved first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a little tricky due to the structure of the delegate -- but yes that would be better.

This could be done without a delegate by changing TreeItemModel::data() to return a bolded font for the Qt::FontRole if the crate is selected. That way you could get the list of crates/playlists that should be bold in BasePlaylistFeature/CrateFeature and then tell TreeItemModel to make those entires bold.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This could be done without a delegate by changing TreeItemModel::data() to return a bolded font for the Qt::FontRole if the crate is selected.

I had no luck with those roles, nothing changed if I used them. So I chose the delegate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is also a case where SidebarModel -> TreeItemModel is tripping things up. I'll send you a PR

optionV4->font.setBold(true);
}
}
9 changes: 9 additions & 0 deletions src/library/cratehighlightdelegate.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#include <QStyledItemDelegate>

class CrateHighlightDelegate : public QStyledItemDelegate {

public:
CrateHighlightDelegate(QObject* parent = 0);

void initStyleOption(QStyleOptionViewItem* option, const QModelIndex& index) const;
};
38 changes: 38 additions & 0 deletions src/library/dao/cratedao.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,32 @@ CrateDAO::~CrateDAO() {

void CrateDAO::initialize() {
qDebug() << "CrateDAO::initialize()";

//get the count to allocate HashMap
int tracksInCratesCount = 0;
QSqlQuery query(m_database);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does query.size() return a valid value with sqlite? Than we can get around the second query.

If not, you need not Query if tracksInCratesCount == 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

query.prepare("SELECT COUNT(*) from " CRATE_TRACKS_TABLE);

if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

tracksInCratesCount = query.value(0).toInt();

m_cratesTrackIsIn.reserve(tracksInCratesCount);

//now fetch all Tracks from all crates and insert them into the hashmap
query.prepare("SELECT track_id, crate_id from " CRATE_TRACKS_TABLE);
if (!query.exec()) {
LOG_FAILED_QUERY(query);
}

const int trackIdColumn = query.record().indexOf("track_id");
const int crateIdColumn = query.record().indexOf("crate_id");
while (query.next()) {
m_cratesTrackIsIn.insert(query.value(crateIdColumn).toInt(),
query.value(trackIdColumn).toInt());
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This basically copies the whole crates table to memory.
Up to now we have to care about this table and the original library table.
This is extra work for all crate actions and a source of errors because of two data storages.

Do we have test data, how this compares to the original solution?
I think we have a penalty on mass actions.
Is it worth the work? Remember that the selected Track slot is already decoupled by a timer.

Is there an alternative way to speed up the sql lookups e.g. a temp table?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This basically copies the whole crates table to memory.
Up to now we have to care about this table and the original library table.
This is extra work for all crate actions and a source of errors because of two data storages.

That's right -- but it's not that much memory (roughly 16-bytes per tuple so we're talking about like 16MB if every track was in a playlist in a 1-million track library). The main issue is keeping them in sync and since the DAO is already the gate-keeper it will do that fine.

I think having an in-memory index for fast lookups will help for bugs like these too:
https://bugs.launchpad.net/mixxx/+bug/1402133
https://bugs.launchpad.net/mixxx/+bug/1021422

For a GMail-style label delegate in the main library view we will need a fast lookup in memory because I can only think of 2 other ways:

  • Doing an additional query per visible track in the UI.
  • Incorporating a join of all crates/playlists into the root library view.

Going with option 1 would mean terrible things for UI performance. Going with option 2 would mean adding a track to a crate would invalidate / recompute the library SQLite view which I don't think would be speedy.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have test data, how this compares to the original solution?
I think we have a penalty on mass actions.
Is it worth the work? Remember that the selected Track slot is already decoupled by a timer.

It's currently hard to capture UI jankiness in a test. But it blocks the UI thread -- that's for sure. Until we can merge Nazar's work and keep the UI thread unblocked when doing DB operations then I think we should go this route.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, which way to go?
For this one we could also query the crates/Playlists the selected track is in in slotTrackSelected and look up there. But that won't solve the other problem for the two mentioned bugs above.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is finally a decision of taste. If you have a working solution that has a reasonable CPU load it is a good one.
It is no issue to change the way of implementation later if we actually found performance issues.

This and to quoted bugs can be solved with an in memory table and with an sql qerry.

Outlook for the future:
Nazars branch provides a non blocking access to sql database and will be part of Mixxx 1.13. The result is prepared in a separate thread and passed to the GUI thread in a new slot.
Once this is integrated we have the option to block the GUI until the result is calculated from an in memory table or delay the GUI update in a non blocking way by until the SQL query is finished.
Such a decoration feature like this is IMHO a candidate for the later option.

Without the non blocking SQL access, an internal table access will block the GUI shorter, than an sql query, but we need more CPU to keep the table up to date and there is an amount of additional code maintenance and complexity. For now it looks like the internal table wins.
Do you have a working solution for both? Take the on that suits more to you.
I have no Idea which performance tweaks are worth the works.

I think I will test it on my atom net-book, once we consider this as merge-able.

By the way: Nazars branch needs a new maintainer. The latest version is already working, but it falls behind the master branch. The next steps to do is to merge it with master by crawling up the master branch one by one database related commit. You will find it here: #73

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is finally a decision of taste. If you have a working solution that has a reasonable CPU load it is a good one.

At the moment this branch contains a working solution. I've worked with it a while without performance problems. I'm on an i5 with 16Gig RAM so your atom would be the better test-case.

Take the on that suits more to you.
As the in-memory table is ready now and tested for me, I would stick with that one for now. Once we have Nazars work in it would be worth refactoring that though - having multiple datasets that have to be in sync is clearly a point of failure.

So what about a test on your atom platform and when scolling by holding down arrowkeys or sth. like this isn't slowed down merge this as is and else test the other solution to see whats better?

I've just looked in nazars branch... will take a while to get into this...

}

unsigned int CrateDAO::crateCount() {
Expand Down Expand Up @@ -236,6 +262,9 @@ bool CrateDAO::deleteCrate(const int crateId) {
transaction.commit();

emit(deleted(crateId));

//Update in-memory map
m_cratesTrackIsIn.remove(crateId);
return true;
}

Expand Down Expand Up @@ -334,6 +363,7 @@ bool CrateDAO::addTrackToCrate(const int trackId, const int crateId) {

emit(trackAdded(crateId, trackId));
emit(changed(crateId));
m_cratesTrackIsIn.insert(crateId, trackId);
return true;
}

Expand All @@ -359,6 +389,7 @@ int CrateDAO::addTracksToCrate(const int crateId, QList<int>* trackIdList) {
// Emitting the trackAdded signals for each trackID outside the transaction
foreach(int trackId, *trackIdList) {
emit(trackAdded(crateId, trackId));
m_cratesTrackIsIn.insert(crateId, trackId);
}

emit(changed(crateId));
Expand All @@ -381,6 +412,7 @@ bool CrateDAO::removeTrackFromCrate(const int trackId, const int crateId) {

emit(trackRemoved(crateId, trackId));
emit(changed(crateId));
m_cratesTrackIsIn.remove(crateId, trackId);
return true;
}

Expand All @@ -401,6 +433,7 @@ bool CrateDAO::removeTracksFromCrate(const QList<int>& ids, const int crateId) {
}
foreach (int trackId, ids) {
emit(trackRemoved(crateId, trackId));
m_cratesTrackIsIn.remove(crateId, trackId);
}
emit(changed(crateId));
return true;
Expand All @@ -421,4 +454,9 @@ void CrateDAO::removeTracksFromCrates(const QList<int>& ids) {
// TODO(XXX) should we emit this for all crates?
// emit(trackRemoved(crateId, trackId));
// emit(changed(crateId));
// remove those tracks from memory-map
}

bool CrateDAO::isTrackInCrate(const int trackId, const int crateId) {
return m_cratesTrackIsIn.contains(crateId, trackId);
}
3 changes: 3 additions & 0 deletions src/library/dao/cratedao.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <QObject>
#include <QMap>
#include <QMultiHash>
#include <QSqlDatabase>

#include "library/dao/dao.h"
Expand Down Expand Up @@ -61,6 +62,7 @@ class CrateDAO : public QObject, public virtual DAO {
bool removeTracksFromCrate(const QList<int>& ids, const int crateId);
// remove tracks from all crates
void removeTracksFromCrates(const QList<int>& ids);
bool isTrackInCrate(const int trackId, const int crateId);

signals:
void added(int crateId);
Expand All @@ -74,6 +76,7 @@ class CrateDAO : public QObject, public virtual DAO {

private:
QSqlDatabase& m_database;
QMultiHash<int,int> m_cratesTrackIsIn;
DISALLOW_COPY_AND_ASSIGN(CrateDAO);
};

Expand Down
Loading