Highlight Crates a Track is in#598
Conversation
|
Hi Markus, thank you for working on this. Sorry I have no answer for your question. So far the code seams to be on the right track. |
|
Thanks for your comments. Ive changed it and will commit shortly. The repaint-issue is harder than I thought. I've tried with dataChanged signal of the TreeItemModel but without success. For now the web gives no other solution for me I fear. But will continue searching. |
|
When is initStyleOption() called? The init prefix makes me think that it is only called one time. It think you have to invalidate the index inside CrateFeature::slotTrackSelected. Than you probably have to override the paint() function to paint yourself or override displayText and change the style there. http://doc.qt.io/qt-4.8/qstyleditemdelegate.html You may also search Github for qstyleditemdelegate to see how others use it. |
|
I've managed to fix it. kind of hackish, but works. I'm telling the model that I will insert rows what gives a repaint. Nothing else worked for me. If you're ok with this, I will also add handling for playlists before we merge this. |
There was a problem hiding this comment.
Can you add here a comment, when it is called?
There was a problem hiding this comment.
did you try
emit dataChanged()
http://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged
"When reimplementing the setData() function, this signal must be emitted explicitly."
QModelIndex().parent() looks a bit scary can we use at least the real indexes here?
This function repaints the whole tree, for some extra points, it would be nice to invalidate only the effected items.
There was a problem hiding this comment.
Yes, i tried emit dataChanged() with various parameters. I couldn't get it to work with that. From that point on, I was unable to repaint only some items so I stick with repainting the whole tree.
There was a problem hiding this comment.
This is similar to how we do it in BaseSqlTableModel. Did you try this way?
QModelIndex left = index(0, 0);
QModelIndex right = index(numRows, numColumns);
emit(dataChanged(left, right));There was a problem hiding this comment.
Yes I did (just now once again to be sure). It has no effect. The Widget will not repaint.
I have a solution now without the sideeffect of moving selection by doing
beginInsertRows(QModelIndex(), rowCount(), rowCount());
endInsertRows()
but that also isn't the nice way...
There was a problem hiding this comment.
I figured this one out -- SidebarModel wasn't re-broadcasting the dataChanged signal. (sent a PR)
Yes, go ahead. |
|
well... playlists are buggy. For some reason, I do not understand, the selection in the treeview moves one item down if a playlist is selected. Will investigate... |
|
Oh. actually that happens for crates also. So we need another way to trigger the repaint than that hack, that seems to have side-effects. |
|
Thanks for working on this Markus. I'm worried that doing blocking database queries in response to UI movements will make it jerky for rapid scrolling through the library. Also, this implementation does one database query per crate/playlist when you could do the same with 2 queries (one to get all the crates a track is in and one to get all the playlists a track is in). A couple ways this could be resolved:
|
|
would you prefer to go with the in memory map or doing it with two querys, by holding the crates/playlists a track is in in memory? When we decide to do it in memory, which dataStructure would be preferred? using Still no nice solution for repainting, but one that works without sideeffects so far. |
I think it'd be best to go with the in-memory map idea. That way we only pay a small insert/update cost on every Playlist/Crate modification operation in the DAOs. Compared to the DB queries associated with each of those operations the extra cost is tiny :). Whereas doing queries in response to GUI operations has caused a lot of jerkiness in our UIs in the past.
A |
|
Hm, I'm getting segfaults on start: |
|
thats's weird... I'm on linux, everything is working here... |
There was a problem hiding this comment.
The segfault is because the internal pointers in the sidebar can be SidebarModel* for items that are children of the root and TreeItem* for all others.
There was a problem hiding this comment.
I'm not that experienced with that casting.
So I should do a dynamic_cast here and check if the result is ok?
dynamic_cast cant cast from void* to another, only downcast. Which way to go then?
Still can't reproduce the segfault. strange.
…ateHighlightDelegate.
|
cannot see difference with my 20 entries for both commits. |
|
@rryan would it be OK to go back to the Delegate solution then, because it was faster for me? |
|
I'd like to avoid delegates as they prevent us from taking faster paths in the Qt library code (generally we've had tons of trouble with the 3 delegates in our library table causing performance issues). |
I think reversing the playlist hash key/value pairing to be (track, playlist) instead of (playlist, track) will help. As it works now we do a contains check for every playlist when you select a new track. Because If the key/values were switched then it would be more efficient since we would typically be iterating through only the playlists IDs the track is in. Another downside is that we do N hash lookups to iterate through the playlists the track is in. Since we have a multi-hash we could actually just search through the iterator. Instead of offering a contains() check from PlaylistDAO we should probably instead offer a lookup method that returns an iterator or maybe just populates a list or a set. |
|
I'm happy to make the above changes and then maybe you could test if they fix your slowness @MK-42? |
I can test that, no problem 👍 |
Sent you a PR |
|
Sorry, I'm busy. Will test this tonight Am 04.06.2015 um 17:04 schrieb RJ Ryan:
|
|
As commented in that pr, thats not faster and i have other playlists/tracks highlighted then those that contain the tracks. So highlighting is wrong. |
Revert "Some fixes." these were slow
|
Works for me now as expected - hovers the right things and is fast. ok to merge for me |
I think this was why we had to add signals to each individual feature.
|
I'd prefer to iterate on the previous commit since it avoids adding extra code and avoids a delegate. (the 3 delegates in our library table are the single biggest source of scrolling smoothness issues in the library). The wrong data was a dumb mistake -- just pushed a commit to my branch: |
|
I've just seen that i missed an important change in your changed and cherry-picked that. But will test your branch now once again. |
|
Well. Now it does highlighting right. But as soon as I expand the 'History' LibraryFeature, it takes about 2 seconds to move the selection if I press a arrow-key on my keyboard. The same with selecting one entry with the mouse. It takes about that time to get the selection change. Like this this is nearly unusable. One could deactivate bolding in the history-tree, but I would actually like the feature to see "did i played that the last time?". Could it be, that the delegate only fetches the 'isBold'-data for the visible items and the current solution for all of them? Edit: Also switching of libraryFeatures got that slow |
Yea, this change populates the bold data once when the track is selected for all playlists. To verify that this method is taking a long time, could you stick a PerformanceTimer in it? For example, stick this at the top of BasePlaylistFeature::slotTrackSelected: ScopedTimer t("BasePlaylistFeature::slotTrackSelected");Then run in --developer mode, select some tracks with the History section open, and then quit. It will print out a stats page to the console including statistics about the run time of that method. How many history playlists do you have? |
about 300 i think. It's not that comfortable to delete them (there is no mass-delete or is there?) so they just build up... Without history expanded: 6.4e+08ns is 0.6 seconds. It feels longer than that for the selection to change but I think that there is the problem then? |
|
The DAO seems fast: so I think it is the for-loop after that setting the bold-info or am I missing something? There we are - its nearly the whole time in that loop. Interesting: The first selection is always fast. And Its not the contains-call on the QSet using that time. Will investigate further |
|
I think the problem is the setData in TreeItemModel which is called hundreds of times and everytime triggers a repaint via emit dataChanged. Edit: now I am sure that thats the problem. I've written an setBold in treeItemModel that does not call the dataChanged signal and now it is fast again. We need to somehow switch that signal of, update all bold states and then retrigger a whole update for the whole treeview. Or what do you think? |
|
@MK-42 ahh thanks for digging into that! Makes sense. To sidestep the updates we can reach into the TreeItem model itself: diff --git a/src/library/baseplaylistfeature.cpp b/src/library/baseplaylistfeature.cpp
index 553e381..550f1d3 100644
--- a/src/library/baseplaylistfeature.cpp
+++ b/src/library/baseplaylistfeature.cpp
@@ -600,16 +600,24 @@ void BasePlaylistFeature::slotTrackSelected(TrackPointer pTrack) {
int trackId = pTrack.isNull() ? -1 : pTrack->getId();
m_playlistDao.getPlaylistsTrackIsIn(trackId, &m_playlistsSelectedTrackIsIn);
+ TreeItem* rootItem = m_childModel.getItem(QModelIndex());
+ if (rootItem == NULL) {
+ return;
+ }
+
// Set all playlists the track is in bold (or if there is no track selected,
// clear all the bolding).
int row = 0;
for (QList<QPair<int, QString> >::const_iterator it = m_playlistList.begin();
it != m_playlistList.end(); ++it, ++row) {
int playlistId = it->first;
- QModelIndex index = m_childModel.index(row, 0);
+ TreeItem* playlist = rootItem->child(row);
+ if (playlist == NULL) {
+ continue;
+ }
bool shouldBold = m_playlistsSelectedTrackIsIn.contains(playlistId);
- m_childModel.setData(index, shouldBold, TreeItemModel::kBoldRole);
+ playlist->setBold(shouldBold);
}
} |
|
But then it needs a full dataChanged refresh using a method like the one you wrote. |
TreeItemModel::triggerRepaint |
|
Thats it. Like that it works. Thanks :) But now ive screwed up that thing so much. Would it be ok to do a PR against your branch and you merge yours into master? |
|
nice! sure I'll commit that patch and make a PR |
|
closing this as @rryan opend another one for this feature. |
3 participants
I've added some basic handling for lp: 1380467 (https://bugs.launchpad.net/mixxx/+bug/1380467)
This will save the last selected track from trackSelected signal of the library and use a delegate for the LibraryTreeView to highlight the crates that track is in.
Todo:
Find a way to repaint the widget after style-change (any hints on that?) for now scrolling the treeView brings it to live.
Apply the same for playlists
Some questions:
btw: what is dao short for? Maybe st. like dataAccessObject? Just curious, maybe makes it easier to read for me.