Rui, if you want to take a look at the branch, that would be great.

The "Login Screen" toggle is just weird. I know the pattern is already used in the mouse panel, but it's still bizarre.
I also wonder how the shell gets changed to "restart session" unless we implement the same logic in the shell that we do in the panel.

I've pushed a more or less complete implementation now.
It includes the restart notification, the login screen page,
and talks to localed.

Testing and review welcome

Created attachment 234818 [details] [review]
squashed patch from wip/region-panel

Review of attachment 234818 [details] [review]:

Other functional comments:
- I get criticals from gnome-desktop on startup:
(gnome-control-center:3583): GnomeDesktop-CRITICAL **: gnome_get_region_from_name: assertion `*name != '\0'' failed
- the "popular" languages are using the translation from iso-codes, instead of the short names we used before (should say "Francais" instead of "Francais (France)", and "English" instead of "English (United States)" for example)
- the "Done" button should be a Cancel/Done combination. I was able to click it without selecting a new language
- the notification popup isn't in the "future" language
- the notification popup covers the language selection
- the notification popup doesn't persist across restarts
- the notification popup doesn't take the current session's locale into account (if I switch from English to French and then back to English, it will tell me to restart)
- the popular languages are at the end of the list when clicking "...", and they're in their own language
- my current language is "English (Great Britain)" and search "French" doesn't show any language, searching for "France" works
- searching for "francais" should show "Français" in the list. See cc_util_normalize_casefold_and_unaccent() in:
http://git.gnome.org/browse/gnome-control-center/tree/shell/cc-util.c
- An empty locale (gsettings get org.gnome.system.locale region returns '') means no selection in the formats dialogue
- The window will grow and grow as you add more input sources, without any limits
- I can only see the "Options" button once I add an IBus input source, Russian users aren't going to be happy
- Clicking on the cogwheel in the input source list doesn't do anything
- Clicking on the keyboard button for Amharic (m17n) shows the "English (US)" keyboard layout. Even if it's the layout it uses for XKB, it's confusing

There seems to be plenty of regressions from the bugs I fixed in the 3.7 timeframe.

::: panels/common/cc-common-language.c
@@ +716,3 @@
+                      gpointer user_data)
+{
+        GtkListStore *store = (GtkListStore *) user_data;

You don't need to cast from gpointer.

::: panels/region/cc-format-chooser.c
@@ +101,3 @@
+        g_free (locale);
+
+#if 0

How come?

::: panels/region/cc-ibus-utils.c
@@ +34,3 @@
+        language_code = ibus_engine_desc_get_language (engine_desc);
+        language = ibus_get_language_name (language_code);
+        display_name = g_strdup_printf ("%s (%s)", language, name);

This leads to names like "Amharic (sera (m17n))" in the list.

(In reply to comment #5)

> - the "Done" button should be a Cancel/Done combination. I was able to click it
> without selecting a new language

Whats wrong with being able to click it before changing the selection ? You were done, after all. Anyway up to Allan.

> - the notification popup covers the language selection

Hard to avoid. Do you think its a problem ?

> - the notification popup doesn't persist across restarts

Should it ? I thought that is what the (unimplemented) shell menu part is supposed to address.

> - The window will grow and grow as you add more input sources, without any
> limits

Having more than 5 is very unlikely. But sure, we can do the same thing we do for wired profiles - put it in a non-scrolling scrolledwindow, and turn on vertical scrollbar when we go over that number.

> - I can only see the "Options" button once I add an IBus input source, Russian
> users aren't going to be happy

Really ? It is showing up fine with two xkb layouts here. 

> - Clicking on the cogwheel in the input source list doesn't do anything

It runs the preferences for the selected ibus engine here. Any errors ?

> - Clicking on the keyboard button for Amharic (m17n) shows the "English (US)"
> keyboard layout. Even if it's the layout it uses for XKB, it's confusing

I haven't looked in detail, but Allan had a mockup for the layout dialog too - does that address this point ?


> ::: panels/region/cc-format-chooser.c
> @@ +101,3 @@
> +        g_free (locale);
> +
> +#if 0
> 
> How come?

Not in the mockup. I wanted to ask Allan if he took it out intentionally. I don't think it is very useful to show that information, fwiw.

(In reply to comment #6)
> (In reply to comment #5)
> 
> > - the "Done" button should be a Cancel/Done combination. I was able to click it
> > without selecting a new language
> 
> Whats wrong with being able to click it before changing the selection ? You
> were done, after all. Anyway up to Allan.

No biggie.

> > - the notification popup covers the language selection
> 
> Hard to avoid. Do you think its a problem ?

I think it is, but Allan can comment.

> > - the notification popup doesn't persist across restarts
> 
> Should it ? I thought that is what the (unimplemented) shell menu part is
> supposed to address.

It should. And reverting the language to the one originally used when the session was launched shouldn't trigger a notification.

Also the use of the word "Session" is too techy. I don't think that word should be used in UI. The old code said "Log out for changes to take effect".

> > - The window will grow and grow as you add more input sources, without any
> > limits
> 
> Having more than 5 is very unlikely. But sure, we can do the same thing we do
> for wired profiles - put it in a non-scrolling scrolledwindow, and turn on
> vertical scrollbar when we go over that number.

That would be good.

> > - I can only see the "Options" button once I add an IBus input source, Russian
> > users aren't going to be happy
> 
> Really ? It is showing up fine with two xkb layouts here. 

$ gsettings get org.gnome.desktop.input-sources sources
[('xkb', 'gb'), ('xkb', 'us')]

The options window is hidden.

> > - Clicking on the cogwheel in the input source list doesn't do anything
> 
> It runs the preferences for the selected ibus engine here. Any errors ?

Not the one in the toolbar, the one in the list. I was expected it to launch the preferences.

> > - Clicking on the keyboard button for Amharic (m17n) shows the "English (US)"
> > keyboard layout. Even if it's the layout it uses for XKB, it's confusing
> 
> I haven't looked in detail, but Allan had a mockup for the layout dialog too -
> does that address this point ?

This one?
https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/region-and-language/png/keyboard-layout-viewer.png
Not really. Amharic *does* use "English US" as the keyboard layout. Just having the title be changed to something that made it clear that the layout is the correct one would be good.

> 
> > ::: panels/region/cc-format-chooser.c
> > @@ +101,3 @@
> > +        g_free (locale);
> > +
> > +#if 0
> > 
> > How come?
> 
> Not in the mockup. I wanted to ask Allan if he took it out intentionally. I
> don't think it is very useful to show that information, fwiw.

OK.

(In reply to comment #7)

> > > - Clicking on the cogwheel in the input source list doesn't do anything
> > 
> > It runs the preferences for the selected ibus engine here. Any errors ?
> 
> Not the one in the toolbar, the one in the list. I was expected it to launch
> the preferences.

Oh, thats just an indicator icon. Thats how I interpreted the mockup. I think we may want to make the rows activatable and just launch the preferences / layout viewer.

Trying to summarize the current design questions:

1. The "Formats" item doesn't exist in the Login Screen. Any
particular reason? It's very easy to implement and I think it would
actually make the "transition" between the 2 modes smoother since, as
is, we lose/get that row and thus the whole input sources section
moves up/down;

2. The cogwheel icon to identify IME sources looks a bit strange. On
#control-center we chatted a bit about it and wondered why don't we
turn these icons into actual buttons that launch the IME settings
dialogs and remove the bottom toolbar one;

3. The bottom toolbar doesn't have the up/down buttons anymore, how
does re-ordering the list work? Or we don't want/need that?

4. Are the bottom right toolbar buttons (cogwheel and keyboard) really
supposed to appear/disappear? Appear only when one row is selected?

5. The "session needs to be restarted" info bar comes out on top of
the Language item (the 1st row) covering it almost completely, is that
ok?

6. Should there be a link to the keyboard shortcuts as before in the
input source options dialog?

7. On the "Add an Input Source" dialog, do we want to fold/unfold the actual sources beneath each language entry in a tree-like fashion or should we "transition" into another list when the user clicks a language item? i.e. have a 2 levels, the 1st being language rows and for each one having a 2nd list with the actual sources - how would the user go back in this case? a back button somewhere?

(In reply to comment #5)
> - the "popular" languages are using the translation from iso-codes, instead of
> the short names we used before (should say "Francais" instead of "Francais
> (France)", and "English" instead of "English (United States)" for example)

Should it really just say "English" instead of "English (United States)"? Which locale should we choose for "English"? AFAIK, there's no "en" locale, we have to explicitly choose "en_US", "en_GB" or one of the other "en_*".

(In reply to comment #9)
> Trying to summarize the current design questions:
> 
> 1. The "Formats" item doesn't exist in the Login Screen. Any
> particular reason? It's very easy to implement and I think it would
> actually make the "transition" between the 2 modes smoother since, as
> is, we lose/get that row and thus the whole input sources section
> moves up/down;

Is formats used in the login screen though?

> 2. The cogwheel icon to identify IME sources looks a bit strange. On
> #control-center we chatted a bit about it and wondered why don't we
> turn these icons into actual buttons that launch the IME settings
> dialogs and remove the bottom toolbar one;

Except you'd still need a way to identify an IME in the add input source dialog.

> 3. The bottom toolbar doesn't have the up/down buttons anymore, how
> does re-ordering the list work? Or we don't want/need that?

There weren't up/down buttons in the previous mockups either - the idea was that we'd do away with ordering the list, since the purpose of reordering isn't clear, and it would hopefully be obsoleted by alt-tab style switching.

> 4. Are the bottom right toolbar buttons (cogwheel and keyboard) really
> supposed to appear/disappear? Appear only when one row is selected?

Hmm, I think that keyboard should always be shown (but be insensitive when a row isn't selected). cogwheel should only be shown when an IME is selected (ie. it should never be insensitive). See bug 692006 for this. I'll update the mockups here.

> 5. The "session needs to be restarted" info bar comes out on top of
> the Language item (the 1st row) covering it almost completely, is that
> ok?

That's how in app notifications currently work. It's not perfect, but I don't think it's terrible - it can be dismissed, after all.

> 6. Should there be a link to the keyboard shortcuts as before in the
> input source options dialog?

It seemed wrong to have a link going from a dialog to another settings panel. That is one disadvantage of this design.

> 7. On the "Add an Input Source" dialog, do we want to fold/unfold the actual
> sources beneath each language entry in a tree-like fashion or should we
> "transition" into another list when the user clicks a language item? i.e. have
> a 2 levels, the 1st being language rows and for each one having a 2nd list with
> the actual sources - how would the user go back in this case? a back button
> somewhere?

I've tried both approaches at various times. I'm honestly not sure, but we don't seem to have a good place for a back button right now. Maybe we could get a second opinion on this.

It should be said that these mockups were very much a first attempt. I'm still not sure about the session restart and login screen parts.

(In reply to comment #11)
> Is formats used in the login screen though?

At least the top panel clock format depends on that setting.

> > 2. The cogwheel icon to identify IME sources looks a bit strange. On
> > #control-center we chatted a bit about it and wondered why don't we
> > turn these icons into actual buttons that launch the IME settings
> > dialogs and remove the bottom toolbar one;
> 
> Except you'd still need a way to identify an IME in the add input source
> dialog.

Indeed. So we'll go ahead with it for now. But maybe we should get a new icon for this going forward?

> There weren't up/down buttons in the previous mockups either - the idea was
> that we'd do away with ordering the list, since the purpose of reordering isn't
> clear, and it would hopefully be obsoleted by alt-tab style switching.

Ok, we have the alt-tab style switching now so it should be fine.

> Hmm, I think that keyboard should always be shown (but be insensitive when a
> row isn't selected). cogwheel should only be shown when an IME is selected (ie.
> it should never be insensitive). See bug 692006 for this. I'll update the
> mockups here.

Sounds good.

> That's how in app notifications currently work. It's not perfect, but I don't
> think it's terrible - it can be dismissed, after all.

Agree.

> It seemed wrong to have a link going from a dialog to another settings panel.
> That is one disadvantage of this design.

Ok, let's keep it as is then.

> I've tried both approaches at various times. I'm honestly not sure, but we
> don't seem to have a good place for a back button right now. Maybe we could get
> a second opinion on this.

Ok, I'd really like to finish this dialog though. I'm currently doing the 2 level thing but we can still change it. I was going to cram a back button in the bottom left fwiw...

Related to that is how the filtering works. When we're filtering should we completely hide the "non-leaf" rows, i.e. the ones that don't actually represent an input source like the "Czech" one in the mockup? Or should we show them if one of the sub-rows matches the filter? Something else?

Created attachment 236028 [details] [review]
squashed patch from wip/region-panel

Ok, me and Allan solved the design questions on IRC. Note that the mockups linked from comment 0 have been updated.

Here's the big patch with all the commits in http://git.gnome.org/browse/gnome-control-center/log?h=wip%2Fregion-panel . It might be better to look at the commits there while reviewing to make more sense out of this.

Review of attachment 236028 [details] [review]:

::: panels/common/cc-common-language.c
@@ +626,3 @@
         ht = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
 
+        insert_language (ht, "en_US");

As mentioned earlier, this loses the "simplified name for common languages" that we used to have.

::: panels/common/cc-language-chooser.c
@@ +69,3 @@
+                        continue;
+
+                if (strcmp (locale_id, language) == 0) {

You're not guarding against locale_id == NULL.

@@ +147,3 @@
+        g_object_set (check, "icon-size", GTK_ICON_SIZE_MENU, NULL);
+        gtk_box_pack_start (GTK_BOX (widget), check, FALSE, FALSE, 0);
+        if (g_strcmp0 (locale_id, current_locale_id) == 0) {

One-liner, so no braces.

@@ +348,3 @@
+        if (child == NULL)
+                return;
+        else if (child == priv->no_results)

Drop the "else" in this case, as you're returning early in the first case.

@@ +350,3 @@
+        else if (child == priv->no_results)
+                return;
+        else if (child == priv->more_item)

Ditto. And reverse add a return to drop the multi-line "else" afterwards.

@@ +414,2 @@
         builder = gtk_builder_new ();
+        gtk_builder_add_from_resource (builder, "/org/gnome/control-center/common/language-chooser.ui", &error);

if (gtk_builder_add_from_resource () == 0) { error path }

::: panels/common/cc-language-chooser.h
@@ +18,3 @@
  *
+ * Written by:
+ *     Matthias Clasen

Remove that change, it's not needed.

@@ +26,2 @@
 #include <gtk/gtk.h>
+#include <glib-object.h>

That's not necessary, the gtk.h include should already drag it in.

::: panels/region/cc-input-chooser.c
@@ +18,3 @@
+ */
+
+#define _GNU_SOURCE

Why is _GNU_SOURCE necessary?

@@ +35,3 @@
+#include <ibus.h>
+#include "cc-ibus-utils.h"
+#endif

Add /* HAVE_IBUS */ after the #endif

@@ +361,3 @@
+    initial = cc_common_language_get_initial_languages ();
+
+  g_hash_table_iter_init (&iter, priv->locales);

Why not use g_hash_table_get_values() instead?

@@ +575,3 @@
+      return TRUE;
+
+  if (!*a && !*b)

*a == NULL && *b == NULL
is clearer to me.

@@ +885,3 @@
+    }
+}
+#endif

HAVE_IBUS.

::: panels/region/cc-input-options.c
@@ +78,3 @@
+{
+  const gchar *value;
+  const gchar *description;

I would prefer if we used gtk_accelerator_get_label()  to generate the translated strings. It would avoid unnecessary translation for translators.

::: panels/region/cc-region-panel.c
@@ +552,3 @@
+        list = ibus_bus_list_engines_async_finish (priv->ibus, result, &error);
+        g_clear_object (&priv->ibus_cancellable);
+        if (!list && error) {

Can list be NULL without an error?

@@ +1497,3 @@
+
+#if 0
+        /* Not convinced this is a good idea even if the sole user is

Please split out in a separate bug.

(In reply to comment #14)
> ::: panels/common/cc-common-language.c
> @@ +626,3 @@
>          ht = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
> 
> +        insert_language (ht, "en_US");
> 
> As mentioned earlier, this loses the "simplified name for common languages"
> that we used to have.

To be honest I don't like that because it becomes inconsistent as soon as the user clicks to see the full list.

> ::: panels/common/cc-language-chooser.h
> @@ +18,3 @@
>   *
> + * Written by:
> + *     Matthias Clasen
> 
> Remove that change, it's not needed.

That's Matthias' call.

> @@ +361,3 @@
> +    initial = cc_common_language_get_initial_languages ();
> +
> +  g_hash_table_iter_init (&iter, priv->locales);
> 
> Why not use g_hash_table_get_values() instead?

Because it returns an allocated list that we have no use for.

> ::: panels/region/cc-input-options.c
> @@ +78,3 @@
> +{
> +  const gchar *value;
> +  const gchar *description;
> 
> I would prefer if we used gtk_accelerator_get_label()  to generate the
> translated strings. It would avoid unnecessary translation for translators.

We can't because gtk accelerators don't support modifier only combinations.

> ::: panels/region/cc-region-panel.c
> @@ +552,3 @@
> +        list = ibus_bus_list_engines_async_finish (priv->ibus, result,
> &error);
> +        g_clear_object (&priv->ibus_cancellable);
> +        if (!list && error) {
> 
> Can list be NULL without an error?

Yes, it would be an empty list meaning that ibus doesn't have any engines available. (Didn't actually test if ibus-daemon even runs in that case though)

> @@ +1497,3 @@
> +
> +#if 0
> +        /* Not convinced this is a good idea even if the sole user is
> 
> Please split out in a separate bug.

I've removed it now, but I don't think it needs a new bug, after all this is the "implement the new design" bug.

I think I've addressed all your other comments in the new patch.

Created attachment 236426 [details] [review]
squashed patch from wip/region-panel

(In reply to comment #15)
> (In reply to comment #14)
> > ::: panels/common/cc-common-language.c
> > @@ +626,3 @@
> >          ht = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
> > 
> > +        insert_language (ht, "en_US");
> > 
> > As mentioned earlier, this loses the "simplified name for common languages"
> > that we used to have.
> 
> To be honest I don't like that because it becomes inconsistent as soon as the
> user clicks to see the full list.

There seems to me 

> > ::: panels/common/cc-language-chooser.h
> > @@ +18,3 @@
> >   *
> > + * Written by:
> > + *     Matthias Clasen
> > 
> > Remove that change, it's not needed.
> 
> That's Matthias' call.

No, you're modifying the indentation of those lines for no reason.

> > ::: panels/region/cc-input-options.c
> > @@ +78,3 @@
> > +{
> > +  const gchar *value;
> > +  const gchar *description;
> > 
> > I would prefer if we used gtk_accelerator_get_label()  to generate the
> > translated strings. It would avoid unnecessary translation for translators.
> 
> We can't because gtk accelerators don't support modifier only combinations.

Nope, it works fine:
#define INVALID_CHAR GDK_KEY_VoidSymbol - 1
res = gtk_accelerator_get_label (INVALID_CHAR, GDK_SHIFT_MASK | GDK_CONTROL_MASK);

And use "res". See also https://bugzilla.gnome.org/show_bug.cgi?id=694075

> > @@ +1497,3 @@
> > +
> > +#if 0
> > +        /* Not convinced this is a good idea even if the sole user is
> > 
> > Please split out in a separate bug.
> 
> I've removed it now, but I don't think it needs a new bug, after all this is
> the "implement the new design" bug.

OK, file it as a separate patch here then, and we'll ask the designers about it.

> I think I've addressed all your other comments in the new patch.

Just the 2 things above.

(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > ::: panels/common/cc-common-language.c
> > > @@ +626,3 @@
> > >          ht = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
> > > 
> > > +        insert_language (ht, "en_US");
> > > 
> > > As mentioned earlier, this loses the "simplified name for common languages"
> > > that we used to have.
> > 
> > To be honest I don't like that because it becomes inconsistent as soon as the
> > user clicks to see the full list.
> 
> There seems to me 

I should finish my comments before going on to the next one.

We have 2 different lists, one with the most popular languages, and one with everything. I think that those don't have to use the same label. For one thing, once you click on "..." those items to be sorted alphabetically in the list, instead of separate. I think it's pretty clear that they're different lists to the user, so I don't think that having different labels is much of a problem (and in this case it's clearer, especially if we're going to reuse this for g-i-s).

I can let it pass if you could open up a new bug about this to ask for designer input.

(In reply to comment #17)
> > > ::: panels/common/cc-language-chooser.h
> > > @@ +18,3 @@
> > >   *
> > > + * Written by:
> > > + *     Matthias Clasen
> > > 
> > > Remove that change, it's not needed.
> > 
> > That's Matthias' call.
> 
> No, you're modifying the indentation of those lines for no reason.

Ah, didn't notice that from comment. Alright, I amended that commit to not fiddle with the license header besides changing the copyright year.

> > > ::: panels/region/cc-input-options.c
> > > @@ +78,3 @@
> > > +{
> > > +  const gchar *value;
> > > +  const gchar *description;
> > > 
> > > I would prefer if we used gtk_accelerator_get_label()  to generate the
> > > translated strings. It would avoid unnecessary translation for translators.
> > 
> > We can't because gtk accelerators don't support modifier only combinations.
> 
> Nope, it works fine:
> #define INVALID_CHAR GDK_KEY_VoidSymbol - 1
> res = gtk_accelerator_get_label (INVALID_CHAR, GDK_SHIFT_MASK |
> GDK_CONTROL_MASK);
> 
> And use "res". See also https://bugzilla.gnome.org/show_bug.cgi?id=694075

I still don't think this is worth it. I would only be able to use it for 3 out of 21 entries - these ones:

  { "alt-shift", N_("Alt+Shift") },
  { "ctrl-shift", N_("Ctrl+Shift") },
  { "alt-ctrl", N_("Alt+Ctrl") },

since gtk_accelerator doesn't distinguish between Left and Right modifier keys and doesn't accept Caps Lock as keyval nor has a string representation for GDK_LOCK_MASK.

As such this would only open the possibility of having inconsistent translations for, say "Shift", because translators in gtk+ might use one form and then a different one in g-c-c.

What I'm doing though is consolidating the 2 definitions we have now for this array into a single place. See the patch I'm attaching next.

(In reply to comment #19)
> (In reply to comment #17)
> > Nope, it works fine:
> > #define INVALID_CHAR GDK_KEY_VoidSymbol - 1
> > res = gtk_accelerator_get_label (INVALID_CHAR, GDK_SHIFT_MASK |
> > GDK_CONTROL_MASK);
> > 
> > And use "res". See also https://bugzilla.gnome.org/show_bug.cgi?id=694075
> 
> I still don't think this is worth it. I would only be able to use it for 3 out
> of 21 entries - these ones:

Aww. 3 out of 21 is pretty bad, thought/hoped it'd be higher than that.

>   { "alt-shift", N_("Alt+Shift") },
>   { "ctrl-shift", N_("Ctrl+Shift") },
>   { "alt-ctrl", N_("Alt+Ctrl") },
> 
> since gtk_accelerator doesn't distinguish between Left and Right modifier keys
> and doesn't accept Caps Lock as keyval nor has a string representation for
> GDK_LOCK_MASK.
> 
> As such this would only open the possibility of having inconsistent
> translations for, say "Shift", because translators in gtk+ might use one form
> and then a different one in g-c-c.
> 
> What I'm doing though is consolidating the 2 definitions we have now for this
> array into a single place. See the patch I'm attaching next.

OK.

Created attachment 236629 [details] [review]
squashed patch from wip/region-panel

Got final a-c-n from Bastien on IRC. Thanks for the review!

Comment on attachment 236629 [details] [review]
squashed patch from wip/region-panel

pushed as individual patches