GNOME Bugzilla – Bug 671980
Improving the design of the online-accounts panel
Last modified: 2012-06-06 12:26:41 UTC
The UI of the online-accounts panel needs to be improved to match with what the designers have in mind. Latest: http://ur1.ca/8m05c Earlier: https://live.gnome.org/Design/SystemSettings/OnlineAccounts
Created attachment 209597 [details] [review] online-accounts: New "Add Account" dialog
Created attachment 209598 [details] [review] online-accounts: Fix spacing and alignment when showing accounts
These patches are meant to work with these changes to gnome-online-accounts: https://bugzilla.gnome.org/671982
Assuming that this is 3.5 stuff as we are under UI freeze. If not, please ask for a freeze exception - see https://live.gnome.org/ThreePointThree#Schedule
(In reply to comment #1) > Created an attachment (id=209597) [details] [review] > online-accounts: New "Add Account" dialog You're missing the frame around the list.
Review of attachment 209597 [details] [review]: It's currently unclear what happens when you click one of the item in the dialogue, as processing the click is somewhat lengthy. You should add an async version of goa_provider_add_account(), and add a spinner to the tree row until the click is processed, and add_account has finished. ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c @@ +110,3 @@ + +static gboolean +tree_view_button_press_event_cb (GtkWidget *widget, Eek. No. Use selection-changed instead. @@ +124,3 @@ + +static gboolean +tree_view_button_release_event_cb (GtkWidget *widget, Ditto.
Created attachment 209885 [details] [review] online-accounts: New "Add Account" dialog
(In reply to comment #5) > (In reply to comment #1) > > Created an attachment (id=209597) [details] [review] [details] [review] > > online-accounts: New "Add Account" dialog > > You're missing the frame around the list. Done.
Created attachment 211307 [details] [review] online-accounts: Remove horizontal scrolling from accounts list
Created attachment 211390 [details] [review] online-accounts: Load and add the GOA CSS style
(In reply to comment #6) > Review of attachment 209597 [details] [review]: > > It's currently unclear what happens when you click one of the item in the > dialogue, as processing the click is somewhat lengthy. > > You should add an async version of goa_provider_add_account(), and add a > spinner to the tree row until the click is processed, and add_account has > finished. Currently there are quite a few places GOA blocks the Gtk main loop while adding an account. Some of these have now been fixed in GOA's master, while there are a few remaining. See: https://bugzilla.gnome.org/672510 > ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c > @@ +110,3 @@ > + > +static gboolean > +tree_view_button_press_event_cb (GtkWidget *widget, > > Eek. No. Use selection-changed instead. > > @@ +124,3 @@ > + > +static gboolean > +tree_view_button_release_event_cb (GtkWidget *widget, > > Ditto. I lifted this idea from shell/cc-shell-item-view.c so that one can navigate using the keyboard and hit enter to activate, or just single click to activate.
Created attachment 212144 [details] [review] online-accounts: Remove horizontal scrolling from accounts list Prevent the accounts treeview from shrinking too much when there are no added accounts.
*** Bug 654193 has been marked as a duplicate of this bug. ***
Created attachment 213492 [details] [review] online-accounts: Don't hardcode the text color
As mentioned on IRC. * The designs for the first screen of the add dialog assumed a different titlebar design. Since we don't have these yet we'll need to have a heading text. Perhaps just like the titlebar text "Add Account". * The spinner that is used after the account type selection is far too large. * The spinner seems to jump a bit to the upper left just at the end * We still have the google auth page size issue.. separate bug.
Created attachment 214050 [details] [review] online-accounts: New "Add Account" dialog This should address the following problems: > * The designs for the first screen of the add dialog assumed a > different titlebar design. Since we don't have these yet we'll > need to have a heading text. Perhaps just like the titlebar text > "Add Account". > * The spinner that is used after the account type selection is far too large.
Review of attachment 211390 [details] [review]: ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c @@ +262,3 @@ priv = add_account->priv; + css_path = goa_util_get_css (); If you're going to stuff the css file somewhere, might as well be using a GResource so we don't have to load it from the disk.
Review of attachment 212144 [details] [review]: Looks good. No shortened URL in the commit logs though. Use the anchor inside the design page instead.
Review of attachment 213492 [details] [review]: Looks good
Review of attachment 214050 [details] [review]: ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c @@ +118,3 @@ + + /* be sure to ignore double and triple clicks */ + priv->ignore_release = (event->type != GDK_BUTTON_PRESS); You know that a double-click will generate GDK_BUTTON_PRESS, right? @@ +173,3 @@ + gint height; + + gtk_window_get_size (parent, &width, &height); You do that sort of thing in ->realize, not ->show @@ +180,3 @@ + + if (gtk_tree_view_get_model (GTK_TREE_VIEW (priv->tree_view)) == NULL) + gtk_tree_view_set_model (GTK_TREE_VIEW (priv->tree_view), GTK_TREE_MODEL (priv->list_store)); Why in ->show? Why is the NULL check necessary? @@ +190,3 @@ + + if (priv->tree_view != NULL) + priv->tree_view = NULL; Why is this necessary? @@ +194,3 @@ + if (priv->list_store != NULL) + { + g_object_unref (priv->list_store); Use g_clear_object()
(In reply to comment #20) > Review of attachment 214050 [details] [review]: > > ::: panels/online-accounts/cc-online-accounts-add-account-dialog.c > @@ +118,3 @@ > + > + /* be sure to ignore double and triple clicks */ > + priv->ignore_release = (event->type != GDK_BUTTON_PRESS); > > You know that a double-click will generate GDK_BUTTON_PRESS, right? Yes. I made a mistake in the release handler where the && should be || to have the same behaviour as shell/cc-shell-item-view.c. Fixed. Anything else? > @@ +173,3 @@ > + gint height; > + > + gtk_window_get_size (parent, &width, &height); > > You do that sort of thing in ->realize, not ->show True, but I vaguely recollect having tried it but for some reason it did not work. I don't remember the details anymore. Anyway, I have changed it to ->realize now. > @@ +180,3 @@ > + > + if (gtk_tree_view_get_model (GTK_TREE_VIEW (priv->tree_view)) == NULL) > + gtk_tree_view_set_model (GTK_TREE_VIEW (priv->tree_view), GTK_TREE_MODEL > (priv->list_store)); > > Why in ->show? Why is the NULL check necessary? The NULL check was to avoid setting the model once it has already been set. Anyway, I have changed it to ->realize now and removed the check. > @@ +190,3 @@ > + > + if (priv->tree_view != NULL) > + priv->tree_view = NULL; > > Why is this necessary? To mark the state between ->dispose and ->finalize. Should I remove it? > Use g_clear_object() Fixed.
Created attachment 215584 [details] [review] online-accounts: New "Add Account" dialog
Created attachment 215585 [details] [review] online-accounts: Remove horizontal scrolling from accounts list The designs on live.gnome.org used to be outdated. Not any more.
Created attachment 215586 [details] [review] build: require GOA >= 3.5.1 The CSS is now part of the theme. So we don't need to carry it any more. However we still need to bump the GOA version.
Review of attachment 215584 [details] [review]: Looks good to me now, fwiw
Review of attachment 215585 [details] [review]: Looks like you did what Bastien asked here, so should be good to commit.
Review of attachment 215586 [details] [review]: Looks fine to me
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.