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 _______________________________________________ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev