After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695535 - region: Allow setting system-wide formats
region: Allow setting system-wide formats
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Region & Language
unspecified
Other All
: Normal enhancement
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
3.10
Depends on:
Blocks:
 
 
Reported: 2013-03-10 03:14 UTC by Rui Matos
Modified: 2013-10-15 21:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
region: Remove useless parameter (1.92 KB, patch)
2013-03-10 03:14 UTC, Rui Matos
committed Details | Review
region: Allow setting system-wide formats (6.13 KB, patch)
2013-03-10 03:14 UTC, Rui Matos
reviewed Details | Review
region: Allow setting system-wide formats (5.50 KB, patch)
2013-09-25 14:57 UTC, Rui Matos
committed Details | Review
region: Never show "None" for language and formats (3.41 KB, patch)
2013-09-25 14:57 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2013-03-10 03:14:35 UTC
The first patch is just a small cleanup.
Comment 1 Rui Matos 2013-03-10 03:14:38 UTC
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.
Comment 2 Rui Matos 2013-03-10 03:14:42 UTC
Created attachment 238495 [details] [review]
region: Allow setting system-wide formats

There's really no reason not to do it.
Comment 3 Matthias Clasen 2013-03-11 03:21:57 UTC
Review of attachment 238494 [details] [review]:

sure, why not
Comment 4 Matthias Clasen 2013-03-11 03:25:59 UTC
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.
Comment 5 Bastien Nocera 2013-03-11 09:37:57 UTC
Review of attachment 238494 [details] [review]:

s/locald/localed/
Comment 6 Bastien Nocera 2013-03-11 09:41:01 UTC
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 7 Rui Matos 2013-03-14 16:31:04 UTC
Comment on attachment 238494 [details] [review]
region: Remove useless parameter

Attachment 238494 [details] pushed as 0526ab4 - region: Remove useless parameter
Comment 8 Rui Matos 2013-09-25 14:57:18 UTC
Created attachment 255695 [details] [review]
region: Allow setting system-wide formats

--

Seems like I forgot about this. Updated to master.
Comment 9 Rui Matos 2013-09-25 14:57:31 UTC
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.
Comment 10 Bastien Nocera 2013-09-25 15:54:40 UTC
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.
Comment 11 Rui Matos 2013-09-25 16:36:00 UTC
(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?
Comment 12 Bastien Nocera 2013-09-25 16:44:55 UTC
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).
Comment 13 Rui Matos 2013-10-15 21:20:01 UTC
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