Skip to content
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

Library track selection tweaks #4536

Merged
merged 4 commits into from
Jan 7, 2022
Merged

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 22, 2021

follow-up of #4177, picking bites from #3063 to improve the track selection when changing features and search queries.

  • only attempt to load a saved selection state if the new search query is more specific

@ronso0 ronso0 added this to the 2.4.0 milestone Nov 22, 2021
@ronso0 ronso0 added changelog This PR should be included in the changelog and removed changelog This PR should be included in the changelog labels Nov 22, 2021
@ronso0 ronso0 force-pushed the lib-selection-tweaks branch from f600fe8 to 8310d05 Compare November 22, 2021 01:34
src/library/libraryview.h Show resolved Hide resolved
src/library/searchqueryparser.cpp Outdated Show resolved Hide resolved
src/library/searchqueryparser.cpp Outdated Show resolved Hide resolved
src/library/searchqueryparser.h Outdated Show resolved Hide resolved
src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved
src/widget/wtracktableview.cpp Show resolved Hide resolved
@ronso0 ronso0 force-pushed the lib-selection-tweaks branch from 8310d05 to ddcd895 Compare November 24, 2021 12:25
@ronso0
Copy link
Member Author

ronso0 commented Nov 24, 2021

Note: I marked it as draft because I'll likely rebase -- still want to test and work out some details.
Thanks for the early review though!

@ronso0 ronso0 force-pushed the lib-selection-tweaks branch from ddcd895 to 3c7298b Compare November 24, 2021 14:03
@ronso0 ronso0 marked this pull request as ready for review November 24, 2021 22:37
@ronso0
Copy link
Member Author

ronso0 commented Nov 24, 2021

Actually, this is it.
initially I was planning to pick save TrackIds instead of index if possible from 7b9e405, too, but this state here is sufficient for now, so I'll resume some other PRs instead.

@poelzi
Copy link
Contributor

poelzi commented Nov 25, 2021

Saving only indexes is super annoying, thats why I implemented the IDs. I often use a filter like
"genre played:0" and play some stuff. If tracks are playing and you are in the history for example and switch back, causes the wrong selection. So, the new code tries to save trackids if the view supports those.

@ronso0
Copy link
Member Author

ronso0 commented Nov 25, 2021

Sure. you can open a PR at my branch with that change (only) that also saves the focused column of the current TrackId's row because that's what I missed last time I tested it.

Sooner or later I'll do it myself, but not in this PR

@ronso0 ronso0 marked this pull request as draft November 25, 2021 14:35
@ronso0 ronso0 force-pushed the lib-selection-tweaks branch from 3c7298b to 400bb8f Compare November 25, 2021 21:45
@ronso0 ronso0 marked this pull request as ready for review November 25, 2021 21:48
@ronso0
Copy link
Member Author

ronso0 commented Nov 25, 2021

Okay, now this works also if an invalid (invisible) column is passed to setCurrentTrack() in case it is invoked from somewhere else then onSearch().

@Holzhaus Holzhaus requested a review from uklotzde January 7, 2022 16:07
src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Jan 7, 2022

@uklotzde
I will squash the last --fixup commit after the final LGTM

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I cannot promise that there are no regression. But code looks good, so let's move on. Thank you! LGTM

@uklotzde uklotzde merged commit e7fde41 into mixxxdj:main Jan 7, 2022
@uklotzde
Copy link
Contributor

uklotzde commented Jan 7, 2022

However the local build with Qt 5.15 failed:

/home/uk/volumes/Build/cpp/mixxx/src/library/searchqueryparser.cpp:396:77: error: 'SkipEmptyParts' is deprecated [-Werror,-Wdeprecated-declarations]
    QStringList queryWordList = query.split(kSplitIntoWordsRegexp, QString::SkipEmptyParts);
                                                                            ^
/usr/include/qt5/QtCore/qstring.h:605:24: note: 'SkipEmptyParts' has been explicitly marked deprecated here
        SkipEmptyParts Q_DECL_ENUMERATOR_DEPRECATED
                       ^
/usr/include/qt5/QtCore/qcompilerdetection.h:1160:40: note: expanded from macro 'Q_DECL_ENUMERATOR_DEPRECATED'
#  define Q_DECL_ENUMERATOR_DEPRECATED Q_DECL_DEPRECATED
                                       ^
/usr/include/qt5/QtCore/qcompilerdetection.h:227:45: note: expanded from macro 'Q_DECL_DEPRECATED'
#  define Q_DECL_DEPRECATED __attribute__ ((__deprecated__))
                                            ^
/home/uk/volumes/Build/cpp/mixxx/src/library/searchqueryparser.cpp:396:39: error: 'split' is deprecated: Use Qt::SplitBehavior variant instead [-Werror,-Wdeprecated-declarations]
    QStringList queryWordList = query.split(kSplitIntoWordsRegexp, QString::SkipEmptyParts);
                                      ^
/usr/include/qt5/QtCore/qstring.h:627:23: note: 'split' has been explicitly marked deprecated here
    Q_REQUIRED_RESULT QT_DEPRECATED_VERSION_X_5_15("Use Qt::SplitBehavior variant instead")
                      ^
/usr/include/qt5/QtCore/qglobal.h:374:45: note: expanded from macro 'QT_DEPRECATED_VERSION_X_5_15'
# define QT_DEPRECATED_VERSION_X_5_15(text) QT_DEPRECATED_X(text)
                                            ^
/usr/include/qt5/QtCore/qglobal.h:294:33: note: expanded from macro 'QT_DEPRECATED_X'
#  define QT_DEPRECATED_X(text) Q_DECL_DEPRECATED_X(text)
                                ^
/usr/include/qt5/QtCore/qcompilerdetection.h:675:55: note: expanded from macro 'Q_DECL_DEPRECATED_X'
#    define Q_DECL_DEPRECATED_X(text) __attribute__ ((__deprecated__(text)))
                                                      ^
2 errors generated.

Fixed by #4609.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants