BaseSqlTableModel: disable undesirable sorting#4735
BaseSqlTableModel: disable undesirable sorting#4735davidchocholaty wants to merge 2 commits intomixxxdj:mainfrom
Conversation
This commit fixes undesirable tracks sorting in tables.
The mentioned situation could have occurred, if the stars rating
was changed for some tracks and then after the action
of removing tracks, adding tracks or filtering, the tracks
were sorted.
So, when removing tracks now tracks are kept at the same position
for a user. With adding tracks the new tracks are added at the end
of the tracks table. The automatic sorting is kept when it is changed
between track tables, for example between crates and tracks.
Fixes: #1387373 ("Remove a track from a crate triggers sorting")
Reported-by: Jean Claveau
|
Thank you for working on this issue! Does this bug occur in Mixxx 2.3.2? If so, the fix should go to the 2.3 branch. Please rebase and force-push. |
Such a |
Oh wow, I didn't look at the change set. |
|
Thank you for this not that easy first PR. It took me a while to wrap my head around it. It solves the issue, but for my understanding it needs some tweaks. I have found two remaining issues:
I think we should either resort after dropping a file, or place it where it was dropped. |
uklotzde
left a comment
uklotzde
left a comment
There was a problem hiding this comment.
This PR doesn't work for me. Search results are not updated as expected when modifying the query.
I am also concerned about storing even more data in memory.
| } | ||
|
|
||
| if (!m_sortedTracksIndexes.isEmpty()) { | ||
| if (trackIds.size() < m_sortedTracksIndexes.size()) { |
There was a problem hiding this comment.
These assumptions are wrong. An arbitrary number of tracks could be added or removed. Even if the size didn't change then the number of additions equals the number of removals. That's all you could deduce.
There was a problem hiding this comment.
I just want to make sure I understand the problem correctly. My solution do these comparisons to find out if tracks were removed or added by the last sorted tracks. As you said, it is possible to remove and add the same count of tracks, I agree, but in my opinion, removing tracks and adding tracks will be done by separated method calls and it could not be done together. Or am I wrong?
There was a problem hiding this comment.
Making assumptions about such arbitrary preconditions is incorrect. The invocation of select() doesn't guarantee any of those.
| qDebug() << "Rows actually received:" << rowInfos.size(); | ||
| } | ||
|
|
||
| if (!m_sortedTracksIndexes.isEmpty()) { |
There was a problem hiding this comment.
Can we get rid of this whole block, if we just use a vector of tracks?
Removed tracks are simply missing and skipped when sorting.
New tracks are missing and can be sorted in by the code for dirty tracks.
For a visual feedback it would be nice to either scroll to the dropped track or place it just where it was dropped, which requires even more refactoring.
There was a problem hiding this comment.
I think it would be possible, I'll take a look at it.
| } else { | ||
| // Loop through tracks and store their indexes | ||
| // from the previous sorted order. | ||
| while (query.next()) { |
There was a problem hiding this comment.
If sortedIndexes would be a vector we can just loop over it and assign the sorting value consecutive if found.
There was a problem hiding this comment.
This PR doesn't work for me. Search results are not updated as expected when modifying the query.
I am also concerned about storing even more data in memory.
You are right, that it is a little bit problematic because more memory has to be used. Anyway, I couldn't find out another solution because it is necessary to store sorted tracks order. While tracks are chosen in filterAndSort() method it is necessary to have available the previous sorted order.
There was a problem hiding this comment.
If sortedIndexes would be a vector we can just loop over it and assign the sorting value consecutive if found.
Ok, I'll try to remake it.
There was a problem hiding this comment.
Thank you.
Some Ideas regarding the extra memory of redundant data:
I think we can safely reuse QVector BaseSqlTableModel::m_rowInfo which holds the old sorting and is used to implement the QAbstractTableModel interface.
BaseTrackCache::m_trackOrder this was original a local variable from BaseTrackCache::filterAndSort() made a class member to keep the allocated memory for later calls, this has changed content due to the shared nature of BaseTrackCache. Maybe we can also get rid of this ...
There was a problem hiding this comment.
Thank you for the tips. Sounds good to me. I will try what will be possible asap. I agree, that adding this new feature can be a little bit problematic with wasting memory or data duplication in some ways.
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
Bot
added
the
stale
github-actions
Bot
removed
the
stale
|
I think the issue has finally been fixed by #15872 I'll close this abandoned PR now to clean up the list of open PRs. |
4 participants
Fixes undesirable tracks sorting in tables.
The mentioned situation could have occurred if the star rating
was changed for some tracks and then after the action
of removing tracks, adding tracks or filtering, the tracks
were sorted.
So, when removing tracks now tracks are kept at the same position
for a user. With adding tracks the new tracks are added at the end
of the tracks table. The automatic sorting is kept when it is changed
between track tables, for example between crates and tracks.
Fixes: lp:1387373 "Remove a track from a crate triggers sorting"
Reported by: Jean Claveau