| Summary: | useless migration wizard | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] frameworks-kwallet | Reporter: | Harald Sitter <sitter> |
| Component: | general | Assignee: | Valentin Rusu <valir> |
| Status: | VERIFIED FIXED | ||
| Severity: | normal | CC: | kdelibs-bugs, mklapetek, rdieter |
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| See Also: | https://bugs.kde.org/show_bug.cgi?id=334373 | ||
| Latest Commit: | http://commits.kde.org/kwallet-framework/127efedd1668b546d0ac8c83655a2056d0439f29 | Version Fixed In: | |
| Sentry Crash Report: | |||
|
Description
Harald Sitter
2015-08-07 11:15:11 UTC
I agree with Harald that migration should happen automatically, behind the scenes, so that everything seems very seamless (wrt switching from kdelibs app to kf5 app) and the user is happy by not having to do anything while everything just works(tm). Could be done. However, wouldn't it be nice for the user to display some message box after migrating just to keep him informed? Oh, and I see another problem. Even if the migration starts in the background, the new wallet creation request will prompt the user for a password. So, without the migration wizard context, that dialog would pop-up out of the blue. The user may get even more confused in that case, IMHO. Waiting for your replies, I'll check the code to see if the pam module could help preventing that new wallet password prompt. Regarding the message box, I think even a notification is not necessary for the migration. It should "just work". The user doesn't really need to know about this if everything goes according to plan and the migration works. If the migration fails however it would absolutely make sense to show a notification that allows the user to inspect the problem (look at the log view?) and/or file a bug etc. kwallet-pam solves the password dialog as the user should not get any dialog if pam can unlock the wallet, so that should not be a problem for 99% of mainstream users. In instances without kwallet-pam I do not think this would be the only case where kwallet throws arbitrary password requests without context (plasma-nm wifi for example?). It's really more of a general design flaw than a problem limited to the this particular instance unfortunately. Also I just noticed that this wizard essentially renders kwallet-pam user experience broken on systems with both kwallet4 and kwallet5. In order to allow existing wallets to migrate I understand that kwallet5 must be started after kwallet4, so kwallet-pam systems would *always* run into the launchpad bug I mentioned above where kwalletd5 wants to migrate an empty wallet for new users. With kwallet-pam being part of Plasma 5.4 we really need to get this fixed as soon as possible to not have problems with the user experience for too long :/ (In reply to Harald Sitter from comment #3) > Regarding the message box, I think even a notification is not necessary for > the migration. It should "just work". The user doesn't really need to know > about this if everything goes according to plan and the migration works. If > the migration fails however it would absolutely make sense to show a > notification that allows the user to inspect the problem (look at the log > view?) and/or file a bug etc. Ok for that, makes sense. > Also I just noticed that this wizard essentially renders kwallet-pam user > experience broken on systems with both kwallet4 and kwallet5. > In order to allow existing wallets to migrate I understand that kwallet5 > must be started after kwallet4, That's not exact. The migration wizard actually handles kwalletd4 start via QDBusConnection::startService() > so kwallet-pam systems would *always* run > into the launchpad bug I mentioned above where kwalletd5 wants to migrate an > empty wallet for new users. That's not exact because of what I pointed-out just before. > With kwallet-pam being part of Plasma 5.4 we really need to get this fixed > as soon as possible to not have problems with the user experience for too > long :/ User experience is important, that's true. However, kwallet-pam have some design flaws that creates some problems here. For instance, it assumes user only has one wallet, always named "kdewallet". See BUG:334373 for more information. On the other hand, the migration wizard gracefully enumerates old wallets and attempt to re-create them with the same names. Users may be quite happy with that, IMHO. As a matter of fact, this request will lead to some hacky code in the migration agent in order to adjust for the hard-coded wallet created by the pam module. Finally, I won't remove the migration wizard code. I'll introduce a configuration flag allowing it to be brought back by the users. Git commit 127efedd1668b546d0ac8c83655a2056d0439f29 by Valentin Rusu. Committed on 07/08/2015 at 16:59. Pushed by vrusu into branch 'master'. Stop showing the migration wizard by default If the migration wizard is needed, then add this to kwalletrc [Migration] showMigrationWizard=true On systems having kwallet-pam the migration agent would also merge all the old wallets into the default LocalWallet, as a side effect. This would avoid wallet creation prompts, though. M +1 -1 src/runtime/kwalletd/main.cpp M +34 -8 src/runtime/kwalletd/migrationagent.cpp M +3 -2 src/runtime/kwalletd/migrationagent.h M +1 -1 src/runtime/kwalletd/migrationwizard.cpp http://commits.kde.org/kwallet-framework/127efedd1668b546d0ac8c83655a2056d0439f29 This should now work as expected. However, could you please test and return here if further work should be done. Seems to work splendidly. Thanks Valentin! |