(fix) Playlists: keep correct track selection (# position) when sorting - #13103
Conversation
a4c0768 to
f99815a
Compare
f99815a to
fe9c71d
Compare
|
I went with the position->row hash. |
|
This is actually a bugfix for 2.4, though it's low prio and I was to lazy to resolve the conflicts when applying this to 2.4 (since I worked off a main-based branch with lots of changes). |
|
LGTM with a minor nitpick. |
|
What you mean with nitpick? |
|
I mean my being "pingelig" about comparing with <=0 instead of -1 |
|
Did you miss to Submit your review? I don't see anything. |
Oops! |
fecdccc to
3191b6e
Compare
|
There's a merge conflict now |
3191b6e to
81d60a1
Compare
d5e1ceb to
81d60a1
Compare
|
I have rebased this onto 2.4 and dropped the 'p' prefix commit in order to hopefully reduce merge conflicts with main. |
|
@m0dB I think I addressed your comments, wanna check it once more? |
ronso0
changed the title
|
ping |
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
Its works like a charm. The position look up can be simplified see my comments.
| trackPosToRows.insert(rowInfo.position, i); | ||
| } | ||
| } | ||
| if (!m_positionColumn.isEmpty()) { |
There was a problem hiding this comment.
How about wrapping this to bool hasPositionColumn()?
I am unsure, but it can probably be reimplemented as:
fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION) >= 0
| @@ -271,6 +279,18 @@ void BaseSqlTableModel::select() { | |||
| rowInfo.trackId = trackId; | |||
| // current position defines the ordering | |||
There was a problem hiding this comment.
Unrelated: I do not really understand this comment?
rowInfo.order will be the rownNumber maybe should remove the comment and rename the element instead.
There was a problem hiding this comment.
yup, removed and renamed order -> row
| struct RowInfo { | ||
| TrackId trackId; | ||
| int order; | ||
| int position; // used by playlist models only |
There was a problem hiding this comment.
This seems to be already in the metadata below. The name metadata IMHO misleading. Can we rename it into columnValues or such?
As a workaround it is accessed as const QVector<QVariant>& columns = rowInfo.metadata;
There was a problem hiding this comment.
I think you can replace the position with a getter function in BaseSqlTableModel: (Not tested)
int getPosition() {
int posColumn = fieldIndex(ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION);
if (posColumn < 0) {
return -1;
}
return metadata[posCoumn];
}
There was a problem hiding this comment.
True. I copied the trackId implementation, but the position is needed (explicitly) only once to populate TrackPos2Row, so this change makes sense. Thanks!
5995cf9 to
cd160bb
Compare
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
LGTM. Thank you. Con you sqash the fixup and resolve the conflicts?
Than we can merge this.
cd160bb to
29526d7
Compare
|
Thank you. |
4 participants
Previously, tracks were reselected by TrackId after sorting which would add wrong tracks to the selection in case the playlist contains tracks multiple times.
Introduces QHash<int row, int position>, equivalent to existing
m_trackIdToRowshash, and repopulates that on every select (if table is a playlist).Then, when sorting, query that for each previously selected playlist position, select, done.