We were accidentally overwriting first account in the model with
currently logged in user after polling AccountsService
BUG: 336994
davidedmundson |
Plasma |
We were accidentally overwriting first account in the model with
currently logged in user after polling AccountsService
BUG: 336994
check if kcmshell5 user_manager lists mutliple users on cold boot
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
Bulletproof steps to reproduce for me:
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...
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.
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 Diff #34027) | 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 ↗ | (On Diff #34027) | 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. |
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
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
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.