GNOME Bugzilla – Bug 702351
Restart session in app notification issues
Last modified: 2017-07-10 13:28:59 UTC
When you change the language, it is necessary to restart the session for the change to take effect. For this we show an in-app notification. There are a couple of issues with this: * The text does not wrap - in some languages this causes the notification to become wider than the window. * If you close the window without acting on the restart session notification, there is no way to get back to it. We should probably use a system notification instead - this can be persistent so that you don't lose the opportunity to restart.
(In reply to comment #0) > When you change the language, it is necessary to restart the session for the > change to take effect. For this we show an in-app notification. There are a > couple of issues with this: > > * The text does not wrap - in some languages this causes the notification to > become wider than the window. > * If you close the window without acting on the restart session notification, > there is no way to get back to it. Gah. This is something that was working before the redesign. I guess this is a 3.8.x bug too. > We should probably use a system notification instead - this can be persistent > so that you don't lose the opportunity to restart. No, not really. You'd lose the locality of the notification. It also makes it clearer that the notification happened in response to you changing the language.
The latest mockups have some guidance for showing a permanent message in the panel about restarting the session: https://wiki.gnome.org/Design/SystemSettings/RegionAndLanguage#Tentative_Guidelines
Created attachment 354618 [details] [review] region: Embed "Restart session" button below the Language entry This change is based on the mockups available at https://wiki.gnome.org/Design/SystemSettings/RegionAndLanguage
Created attachment 354619 [details] [review] region: Drop "restart" in-app notification This changes are based on the mockups available at https://wiki.gnome.org/Design/SystemSettings/RegionAndLanguage
Created attachment 354620 [details] [review] region: Make the "Restart" notification persistent When the Language is changed in the Region panel, a "Restart" notification is shown, but if the user closes the window without acting on the restart session notification, there is no way to get back to it. This way we create a temporary file in the g_get_tmp_dir () directory flagging whether we should present the "Restart" notification.
Created attachment 354674 [details] [review] region: Make the "Restart" notification persistent When the Language is changed in the Region panel, a "Restart" notification is shown, but if the user closes the window without acting on the restart session notification, there is no way to get back to it. This way we create a temporary file in the g_get_tmp_dir () directory flagging whether we should present the "Restart" notification.
Review of attachment 354619 [details] [review]: ::: panels/region/cc-region-panel.c @@ -87,3 @@ GCancellable *cancellable; GtkWidget *overlay; this variable is no longer needed
Review of attachment 354618 [details] [review]: ::: panels/region/region.ui @@ +132,3 @@ + <property name="xalign">0</property> + <property name="wrap">True</property> + <property name="max-width-chars">31</property> we probably don't need max-width-chars here @@ +136,3 @@ + <attributes> + <attribute name="scale" value="0.9"/> + <attribute name="foreground" value="#4a90d9"/> not a fan of the explicit color. if the theme ever changes action buttons to look green this will look out of place. just use dim? @@ +152,3 @@ + <property name="margin_top">12</property> + <property name="margin_bottom">12</property> + <property name="valign">end</property> you mean xalign? right now, with max-width-chars being used above, the button shows up in the middle of white space
Review of attachment 354674 [details] [review]: we should probably remove the file and dismiss the button if users change their minds and switch back to the currently used language ::: panels/region/cc-region-panel.c @@ +89,3 @@ GtkWidget *overlay; GtkWidget *restart_notification; + gchar *has_restarted_file_path; needs_restart_file_path? @@ +238,3 @@ } + + if (!g_file_set_contents (priv->has_restarted_file_path, "yes", -1, NULL)) if the contents doesn't matter I'd prefer that we write an empty string instead @@ +1837,3 @@ gtk_container_add (GTK_CONTAINER (self), priv->overlay); + + priv->has_restarted_file_path = g_build_filename (g_get_tmp_dir(), g_get_user_runtime_dir() is a better fit for this since it's user-wide instead of system-wide
Created attachment 355089 [details] [review] region: Embed "Restart session" button below the Language entry This change is based on the mockups available at https://wiki.gnome.org/Design/SystemSettings/RegionAndLanguage
Created attachment 355090 [details] [review] region: Drop "restart" in-app notification This changes are based on the mockups available at https://wiki.gnome.org/Design/SystemSettings/RegionAndLanguage
Created attachment 355091 [details] [review] region: Make the "Restart" notification persistent When the Language is changed in the Region panel, a "Restart" notification is shown, but if the user closes the window without acting on the restart session notification, there is no way to get back to it. This way we create a temporary file in the g_get_tmp_dir () directory flagging whether we should present the "Restart" notification.
Review of attachment 355089 [details] [review]: with max-width-chars added back on the label, looks fine ::: panels/region/region.ui @@ +131,3 @@ + <property name="margin_bottom">12</property> + <property name="xalign">0</property> + <property name="hexpand">True</property> aha, this makes the button be properly aligned to the end of the row @@ +133,3 @@ + <property name="hexpand">True</property> + <property name="wrap">True</property> + <property name="label" translatable="yes">Restart the session for changes to take effect</property> Sorry, we do need max-width-chars here (around 30) or this won't wrap on languages that translate to a longer sentence @@ +154,3 @@ + <property name="margin_top">12</property> + <property name="margin_bottom">12</property> + <property name="valign">end</property> this still seems wrong to me, there's no valign to do here
Review of attachment 355090 [details] [review]: looks fine
Review of attachment 355091 [details] [review]: the commit message should be changed to refer to g_get_user_runtime_dir() instead ::: panels/region/cc-region-panel.c @@ +209,3 @@ + g_file_delete (g_file_new_for_path (priv->needs_restart_file_path), + NULL, NULL); if the user cancels the logout dialog we'll still have the button showing and the file will be gone. not easy to detect though and seems like a small enough corner case so let's not bother :-) @@ +230,1 @@ setlocale (LC_MESSAGES, locale); I don't know when this stopped working but it doesn't work in master right now either so I think we should remove it, perhaps in an extra patch. It was supposed to show the restart string translated in the target language but I guess that was never really a problem since if a user was able to navigate the UI until this point in the current language, translating the "needs restart" string only is pointless. @@ +1849,3 @@ + + priv->needs_restart_file_path = g_build_filename (g_get_user_runtime_dir (), + "gnome-control-center-region-has-restarted", let's use -needs-restart on the file name here for consistency also, the string needs to be freed on finalize()
(In reply to Rui Matos from comment #13) > Review of attachment 355089 [details] [review] [review]: > > with max-width-chars added back on the label, looks fine > > ::: panels/region/region.ui > @@ +131,3 @@ > + <property > name="margin_bottom">12</property> > + <property name="xalign">0</property> > + <property name="hexpand">True</property> > > aha, this makes the button be properly aligned to the end of the row > > @@ +133,3 @@ > + <property name="hexpand">True</property> > + <property name="wrap">True</property> > + <property name="label" > translatable="yes">Restart the session for changes to take effect</property> > > Sorry, we do need max-width-chars here (around 30) or this won't wrap on > languages that translate to a longer sentence > > @@ +154,3 @@ > + <property > name="margin_top">12</property> > + <property > name="margin_bottom">12</property> > + <property name="valign">end</property> > > this still seems wrong to me, there's no valign to do here ah, I forgot to mention. If you the translated sentence has multiple lines, it would cause the [Restart] button to fill the whole space vertically. The same in the gnome-control-center-alt, if you resize the window horizontally to the minimum possible -> the restart button fills the container vertically too.
Created attachment 355249 [details] [review] region: Embed "Restart session" button below the Language entry This change is based on the mockups available at https://wiki.gnome.org/Design/SystemSettings/RegionAndLanguage
Created attachment 355250 [details] [review] region: Make the "Restart" notification persistent When the Language is changed in the Region panel, a "Restart" notification is shown, but if the user closes the window without acting on the restart session notification, there is no way to get back to it. This way we create a temporary file in the g_get_user_runtime_dir () directory flagging whether we should present the "Restart" notification.
Review of attachment 355249 [details] [review]: ok
Review of attachment 355250 [details] [review]: lgtm
Thanks! Attachment 355090 [details] pushed as 79295b6 - region: Drop "restart" in-app notification Attachment 355249 [details] pushed as a739ebf - region: Embed "Restart session" button below the Language entry Attachment 355250 [details] pushed as db551f1 - region: Make the "Restart" notification persistent