We were accidentally overwriting first account in the model with
currently logged in user after polling AccountsService
BUG: 336994
Details
Details
- Reviewers
davidedmundson - Group Reviewers
Plasma - Commits
- R128:ff88e24e4380: Split replaceAccount from addAccountToCache
check if kcmshell5 user_manager lists mutliple users on cold boot
Diff Detail
Diff Detail
- Repository
- R128 User Manager
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Bulletproof steps to reproduce for me:
- System Settings > Account Details > User Manager > Create a new user
- Give the new user admin rights
- Log out
- Log in as that the new user
- Go to System Settings > Account Details > User Manager
The user you were just logged in as before isn't listed!
This patch fixes that bug for me. The patch looks sane, but this code seems fragile and really needs an autotest...
Comment Actions
It might work, but it's overly messy.
We have addAccountToCache which has an argument pos, which determines if we're adding or replacing.
Then we have a second replace method replaceAccount which also does replacing.
So we have two ways of replacing an entry both and both are used once, and they're both identical except for a very subtle change on usage of m_userPath.
I'm sure we can do something better.
Comment Actions
We have addAccountToCache which has an argument pos, which determines if we're adding or replacing.
I've actually removed this bit, addAccountToCache now only adds accounts (either inserts at pos or appends)
Though I agree that current code is probably a bit fragile, maybe the model can be changed the way there's no need in manual ordering or at least storing "fake user". If that seems like a better approach I can look into that.
| src/lib/accountmodel.cpp | ||
|---|---|---|
| 361 | On second look, I might have misunderstood. This isn't a replace, it's just making sure the current user is at the top? | |
| src/lib/accountmodel.cpp | ||
|---|---|---|
| 361 | Yes. Replace code was added to addAccountToCache at some point because of the "modify new user entry" logic (line 434 in the original), which broke addAccountToCache here. This patch moves the replace code out to replaceAccount so addAccountToCache does what the name says again. In fact replaceAccount technically adds a user too but does it by replacing fake user entry ("+ Add new user" in the list) with it 😕 This whole bit might be worth refactoring to kick fake user entry out of model containers & add tests, but I don't think I'll be able to do it this week. | |
Comment Actions
Well, it does apply cleanly to Plasma/5.12 branch and it still works, but I don't really know if there's a special procedure for landing patches onto stable branches
Comment Actions
I meant it more in terms of "not landing it right before next stable release" :v But if it's okay to land now I can land it right now
Comment Actions
The next stable release is 5.12.6, which is scheduled for 2018-06-27 (actually after 5.13!). So I think you should be fine.