GNOME Bugzilla – Bug 770593
info: Split up "Base system" and "OS type" to separate lines
Last modified: 2017-05-01 15:44:53 UTC
We've been carrying http://pkgs.fedoraproject.org/cgit/rpms/control-center.git/tree/distro-logo.patch downstream in Fedora for a while and I'd like to upstream portions of that patch in order to reduce the delta in the downstream patch. After our #gnome-design discussion with aday and hadess in the morning today, I think the following three patches might be good for upstream. Here's before and after screenshots: Before: https://kalev.fedorapeople.org/gnome-control-center-details1.png After: https://kalev.fedorapeople.org/gnome-control-center-details2.png
Created attachment 334431 [details] [review] info: Improve fallbacks if PRETTY_NAME isn't set Try to use NAME + VERSION_ID and fall back to "Unknown" if that didn't work either.
Created attachment 334432 [details] [review] info: Split up "Base system" and "OS type" to separate lines ... as per #gnome-design discussion.
Created attachment 334433 [details] [review] info: Use semicolon for separating build ID Otherwise we can end up having "Fedora 25 (Workstation Edition) (Build ID: asdf)", which looks awkward with multiple parenthesis.
Created attachment 334437 [details] [review] info: Rename "base system" to "OS name" ... as per aday's suggestions on #gnome-design.
This is how it looks now: https://kalev.fedorapeople.org/gnome-control-center-details4.png
Works for me!
Review of attachment 334431 [details] [review]: Looks good.
Review of attachment 334432 [details] [review]: > ... as per #gnome-design discussion. I'd rather have the explanation in-line in the commit message itself. That's also a very short label to add, which seems a bit weird to me. Maybe that's better integrated into the Processor line? ::: panels/info/cc-info-panel.c @@ +508,3 @@ + + /* translators: This is the type of architecture for the OS */ + return g_strdup_printf (_("%d-bit"), bits); Needs context I think.
Review of attachment 334433 [details] [review]: Sure.
Review of attachment 334437 [details] [review]: Sure.
Created attachment 350547 [details] [review] info: Improve fallbacks if PRETTY_NAME isn't set Rebase against master.
Created attachment 350548 [details] [review] info: Split up "Base system" and "OS type" to separate lines Rebase against master. Since I lost it, I can't really improve the commit message with the contents of that discussion. I also removed an unnecessary translator comment.
Created attachment 350549 [details] [review] info: Use semicolon for separating build ID Rebase against master.
Created attachment 350550 [details] [review] info: Rename "base system" to "OS name" Rebase against master. Again, lost the discussion.
Review of attachment 350548 [details] [review]: As per comment 8. ::: panels/info/cc-info-overview-panel.c @@ +497,3 @@ + + /* translators: This is the type of architecture for the OS */ + return g_strdup_printf (C_("Processor architecture", "%d-bit"), bits); I think this might need a g_dngettext() call combined with NC_(). Best ask i18n. Can you back out this change (the addition of C_()) and add it in a separate patch? I would also simplify the "bits" calculation ("bits = GLIB_SIZEOF_VOID_P * 8") in a separate patch.
Review of attachment 350550 [details] [review]: This one would also need a reasoning. References to an IRC channel aren't really future-proof.
Created attachment 350683 [details] [review] info: Split up "Base system" and "OS type" to separate lines Having the OS name and architecture at the same place might prove to be hard to understand that it's the processor arch. Fix that by splitting the OS name and type labels in different rows.
(In reply to Bastien Nocera from comment #15) > Review of attachment 350548 [details] [review] [review]: > ::: panels/info/cc-info-overview-panel.c > @@ +497,3 @@ > + > + /* translators: This is the type of architecture for the OS */ > + return g_strdup_printf (C_("Processor architecture", "%d-bit"), bits); > > I think this might need a g_dngettext() call combined with NC_(). Best ask > i18n. > IMHO the simplest solution from the translators’ point of view would be to provide two separate translatable strings: “32-bit” and “64-bit”. I don’t expect 128-bit systems any time soon, and 16-bit GNOME is probably also out of the question. :)
(In reply to Piotr Drąg from comment #18) > (In reply to Bastien Nocera from comment #15) > > Review of attachment 350548 [details] [review] [review] [review]: > > ::: panels/info/cc-info-overview-panel.c > > @@ +497,3 @@ > > + > > + /* translators: This is the type of architecture for the OS */ > > + return g_strdup_printf (C_("Processor architecture", "%d-bit"), bits); > > > > I think this might need a g_dngettext() call combined with NC_(). Best ask > > i18n. > > > > IMHO the simplest solution from the translators’ point of view would be to > provide two separate translatable strings: “32-bit” and “64-bit”. I don’t > expect 128-bit systems any time soon, and 16-bit GNOME is probably also out > of the question. :) Right, that would do :)
Created attachment 350697 [details] [review] info: Rename "base system" to "OS name" Using "OS name" is clearer than "Base system" when we want to check what's the current OS name. Fix that by renaming the label.
Created attachment 350698 [details] [review] info: Improve translation of OS type Instead of having a single translation using a printf-formatted string, hardcode the 32- and 64-bit variants of the OS type to avoid any translation problems.
Created attachment 350699 [details] [review] info: Split up "Base system" and "OS type" to separate lines Actually improve the commit message.
Review of attachment 350697 [details] [review]: > Using "OS name" is clearer than "Base system" when we > want to check what's the current OS name. That seems like a self-fulfilling explanation. I'd say: Using "OS name" is clearer than "Base system" when we want to check what the current OS/distribution combination is.
Review of attachment 350698 [details] [review]: Looks good.
Review of attachment 350699 [details] [review]: If this goes before the 2 I just reviewed, it looks fine. > to be harder [...] that harder _than_
Thanks for the reviews, Bastien. The commit messages were fixed per your suggestions. Attachment 350547 [details] pushed as eeb5594 - info: Improve fallbacks if PRETTY_NAME isn't set Attachment 350549 [details] pushed as fb1291b - info: Use semicolon for separating build ID Attachment 350697 [details] pushed as 5cc33d6 - info: Rename "base system" to "OS name" Attachment 350698 [details] pushed as 5a01034 - info: Improve translation of OS type Attachment 350699 [details] pushed as 439548b - info: Split up "Base system" and "OS type" to separate lines