GNOME Bugzilla – Bug 695535
region: Allow setting system-wide formats
Last modified: 2013-10-15 21:20:09 UTC
The first patch is just a small cleanup.
Created attachment 238494 [details] [review] region: Remove useless parameter locald settings are system settings by definition so we can just use priv->system_language directly.
Created attachment 238495 [details] [review] region: Allow setting system-wide formats There's really no reason not to do it.
Review of attachment 238494 [details] [review]: sure, why not
Review of attachment 238495 [details] [review]: Should probably keep this new feature master-only ::: panels/region/cc-region-panel.c @@ +505,3 @@ + name = gnome_get_country_from_locale (region, region); + else + name = g_strdup (C_("Region", "None")); Just like all the other places, I don't think we want to show 'None' here. If there's no value, show the system language here, since it will apply to all locale facets.
Review of attachment 238494 [details] [review]: s/locald/localed/
Review of attachment 238495 [details] [review]: ::: panels/region/cc-region-panel.c @@ +348,3 @@ gtk_widget_destroy (GTK_WIDGET (chooser)); if (changed) That's the retval of update_region(). Given that you return FALSE if the notification shouldn't be shown (rather than if it's not changed), please change the variable name.
Comment on attachment 238494 [details] [review] region: Remove useless parameter Attachment 238494 [details] pushed as 0526ab4 - region: Remove useless parameter
Created attachment 255695 [details] [review] region: Allow setting system-wide formats -- Seems like I forgot about this. Updated to master.
Created attachment 255696 [details] [review] region: Never show "None" for language and formats If the settings backends fail to give us valid values, show the current environment instead of "None" since we do know which language is actually being used.
Review of attachment 255696 [details] [review]: This looks fine, but I'd really like us to have a regression test suite for that sort of thing. We seem to be fixing the same (or similar enough) bugs over and over.
(In reply to comment #10) > This looks fine, but I'd really like us to have a regression test suite for > that sort of thing. We seem to be fixing the same (or similar enough) bugs over > and over. We did fix something like this for the input sources list but the implementation is necessarily different. There is the user's region (formats) empty setting problem which this patch touches to add the comment since I was already changing the surrounding code to handle gnome_get_country_from_locale() returning NULL (in case what we feed it is garbage). But that issue was fixed already, I just added the comment to make the logic clearer. What about the other (main) patch here?
Review of attachment 255695 [details] [review]: That looks fine as well, even though I have a hard time following the logic (before and after fwiw).
Attachment 255695 [details] pushed as 9e975b0 - region: Allow setting system-wide formats Attachment 255696 [details] pushed as 455b457 - region: Never show "None" for language and formats