Hi,
Alan wrote (25 Sep 2013 15:12:34 GMT) :
> I believe the feature/liveusb_ui_improvement is now ready to merge.
Great work! Here we go.
Minor process note: next time, please reassign to the current RM when
asking for a merge. Thanks in advance.
As a bonus, it seems that you've implemented #6258 (Mention that the
persistent volume will be preserved while upgrading). Great!
The phrasing you chose is:
Your persistent volume will be preserved. Continue?
It seems a bit less clear to me (for users who have no persistent volume)
than the first phrasing proposed on #6258, that is:
Any persistent volume on this stick will remain unchanged.
So I've implemented this other phrasing in the master branch.
I've replaced "stick" with "device", to be consistent with the
terminology of the rest of the app, and we may very well be installing
onto a SD card, hard drive or what not. Any better proposal, anyone?
Also, this does not look correct English to me:
You are going to upgrade Tails on the device %s %s.
So I've changed it to:
You are going to upgrade Tails on the %s %s device.
... and even to:
You are going to upgrade Tails on the %(vendor)s %(model)s device.
... to ease translators' work, in particular for RTL languages.
Just curious: any specific reason that I've missed to display the
device size in the confirmation dialog on initial install, but not at
upgrade time?
> - self.status(_("Warning: All data on the selected drive will "
> - "be lost."))
Ouch. I strongly disagree with dropping this information, when we're
going to partition the destination storage device, so I've
implemented:
_("You are going to install Tails on the %(vendor)s %(model)s device (%(size)s). "
"All data on the selected drive will be lost. "
"Continue?")
I've changed:
_("You are going to install Tails on the device %s %s (%s). Continue?")
to
_("You are going to install Tails on the %(vendor)s %(model)s device (%(size)s). Continue?")
for the same reason as the upgrade message.
> +def _format_bytes(value):
> + return '%0.1f GB' % (value / 10.0**9)
I appreciate the minor refactoring, but then I'd rather:
* either use some library to display human-readable units in all
cases (think: 2TB external drive) to improve UX
* or simply rename the function to _format_bytes_in_GB, so that at
least the name expresses its current limitations.
Anyway, this is certainly not blocking, so I did not do it myself.
> + pretty_name = "%s %s (%s)" % (info['vendor'], info['model'], details)
This isn't l18n'd and can't work for RTL languages. Changing to:
pretty_name = _("%(vendor)s %(model)s (%(details)s)")
% {'vendor': info['vendor'],
'model': info['model'],
'details': details }
I do realize there is similar code in the same are that isn't
l18n-ready, and some of the code you're replacing wasn't either, but
still, when introducing new strings, let's make sure they can be
translated :)
OK, done with the code review, let's switch to testing.
First of all, I like the new confirmation dialog a lot. I generally
dislike such UI components, but in this case, I do think it makes
sense (at least until we revamp the installer UI entirely, I guess).
However, the No/Yes buttons ordering seems unusual to me. I didn't
dare patching it, as I thought you might have followed the GNOME HIG
or something, and I may be missing something. Any reason to order it
this way?
The device choice menu doesn't fit, and has an ugly "go down" button
when I:
1. start the GUI with only one possible device plugged in
2. plug a second device
3. open the menu
It's not as painful to use as the greeter language menu, but still.
I can't reproduce this is the target device is plugged before I start
the installer. Can you reproduce this?
To end with, the test suite was not updated yet, and is likely broken
by these changes, so please file a ticket about it, assigned to me if
you wish (I'd rather do it together, though). This must be fixed
before the 0.21 freeze.
Now going to test my commits before I push them :)
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
