Skip to content

restore last selected library feature/playlist/crate on restart - #15460

Open
mxmilkiib wants to merge 2 commits into
mixxxdj:mainfrom
mxmilkiib:feature/restore-last-library-selection
Open

restore last selected library feature/playlist/crate on restart#15460
mxmilkiib wants to merge 2 commits into
mixxxdj:mainfrom
mxmilkiib:feature/restore-last-library-selection

Conversation

@mxmilkiib

Copy link
Copy Markdown
Contributor

implements #10125

persists the currently selected sidebar item (feature or child) to user settings and restores it on next mixxx startup.

uses feature title + child data for robust matching across sessions. gracefully falls back to default selection if saved item not found.

config keys:

  • [Library] LastSelectedFeature: stores feature title
  • [Library] LastSelectedChild: stores child item data (playlist/crate id)

@daschuer daschuer left a comment

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.

Thank you for this nice feature.

Comment thread src/library/sidebarmodel.cpp Outdated
Comment thread src/library/sidebarmodel.cpp Outdated
@ronso0

ronso0 commented Oct 8, 2025

Copy link
Copy Markdown
Member

Why not simply store the number of the selected row + childrow?
The config value is not ment to be readable anyway.
And it would save us the TreeItem inspection.

[Library]
LastSelectedFeature 2-3

would be the 4th Playlist item.

Comment thread src/library/sidebarmodel.cpp Outdated
QModelIndex found = findChild(childIndex);
if (found.isValid()) {
return found;
}

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.

No need to iterate over children manually, just use pChildModel::match() and stop at first match.

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'd still prefer the row+childrow approach btw.

Note: both name- and index-based lookup will fail for Computer as the tree is created on demand / on expand.
For History it may fail if auto-cleanup deleted the selected playlist.

@mxmilkiib

Copy link
Copy Markdown
Contributor Author

it can be changed if you think it would be less brittle for all/most cases

@ronso0

ronso0 commented Oct 8, 2025

Copy link
Copy Markdown
Member

Well, there's no way to make it work for Computer and obviously auto-removed History items can't be selected.
So, while I'd prefer the index approach (saving is much simpler IMO), the name approach is okay IMHO if we want to make it work 100% also for history. In that case, please check the match method I mentioned as that would reduce code siginificantly afaict.

Comment thread src/library/sidebarmodel.cpp Outdated
pFeature->activateChild(savedIndex);
}
}
}

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.

Why not use SidebarModel::clicked(index)?
Looks it does the exact same thing.

@ronso0

ronso0 commented Oct 8, 2025

Copy link
Copy Markdown
Member

I'm curious, did you use ai to code this?

@mxmilkiib

Copy link
Copy Markdown
Contributor Author

my preference would be for it to have that wider scope but with the graceful fallback/degradation

thanks, I'll check/use the suggestions later

yes, I used Claude Sonnet 4.5 thinking

it feels like Claude 4+, and GPT-5, are half-way around turning the not-slop corner. afaiu, this relates to the context window embiggening

(Windsurf currently has reduced rates for both, which is one facet of the bubble right now tbqf. I can't afford pro-level-spec-taking project-building Claude Opus)

quality depending on the correct promoting and process used; for recent context, the two big polarised HN stories about curl & AI (and stories like this and related comments)

(this aside from the relatively complex proof-is-in-the-pudding LPP3 script I've put together, lots of time being mindful of code smells, 8100+ LOC that I'll eventually modularise)

I'm thinking of getting together a RULES txt file guide for working with Mixxx, to collate patterns and preferences from the project (beyond just pointing to the wiki, which I forgot last night)

it's also helping me greatly with git and feature-branch/PR management all

(plus I've also recently finally massively refactored many dotconfigs, including making several AwesomeWM Lua modules)

@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch 2 times, most recently from e76622b to b2bde95 Compare October 18, 2025 10:27
Comment thread src/library/sidebarmodel.cpp Outdated
Comment on lines +633 to +634
QString featureIconName;
QString childData;

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.

Suggested change
QString featureIconName;
QString childData;
QString featureName;
QString childName;

@ronso0

ronso0 commented Oct 18, 2025

Copy link
Copy Markdown
Member

This is currently failing because the value QAbstractItemModel::match() is looking for is a QString (from config), but the model's data may either be int (playlist or crate id) or a QString (Computer -> Quick Links -> path).
Converting the config string might work, but we may also have item data consisting of int which are actually strings.
I think we need to first figure which data type the child model uses (query first child) and convert to that.

@ronso0

ronso0 commented Oct 18, 2025

Copy link
Copy Markdown
Member

Besides, we need to use TreeItemModel::kDataRole for match().

Here's a fixup 6582dc2
(incl. reduced nesting, type check etc.)

Tested with playlists and Quick Links items.

@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from b2bde95 to 28ec3de Compare October 19, 2025 01:21
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch 2 times, most recently from b1a8f86 to f188d9a Compare October 19, 2025 18:09
@github-actions github-actions Bot added the qml label Oct 19, 2025
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from f188d9a to 5eb6c5a Compare October 19, 2025 18:14
@mxmilkiib
mxmilkiib requested a review from daschuer October 19, 2025 19:48
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from 5eb6c5a to ad5067a Compare October 20, 2025 00:02
@mxmilkiib
mxmilkiib marked this pull request as ready for review October 20, 2025 00:45
@mxmilkiib

Copy link
Copy Markdown
Contributor Author

it now stores the positioning of the sidebar view after each change

so, instead of trying to have the selected entry be bottom-most or centre, it just restores where the user had the scroll position beforehand

I think the requests were folded in properly

any other failure modes I should add protection for?

Comment thread src/library/sidebarmodel.cpp Outdated
// this event.
stopPressedUntilClickedTimer();
if (index.isValid()) {
// save the selection for restoration on restart

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 we need to store the selection only on shutdown, ie. in the destructor, not on every click?

Comment thread src/widget/wlibrarysidebar.cpp Outdated
connect(verticalScrollBar(),
&QScrollBar::valueChanged,
this,
&WLibrarySidebar::saveScrollPosition);

@ronso0 ronso0 Oct 21, 2025

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.

Let's not write to config for every scroll.

And not remove comments unless there's a real reason.

Comment thread src/library/treeitemmodel.cpp Outdated
if (rowCount() == 0) {
return;
}

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.

why do we need this?

does this prevent repainting a child model if we for example removed all History playlists?

Comment thread src/widget/wlibrarysidebar.cpp Outdated
// Start a timer to auto-expand sections the user hovers on.
void WLibrarySidebar::dragMoveEvent(QDragMoveEvent* event) {
// qDebug() << "dragMoveEvent" << event->mimeData()->formats();
// Start a timer to auto-expand sections the user hovers on.

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.

Suggested change
// Start a timer to auto-expand sections the user hovers on.
// Start a timer to auto-expand sections the user hovers on.

Comment thread src/widget/wlibrarysidebar.cpp Outdated

if (!scrollToIndex) {
// restore saved scroll position after tree expansion completes
QTimer::singleShot(100, this, &WLibrarySidebar::restoreScrollPosition);

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.

why do we need this?
IIUC this also a no-op in most cases (click), but requires reading from config

@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from a914597 to ff6507a Compare November 14, 2025 18:30
@mxmilkiib

Copy link
Copy Markdown
Contributor Author

oops how do I github 🐾

@ronso0 ronso0 left a comment

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 looks like you added some minor bugfixes. Please create separate commits for those and explain what's wrong with the previous implementation.

Comment thread src/library/library.cpp Outdated
Comment on lines +82 to +83
m_pKeyNotation.reset(
new ControlObject(mixxx::library::prefs::kKeyNotationConfigKey));

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.

Why reset?
And this is a std::unique_ptr, not a plain ControlObject*

Comment thread src/library/sidebarmodel.cpp Outdated
Comment on lines +709 to +712
// feature not found, can't search for child
if (featureRow < 0) {
return {};
}

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 can be moved up, below for (int i = 0; i < m_sFeatures.size(); ++i) {

1, // stop at first match
Qt::MatchExactly | Qt::MatchRecursive);

if (!matches.isEmpty() && matches.first().isValid()) {

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.

results of QAbstractItemModel::match() should never be invalid.
If you think we need to ensure validity, use

VERIFY_OR_DEBUG_ASSERT(matches.first().isValid()) {
    // some qWarning()
    return {};
}

Comment thread src/widget/wlibrarysidebar.cpp Outdated

void WLibrarySidebar::setup(UserSettingsPointer pConfig) {
m_pConfig = pConfig;
// removed: connection to save on every scroll - now saves on shutdown only

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.

irrelevant for devs touching this in the future, please remove

@ronso0
ronso0 marked this pull request as draft February 6, 2026 13:54
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch 2 times, most recently from 84a1e8e to d36954a Compare February 14, 2026 07:30
@ronso0

ronso0 commented Feb 16, 2026

Copy link
Copy Markdown
Member

CI is failing. Both pre-commit and the build jobs.

@mxmilkiib

Copy link
Copy Markdown
Contributor Author

rebased and restructured into 4 commits addressing ronso0's feedback. also fixed several bugs in the original implementation:

  • saveSelectionToConfig crashed on root feature nodes (internalPointer() == this, not a TreeItem*)
  • scheduleSelectionSave was never called from clicked()
  • child matching used a custom DataRole unknown to child models; switched to Qt::DisplayRole
  • activateDefaultSelection() was called unconditionally after restore, overwriting it
  • feature/child not activated on restore, so track table didn't populate without a click

also extended the feature to save/restore the selected track row within the table (via TrackId persisted to config, consumed in loadTrackModel).

@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch 2 times, most recently from 557302a to a78ea19 Compare February 26, 2026 09:22
@ronso0

ronso0 commented Feb 26, 2026

Copy link
Copy Markdown
Member

FYI CI builds failed

@ronso0

ronso0 commented Feb 26, 2026

Copy link
Copy Markdown
Member

.. and unrelated changes slipped in

@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from a78ea19 to 6ee9eff Compare February 28, 2026 08:03
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch 4 times, most recently from 50837a2 to 03d7fca Compare May 17, 2026 02:23
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from 03d7fca to bb0ffce Compare May 26, 2026 06:13
@mxmilkiib

mxmilkiib commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

why it this stale again?

I'm not "developing", just catching up

I think it's stale because the implementation was switched out halfway through the PR and it was too much to follow at the time, but afair the later commit set neatened things up

@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from bb0ffce to 3bf3c18 Compare August 3, 2026 20:54
@mxmilkiib
mxmilkiib force-pushed the feature/restore-last-library-selection branch from 3bf3c18 to e322da9 Compare August 7, 2026 15:04
@mxmilkiib
mxmilkiib marked this pull request as ready for review August 8, 2026 03:35
@mxmilkiib

Copy link
Copy Markdown
Contributor Author

redone, actually confirmed working, still testing

now as two commits. the track reselection needed a bit extra workflow

hopefully it's methods are now concise and respectful enough of existing Mixxx systems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants