Redesign and refactoring of installation dialogs #84
Conversation
- WIP: encryption method was removed from the dialogs and has not be added elsewhere yet - Three options are now presented (new, import and skip) - Code moved to lib/users/dialogs reusing dialogs infrastructure when possible - Fixed the rpm to build correctly - Still a lot of old code, this is not intended as a full rewrite
|
|
||
| module Yast | ||
| class InstUserFirstDialog | ||
| include ::UI::EventDispatcher |
There was a problem hiding this comment.
why not use whole Dialog base class? https://github.com/yast/yast-yast2/blob/master/library/general/src/lib/ui/dialog.rb
There was a problem hiding this comment.
at least document why it cannot be used
There was a problem hiding this comment.
Dialog class assumes a non-wizard dialog (it calls to UI.CloseDialog, for example). Using it in this context means overriding almost the whole class (only the includes are really useful). We could, of course, also thing about modifying Dialog to play more nicely with installation wizards.
There was a problem hiding this comment.
thanks for answer, so plase at least document it in code why it is not used. Dialog can be of course also adapted to work nicely in wizard dialogs
There was a problem hiding this comment.
Expect a pull request today adding a InstallationDialog class to yast2 (I have already needed it twice).
There was a problem hiding this comment.
Please do not call it InstallationDialog as wizard is used e.g. also in bootloader which is not only installation dialog. Better WizardDialog?
There was a problem hiding this comment.
In its current form, it is really InstallationDialog, since it uses Yast::GetInstArgs, makes accept_handler kind of an alias for next_handler and that kind of installation specific stuff.
There was a problem hiding this comment.
@jreidinger Here you are the premiere yast/yast-yast2#427 I'll add tests tomorrow morning.
| ) | ||
| end | ||
| deep_copy(@ret) | ||
| ret |
There was a problem hiding this comment.
if you touch this file I really recommend to use ProposalClient as base class - https://github.com/yast/yast-yast2/blob/master/library/general/src/lib/installation/proposal_client.rb
ancorgs
changed the title
|
hidden value in else is medium serious for me. another one is nitpick. so almost LGTM 😉 |
|
I have one little nitpicking: The dialog title is "Local Users" even if there is nothing to import and thus you can create just one "Local User". So, either I'd like to have it different dependent on this fact or just "Local User" (one). |
|
for me LGTM |
|
LGTM, thanks for the changes! |
Redesign and refactoring of installation dialogs
This pull request tries to address the usability problems reported for the "create user" screen during installation.
Problems of the previous dialog for user creation
Here you can see the previous screen.
There is quite some functionality hidden there:
Apart from the usability problems, the source code was in really bad shape. Poorly structured, with no unit tests, with a lot of duplication and dead code and with some bugs.
The new dialog
The normal installation workflow have now 2 steps:
Creation of user. This is how it looks in a clean system and in a system with previous users
Root's password. As usual, this step is skipped if "use this password for system administrator" is checked. It looks like always (not worth an additional screenshot).
That's it. No more hidden functionality.
But you could be wondering where the encryption method selector ended up. That depends on the content of the proposal section of the control.xml of the distribution.
Option a) Nothing (current) - The option is simply not there anymore
Option b) users proposal - The installation summary looks like this (check "users settings" section)
Option c) users_encryption proposal - Summary looks like this (check the "password encryption type" section)
With both options b and c, clicking on the name of the method will take the user to this screen
Code refactoring
As important as the visual and workflow redesign it the code refactoring, which adds object orientation and some unit tests. As a side effect of that refactoring, the
InstallerDialogclass dialog was added to yast2.rpm for further reuse all along YaST in the future. See yast/yast-yast2#427 for details