GNOME Bugzilla – Bug 774222
Update the Online Accounts panel UI
Last modified: 2017-02-24 16:28:59 UTC
We have new mockups for the Online Accounts panel. This bug is meant to track G-C-C side of the changes - GOA patches will be provided soon.
Created attachment 339508 [details] [review] online-accounts: turn into a final template class Since CcPanel has support for G_DEFINE_{FINAL|DERIVABLE}_TYPE now, lets use it in the Online Accounts panel and simplify some code.
Created attachment 339509 [details] [review] online-accounts: use a listbox instead of a treeview As part of the port to the redesigned Online Accounts panel, the main widget to be displayed is a GtkListBox, so we can have finer control of the UI elements and eventually be able to put real widgets instead of using cell renderers. This commit, then, makes the Online Account panel use a listbox widget in the sidebar. The behavior of the panel was not changed. Since its using a listbox now, we can drop the custom GoaPanelAccountsModel class.
Created attachment 339510 [details] [review] online-accounts: use the ui file to connect to signals Simplify the code by using the GtkBuilder file to connect to the signals instead of manually doing it in the code.
Created attachment 339511 [details] [review] online-accounts: use a dialog to manage the online account Following the mockups, the Online Accounts panel shall use a modal dialog to edit online accounts. This commit moves the current widgets to a dialog, and show this dialog whenever an account is selected.
Created attachment 339512 [details] [review] online-accounts: make the main listbox non-selectable Instead of selecting an account, this commit makes the account editor dialog only visible through explicit activation of the account row. As a side effect, this commit temporarily makes removing an account non-functional.
Created attachment 339513 [details] [review] online-accounts: add a header label Add a header label as per the latest mockups.
Created attachment 339514 [details] [review] online-accounts: make the entire panel scrollable The current implementation of the Online Accounts panel allows only the account list to scroll. To prepare ouselves for the future changes, lets make the entire panel scrollable.
Created attachment 339515 [details] [review] online-accounts: add a providers listbox to the panel This commit starts moving the contents of the add account dialog class to the panel itself, by adding a providers listbox below the list of available account as per the new mockups. The next commits are focused on finishing this move and removing the add account dialog.
Created attachment 339516 [details] [review] online-accounts: remove unused widgets These widgets are not used anymore, and they're not present in the latest designs.
Created attachment 339517 [details] [review] online-accounts: add a stack to the account dialog This will be used to unify the account creation proccess and the account editor.
Created attachment 339518 [details] [review] online-accounts: make adding an account inline to the edit dialog When adding an account, the old proccess was: use the (removed) toolbar to open the New Account dialog, select a provider in that dialog, add the account and see the newly created account in the panel itself. That approach had issues, e.g. the user would have to close the dialog if she mistakenly selected a provider. After moving the provider list to the panel itself, it doesn't make sense anymore to have another provider list inside the dialog. Fix this by moving the new account view to the accounts dialog.
Created attachment 339519 [details] [review] online-account: explicitly remove the dialog's border width Looks like Gtk+ assumes all dialogs' internal-vbox widgets have a 2px border, so explicitly set it to 0.
Created attachment 339520 [details] [review] online-accounts: add a remove button to the account dialog After temporarily removing the ability to delete accounts in efa03cfa57b, lets start bringing it back again by adding a Remove Account button to the account editor dialog, as per the recent mockups.
Created attachment 339521 [details] [review] online-account: bring back account removal Following the previous commit, this commit effectively brings back the account removal feature that was temporarily removed by efa03cfa57b. This is now reimplemented as an in-app notification, and users now have the option to undo an accidentally removed account.
Created attachment 339522 [details] [review] online-accounts: fix indentation To not clutter the previous commit, the indentation of the UI file was not changed. This commit does nothing but fix the indentation.
Created attachment 339523 [details] [review] online-accounts: keep account editor dialog visible when adding account When adding a new account, the account editor dialog always gets hidden when any response is given. This breaks, e.g., the ownCloud process, and hides the dialog while it's still validating the account. Fix that by only hiding the dialog on cancel or close.
Created attachment 339524 [details] [review] online-accounts: small UI improvement Add more margin to the lists.
Created attachment 339525 [details] [review] online-accounts: add separators between listbox rows
Created attachment 339526 [details] [review] online-accounts: hide remove button when account is locked
Created attachment 339527 [details] [review] online-accounts: add a bottom row to display non-branded providers As per the mockups, and mimicking the dialog behavior, add a bottom row that shows non-branded providers.
Created attachment 339528 [details] [review] online-accounts: drop GoaAddAccountDialog The dialog is not used anymore, and can be safely removed.
Created attachment 339529 [details] [review] online-accounts: add an offline message label When offline, we currently block the new accounts listbox so that users can't add new accounts. There is, however, no visual indication that we're offline, besides the listbox. Fix that by adding a descriptive label that's only visible when offline.
Created attachment 339530 [details] [review] online-accounts: make the account editor box vertical When credentials expire, they're being added horizontally since this is the default value of the GtkOrientable:orientation property. Fix that by making the account editor box vertical, and adding some spacing.
Created attachment 339543 [details] [review] online-accounts: align the panel widgets in the middle The current implementation of the Online Accounts panel allows 2 states: either the widgets of the panel fill the whole horizontal space, or they shrink and fill only the absolutely minimum. The ideal solution, however, is to make them grow with the panel. Fix that by turn the main box into a GtkGrid, and adding stub widgets that expand horizontally and pull the main widgets to the middle, allowing them to cover at most 1/3 of the screen.
Created attachment 339544 [details] [review] online-accounts: only show the accounts list when there are accounts When the user has no account set, the current implementation of the Online Accounts panel shows a weird 1px line that is the empty list. Fix that by only showing the list when there are accounts available.
Created attachment 339550 [details] [review] online-accounts: only show the accounts list when there are accounts Found an issue in the last patch.
Review of attachment 339508 [details] [review]: Thanks for these patches! ::: panels/online-accounts/cc-online-accounts-panel.c @@ -243,3 @@ - gtk_style_context_set_junction_sides (context, GTK_JUNCTION_BOTTOM); - context = gtk_widget_get_style_context (panel->toolbar); - gtk_style_context_set_junction_sides (context, GTK_JUNCTION_TOP); This chunk is not really related to this patch. Could you please split it into a separate one? ::: panels/online-accounts/cc-online-accounts-panel.h @@ +26,3 @@ G_BEGIN_DECLS +#define CC_TYPE_ONLINE_ACCOUNTS_PANEL (cc_goa_panel_get_type()) While it doesn't break the build or anything, this isn't right. Typo? :) The correct thing to do would be to do: s/CC_GOA_TYPE_PANEL/CC_TYPE_GOA_PANEL ... in a separate patch.
Review of attachment 339509 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +420,3 @@ else { + show_page_nothing_selected (self); This isn't getting called for the 'no configured accounts' case. I don't know why. ::: panels/online-accounts/online-accounts.ui @@ -33,3 @@ <child> - <object class="GtkTreeView" id="accounts_treeview"> - <property name="width_request">278</property> Without the width_request, the list box will be very narrow when there are no accounts ... @@ +35,3 @@ <property name="visible">True</property> <property name="can_focus">True</property> + <property name="vexpand">True</property> ... and the vexpand isn't really needed. Not a big deal, but would be nice to avoid spurious changes. @@ +37,3 @@ + <property name="vexpand">True</property> + <property name="selection_mode">browse</property> + <signal name="row-selected" handler="on_listbox_selection_changed" object="CcGoaPanel" swapped="no" /> Isn't swapped="no" the default? If we really want to be verbose then there is also G_CONNECT_AFTER. :)
Created attachment 340778 [details] [review] online-accounts: turn into a final template class Code review applied.
Created attachment 340790 [details] [review] online-accounts: use a listbox instead of a treeview Code review applied.
Review of attachment 340778 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ -243,3 @@ - gtk_style_context_set_junction_sides (context, GTK_JUNCTION_BOTTOM); - context = gtk_widget_get_style_context (panel->toolbar); - gtk_style_context_set_junction_sides (context, GTK_JUNCTION_TOP); Would have been nice to split this out.
Review of attachment 340790 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ -295,3 @@ - - out: - gtk_widget_show_all (GTK_WIDGET (panel)); Let's keep this and use GtkWidget:no-show-all. See below. @@ +438,2 @@ + /* Select the first row */ + gtk_list_box_select_row (listbox, gtk_list_box_get_row_at_index (listbox, 0)); We need to call show_page_nothing_selected here. Otherwise, it won't properly show the empty view if the panel is started without any accounts. @@ +459,3 @@ + box = g_object_new (GTK_TYPE_BOX, + "spacing", 6, + "margin", 6, We didn't have a margin. @@ +477,3 @@ } + icon = gtk_image_new_from_gicon (gicon, GTK_ICON_SIZE_DND); GTK_ICON_SIZE_DIALOG, not DND. Creating a GtkImage with a NULL GIcon will lead to a CRITICAL. Even though the GIcon will only be NULL on a broken system, it would be nice to avoid the CRITICAL since we already handle the GError. @@ +491,3 @@ + "xalign", 0.0, + "use-markup", TRUE, + "hexpand", TRUE, We should also set ellipsize=END. @@ +498,3 @@ + + /* "Needs attention" icon */ + icon = gtk_image_new_from_icon_name ("dialog-warning-symbolic", GTK_ICON_SIZE_BUTTON); It will be safer to set GtkWidget:no-show-all on 'icon', instead of selectively calling gtk_widget_show on the others. @@ +499,3 @@ + /* "Needs attention" icon */ + icon = gtk_image_new_from_icon_name ("dialog-warning-symbolic", GTK_ICON_SIZE_BUTTON); + gtk_style_context_add_class (gtk_widget_get_style_context (icon), "dim-label"); If this is needed by the new design, then we should do it in a separate patch. @@ +553,3 @@ + + children = gtk_container_get_children (GTK_CONTAINER (self->accounts_listbox)); + prev = NULL; 'prev' is not needed because GList is a doubly-linked list. @@ +563,3 @@ + if (row_object == object) + { + gtk_list_box_select_row (GTK_LIST_BOX (self->accounts_listbox), prev ? prev->data : NULL); We also need to account for the case where 'prev' might be NULL. ie. deleting the first account in a list of more than one accounts. @@ +723,3 @@ + gint response; + + selected_row = gtk_list_box_get_selected_row (GTK_LIST_BOX (panel->accounts_listbox)); I would check selected_row for NULL here. Just in case, you know. ::: panels/online-accounts/online-accounts.ui @@ +35,3 @@ <property name="visible">True</property> <property name="can_focus">True</property> + <property name="width_request">278</property> Since we set hexpand=TRUE on the GtkLabels in each row, we need to set hexpand=FALSE on the GtkListBox for the width_request to be honoured. This is different from calling gtk_tree_view_column_pack_start with expand=TRUE. Sorry for not mentioning this earlier.
Created attachment 341069 [details] [review] online-accounts: Fix the namespace prefix I took the liberty to fix the above issues and pushed to master.
Created attachment 341070 [details] [review] online-accounts: Use G_DECLARE_FINAL_TYPE
Created attachment 341071 [details] [review] online-accounts: Remove unnecessary code
Created attachment 341072 [details] [review] online-accounts: Turn into a template class
Created attachment 341073 [details] [review] online-accounts: Use a listbox instead of a treeview
Review of attachment 339510 [details] [review]: Perfect! Pushed to master.
*** Bug 702345 has been marked as a duplicate of this bug. ***
Review of attachment 339511 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +378,3 @@ + + gtk_header_bar_set_subtitle (GTK_HEADER_BAR (panel->edit_account_headerbar), + goa_account_get_presentation_identity (account)); These mockups specify the header bar title as "<GoaAccount:provider-name> account": https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/online-accounts/aday-alt/online-accounts-account-dialogs.png @@ -386,3 @@ } - gtk_list_box_select_row (GTK_LIST_BOX (panel->accounts_listbox), account_row); This will break the following command line invocation: $ gnome-control-center online-accounts account_1480532693_35 @@ -553,3 @@ if (row_object == object) { - gtk_list_box_select_row (GTK_LIST_BOX (self->accounts_listbox), prev ? prev->data : NULL); Since we are no longer selecting a new row after removing an account, it will be nice to teach on_listbox_selection_changed to check if the GtkListBox is empty before calling show_page_nothing_selected. ::: panels/online-accounts/online-accounts.ui @@ +143,2 @@ <child> + <object class="GtkButton" id="add_account_button"> Can't we completely remove this? The new designs don't have any "Add an online account" text, and it would certainly make the diff more readable.
Created attachment 341082 [details] [review] online-accounts: Use a dialog to manage the online account I address the above comments, and tried to keep it as functional as possible.
Comment on attachment 339511 [details] [review] online-accounts: use a dialog to manage the online account This is an obsolete patch that was marked as needs-work. I changed it to committed by mistake. Sorry.
Created attachment 342035 [details] [review] online-accounts: Use a dialog to manage the online account I should never touch a computer while I am hungry and sleepy. Somehow had managed to attach the wrong version of the patch, while I actually pushed the right one. Sorry.
Review of attachment 339512 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +374,3 @@ const gchar *account_id) { + GoaObject *row_object; Looks suspicious. See below. @@ +388,1 @@ account = goa_object_peek_account (row_object); Looks like a typo. row_object would be NULL here. The s/row_object/object/ change seems unnecessary. @@ -553,3 @@ - - if (selected_object == object) - show_page_account (panel, selected_object); I wouldn't remove this. Even with the new designs, if the account that we are looking at changes state (ie, GoaClient::account-changed is emitted) we will have to update the UI. Since GtkListBox doesn't offer a way to get the last activated row, I think we should track it ourselves and update the UI if the active object changes. ::: panels/online-accounts/online-accounts.ui @@ +37,3 @@ <property name="vexpand">True</property> + <property name="selection_mode">none</property> + <signal name="row-activated" handler="on_listbox_row_activated" object="CcGoaPanel" swapped="no" /> We can do swapped="yes" since the GtkListBox is an instance variable.
Created attachment 342038 [details] [review] online-accounts: Update the UI if IsLocked changes Fixes a minor bug and will simplify future changes.
Created attachment 342039 [details] [review] online-accounts: Make the main listbox non-selectable I took the liberty to rebase it against current master and makd the above changes.
Review of attachment 339513 [details] [review]: ::: panels/online-accounts/online-accounts.ui @@ +12,2 @@ <property name="spacing">18</property> + <property name="margin">18</property> Don't we need even more than 18? I am looking at: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/online-accounts/aday-alt/online-accounts-panel.png Or we can just commit the addition of the GtkLabel and do the pixel counting in a later commit.
Created attachment 342768 [details] [review] online-accounts: Add a header label as per the latest mockups I pushed the bit that adds the GtkLabel without changing the margin and border-width, since those need to be higher anyway.
Created attachment 342770 [details] [review] online-accounts: Simplify setting the margin
Review of attachment 339514 [details] [review]: Are you sure about this? The new mockups [1] are not terribly explicit about how scrolling should work, but it gives the impression that only the list boxes should scroll. Or am I missing something? [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/online-accounts/aday-alt/online-accounts-panel.png
(In reply to Debarshi Ray from comment #50) > Review of attachment 339514 [details] [review] [review]: > > Are you sure about this? The new mockups [1] are not terribly explicit about > how scrolling should work, but it gives the impression that only the list > boxes should scroll. Or am I missing something? From #gnome-design on GIMPNet: 14:29 <rishi> feaneron: aday: Hey! Am I misunderstanding something here: https://bugzilla.gnome.org/show_bug.cgi?id=774222#c50 ? 14:30 <aday> rishi: the whole panel should scroll 14:31 <aday> like the notifications panel
Review of attachment 339514 [details] [review]: Looks great, apart from one minor nitpick and the fact that it needs to be rebased on top of master. ::: panels/online-accounts/online-accounts.ui @@ +42,3 @@ + <child> + <object class="GtkListBox" id="accounts_listbox"> + <property name="visible">True</property> Now that the GtkScrolledWindow's min-content-height doesn't directly affect the list box, we need to (temporarily) set vexpand=TRUE so that: (a) the GtkListBox doesn't disappear when there are no accounts (b) seems like the 'add' button doesn't work with the mouse pointer when that happens
Created attachment 342776 [details] [review] online-accounts: Remove unused 'id'
Created attachment 342777 [details] [review] online-accounts: Make the entire panel scrollable Rebased and pushed.
Review of attachment 339515 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +130,3 @@ + gtk_container_add (GTK_CONTAINER (self->providers_listbox), row); + + image = gtk_image_new_from_gicon (icon, GTK_ICON_SIZE_DND); GTK_ICON_SIZE_DIALOG, not DND. @@ +157,3 @@ + + return g_strcmp0 (goa_provider_get_provider_name (a_provider, NULL), + goa_provider_get_provider_name (b_provider, NULL)); Umm... we shouldn't sort the providers alphabetically. Long ago, we decided to sort them based on some vague hand-wavy perception of their popularity, and the quality of integration in GNOME. It seems that the new mockups haven't changed this. This order is hard-coded inside gnome-online-accounts, and goa_provider_get_all respects it: https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/goaprovider.c#n108
Created attachment 342778 [details] [review] online-accounts: Add a providers listbox to the panel Rebased, fixed and pushed.
Review of attachment 339516 [details] [review]: Looks good, but needs to be rebased.
Created attachment 342783 [details] [review] online-accounts: Remove unused widgets Rebased and pushed.
Review of attachment 339517 [details] [review]: Looks good, apart from needing a rebase.
Created attachment 342785 [details] [review] online-accounts: Add a stack to the account dialog Rebased and pushed.
Review of attachment 339518 [details] [review]: ::: panels/online-accounts/online-accounts.ui @@ +79,3 @@ <property name="resizable">False</property> <property name="modal">True</property> + <signal name="response" handler="gtk_widget_hide" object="edit_account_dialog" swapped="no" /> We can't hide the GtkDialog on every 'response' emission. The code inside goa_provider_add_account may emit 'response' multiple times and it isn't always appropriate to hide the dialog. eg., if you click 'connect' with a wrong password when adding an ownCloud account. Instead of hiding it, you want to show a message and let the user re-enter the password. If we hide it like this, then the user will see it flicker as it gets hidden and then shown. Instead, we should hide it after goa_provider_add_account returns. In a broader sense, I think it might be easier to destroy the GtkDialog than trying to re-use the same instance across multiple provider and account types. eg., if you try to add an ownCloud account after adding a Google account, the dialog is taller than it needs to be. Some of the GoaProvider sub-classes don't disconnect their signal handlers from the dialog when the add_account vfunc ends. This means that they can run into a memory error next time the dialog is used. Ideally, they should disconnect, but destroying the dialog is more defensive. And if we do decide to destroy it, it might be nicer to move the dialog into its own sub-class and files. Thoughts?
Created attachment 344264 [details] [review] online-accounts: make adding an account inline to the edit dialog When adding an account, the old proccess was: use the (removed) toolbar to open the New Account dialog, select a provider in that dialog, add the account and see the newly created account in the panel itself. That approach had issues, e.g. the user would have to close the dialog if she mistakenly selected a provider. After moving the provider list to the panel itself, it doesn't make sense anymore to have another provider list inside the dialog. Fix this by moving the new account view to the accounts dialog.
Created attachment 344265 [details] [review] online-accounts: explicitly remove the dialog's border width Looks like Gtk+ assumes all dialogs' internal-vbox widgets have a 2px border, so explicitly set it to 0.
Created attachment 344266 [details] [review] online-accounts: add a remove button to the account dialog After temporarily removing the ability to delete accounts in efa03cfa57b, lets start bringing it back again by adding a Remove Account button to the account editor dialog, as per the recent mockups.
Created attachment 344267 [details] [review] online-accounts: bring back account removal Following the previous commit, this commit effectively brings back the account removal feature that was temporarily removed by efa03cfa57b. This is now reimplemented as an in-app notification, and users now have the option to undo an accidentally removed account.
Created attachment 344268 [details] [review] online-accounts: keep account editor dialog visible when adding account When adding a new account, the account editor dialog always gets hidden when any response is given. This breaks, e.g., the ownCloud process, and hides the dialog while it's still validating the account. Fix that by only hiding the dialog on cancel or close.
Created attachment 344269 [details] [review] online-accounts: small UI improvement Add more margin to the lists.
Created attachment 344270 [details] [review] online-accounts: add separators between listbox rows
Created attachment 344271 [details] [review] online-accounts: hide remove button when account is locked
Created attachment 344272 [details] [review] online-accounts: add a bottom row to display non-branded providers As per the mockups, and mimicking the dialog behavior, add a bottom row that shows non-branded providers.
Created attachment 344273 [details] [review] online-accounts: drop GoaAddAccountDialog The dialog is not used anymore, and can be safely removed.
Created attachment 344274 [details] [review] online-accounts: add an offline message label When offline, we currently block the new accounts listbox so that users can't add new accounts. There is, however, no visual indication that we're offline, besides the listbox. Fix that by adding a descriptive label that's only visible when offline.
Created attachment 344275 [details] [review] online-accounts: make the account editor box vertical When credentials expire, they're being added horizontally since this is the default value of the GtkOrientable:orientation property. Fix that by making the account editor box vertical, and adding some spacing.
Created attachment 344276 [details] [review] online-accounts: align the panel widgets in the middle The current implementation of the Online Accounts panel allows 2 states: either the widgets of the panel fill the whole horizontal space, or they shrink and fill only the absolutely minimum. The ideal solution, however, is to make them grow with the panel. Fix that by turn the main box into a GtkGrid, and adding stub widgets that expand horizontally and pull the main widgets to the middle, allowing them to cover at most 1/3 of the screen.
Created attachment 344277 [details] [review] online-accounts: only show the accounts list when there are accounts When the user has no account set, the current implementation of the Online Accounts panel shows a weird 1px line that is the empty list. Fix that by only showing the list when there are accounts available.
(In reply to Debarshi Ray from comment #61) > We can't hide the GtkDialog on every 'response' emission. The code inside > goa_provider_add_account may emit 'response' multiple times and it isn't > always appropriate to hide the dialog. eg., if you click 'connect' with a > wrong password when adding an ownCloud account. Instead of hiding it, you > want to show a message and let the user re-enter the password. If we hide it > like this, then the user will see it flicker as it gets hidden and then > shown. That's correct. > Instead, we should hide it after goa_provider_add_account returns. OK. > In a broader sense, I think it might be easier to destroy the GtkDialog than > trying to re-use the same instance across multiple provider and account > types. eg., if you try to add an ownCloud account after adding a Google > account, the dialog is taller than it needs to be. This can be fixed (and I couldn't reproduce). > Some of the GoaProvider sub-classes don't disconnect their signal handlers > from the dialog when the add_account vfunc ends. This means that they can > run into a memory error next time the dialog is used. Ideally, they should > disconnect, but destroying the dialog is more defensive. I have no strong feelings about either destroying or keeping it alive, except that keeping it alive makes the code much smaller. If GOA has issues not disconnecting some signals, I can gladly fix those issues in GOA. But yet, I don't see any strong and appealing reasons to destroy it and make it a custom class.
Review of attachment 344264 [details] [review]: Thanks for the rebases, Georges! Some minor comments: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +130,3 @@ if (provider == NULL) { + g_object_set_data (G_OBJECT (row), "goa-provider", NULL); Oops. That was my mistake. Sorry. Would be nice to split it out, though. @@ +182,3 @@ + GTK_DIALOG (self->edit_account_dialog), + GTK_BOX (self->new_account_vbox), + &error); As mentioned in comment 61, we shouldn't hide the dialog on every response event. Instead we should do it after returning from goa_provider_add_account if object == NULL. @@ +185,3 @@ + + if (object) + show_page_account (self, object); Between successfully adding an account and showing it, I see a flicker - as if the window got hidden and re-shown. I was expecting a smooth GtkStack transition instead. Do you see this too? I am seeing this even after fixing the above problem.
Created attachment 345248 [details] [review] online-accounts: Fix typo
Created attachment 345249 [details] [review] online-accounts: Make adding an account inline to the edit dialog I took the liberty to avoid hiding on every response. However the flickering between add and show is still there.
Review of attachment 344265 [details] [review]: ++
(In reply to Debarshi Ray from comment #61) > Some of the GoaProvider sub-classes don't disconnect their signal handlers > from the dialog when the add_account vfunc ends. This means that they can > run into a memory error next time the dialog is used. Ideally, they should > disconnect, but destroying the dialog is more defensive. I filed bug 778353
Review of attachment 344266 [details] [review]: ::: panels/online-accounts/online-accounts.ui @@ +117,3 @@ + <property name="orientation">vertical</property> + <child> + <object class="GtkBox" id="accounts_vbox"> Nitpick: tiny whitespace oddity. @@ +128,3 @@ + <property name="valign">end</property> + <property name="halign">end</property> + <property name="vexpand">True</property> Are you sure that we should vexpand the GtkButton, and not the GtkBox above it? It seems to work as expected right now. I am worried that it might break if things get moved around or gtk+ subtly changes behaviour.
(In reply to Debarshi Ray from comment #82) > Are you sure that we should vexpand the GtkButton, and not the GtkBox above > it? It seems to work as expected right now. I am worried that it might break > if things get moved around or gtk+ subtly changes behaviour. I don't expect GtkBox to change it's behavior. It was declared stable, and changing it without further noticing devs would be a bug. But if moving the 'vexpand' to the parent box works, I don't mind having it there.
Created attachment 345252 [details] [review] online-accounts: Add a remove button to the account dialog Moved the vexpand to accounts_vbox and pushed.
Review of attachment 344267 [details] [review]: Nice. Thanks for implementing undoable delete. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +42,3 @@ GoaObject *active_object; + GoaObject *edited_object; This is the same as active_object. @@ +718,3 @@ + GoaObject *row_object = g_object_get_data (l->data, "goa-object"); + + if (goa_object_peek_account (row_object) == account) You can directly use the GoaObject. See below. @@ +829,3 @@ + + /* Show the notification... */ + label = g_strdup_printf (_("Account <b>%s</b> removed"), goa_account_get_presentation_identity (account)); The comments are redundant. :) Your code is quite readable and self-explanatory. On the other hand, it would be good to add a comment explaining the '%s' for translators. By the way, did the designers ask for the bold face? We usually try to avoid exposing such markup to translators, and at least some other applications don't use it. For example, see bug 745756 @@ +835,3 @@ + + /* ... and hide both the account row and the account dialog */ + row = get_row_for_account (panel, account); Just pass panel->removed_object instead of account. @@ +841,3 @@ + + /* Fire up a timer to remove the account */ + panel->remove_account_timeout_id = g_timeout_add_seconds (7, on_remove_account_timeout, panel); Curious - why 7? :) Are other notifications using 7 second timeouts? For what it's worth, Documents and Photos use 10 seconds. ::: panels/online-accounts/online-accounts.ui @@ +47,3 @@ + <style> + <class name="flat" /> + <class name="image-button" /> The "image-button" style class is auto-magically added by GtkButton. @@ +66,3 @@ + </child> + </object> + </child> Nitpick: the identation is off. I'd really like to split this entire file into separate smaller widgets. As the amount of XML increases, it gets harder to read it, understand diffs and resolve conflicts. But we can do it later. Not a blocker for this patch.
Created attachment 345626 [details] [review] online-accounts: Bring back account removal Tweaked the notification string to be '<b>foo@bar.org</b> removed', as suggested by Allan in #gnome-design; fixed the rest of the issues; and pushed.
Review of attachment 344268 [details] [review]: Georges, could you please confirm this: ::: panels/online-accounts/online-accounts.ui @@ +145,3 @@ <property name="modal">True</property> <signal name="delete-event" handler="on_edit_account_dialog_delete_event" object="CcGoaPanel" swapped="yes" /> + <signal name="response" handler="on_dialog_response" object="CcGoaPanel" swapped="no" /> This might not be needed anymore. Cancelling and closing the dialog is taken care of by the delete-event handler. Plus, we hide it once after goa_provider_add_account returns.
Review of attachment 344269 [details] [review]: Needs to be rebased.
Created attachment 345629 [details] [review] online-accounts: Small UI improvement
Review of attachment 344270 [details] [review]: ++
Review of attachment 344271 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +493,3 @@ + /* Hide the Remove button for locked accounts */ + is_locked = goa_account_get_is_locked (goa_object_peek_account (object)); We can use 'account' from the following block of code.
Created attachment 345630 [details] [review] online-accounts: Hide remove button when account is locked
Review of attachment 344273 [details] [review]: ++
Review of attachment 344272 [details] [review]: Needs to be rebased.
(In reply to Debarshi Ray from comment #87) > Review of attachment 344268 [details] [review] [review]: > > Georges, could you please confirm this: > > ::: panels/online-accounts/online-accounts.ui > @@ +145,3 @@ > <property name="modal">True</property> > <signal name="delete-event" > handler="on_edit_account_dialog_delete_event" object="CcGoaPanel" > swapped="yes" /> > + <signal name="response" handler="on_dialog_response" > object="CcGoaPanel" swapped="no" /> > > This might not be needed anymore. Cancelling and closing the dialog is taken > care of by the delete-event handler. Plus, we hide it once after > goa_provider_add_account returns. From #control-center on GIMPNet: 15:23 <rishi> feaneron: Question for you: https://bugzilla.gnome.org/show_bug.cgi?id=774222#c87 15:35 <feaneron> rishi: you're right about the first comment 15:35 <feaneron> sorry, that slipped through during the rebase 15:35 <feaneron> no need to have that "+ <signal..." line
Created attachment 345640 [details] [review] online-accounts: Add a bottom row to display non-branded providers As per the mockups, and mimicking the dialog behavior, add a bottom row that shows non-branded providers.
Created attachment 345641 [details] [review] online-accounts: Add an offline message label When offline, we currently block the new accounts listbox so that users can't add new accounts. There is, however, no visual indication that we're offline, besides the listbox. Fix that by adding a descriptive label that's only visible when offline.
Created attachment 345642 [details] [review] online-accounts: Align the panel widgets in the middle The current implementation of the Online Accounts panel allows 2 states: either the widgets of the panel fill the whole horizontal space, or they shrink and fill only the absolutely minimum. The ideal solution, however, is to make them grow with the panel. Fix that by turn the main box into a GtkGrid, and adding stub widgets that expand horizontally and pull the main widgets to the middle, allowing them to cover at most 1/3 of the screen.
Created attachment 345643 [details] [review] online-accounts: Only show the accounts list when there are accounts When the user has no account set, the current implementation of the Online Accounts panel shows a weird 1px line that is the empty list. Fix that by only showing the list when there are accounts available.
Created attachment 345644 [details] [review] online-accounts: Make the account editor box vertical When credentials expire, they're being added horizontally since this is the default value of the GtkOrientable:orientation property. Fix that by making the account editor box vertical, and adding some spacing.
Review of attachment 345640 [details] [review]: Thanks for the rebase, Georges. Looks good apart from some minor details: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +129,3 @@ GtkWidget *row; GtkWidget *row_grid; + GIcon *icon; Nitpick: tiny little spurious change. @@ +164,3 @@ + features = goa_provider_get_provider_features (provider); + + if ((features & GOA_PROVIDER_FEATURE_BRANDED) > 0) I'll sleep better if you checked for inequality instead. An enumerated type is only guaranteed to be large enough to hold 'int' values. However compilers are allowed to choose the actual type used to represent an enum. For example, if the constants are small enough to fit into a smaller type or are positive, then the compiler might go for something other than 'int'. Right now, GoaProviderFeatures doesn't have any negative constants, so this code might be fine, but there are no guarantees. Checking for inequality saves us the trouble of reading the C specification, experimenting with language implementations, and some head-scratching. Plus it's more common. :) @@ +185,3 @@ + self = user_data; + + if (a == (GtkListBoxRow*) self->more_providers_row) Nitpick: would be nice to use GTK_LIST_BOX (...) instead. Since GObject is primarily a run-time type system, it's nice to have the added run-time assertions, as long as it doesn't hurt performance. @@ +194,3 @@ + + a_branded = (goa_provider_get_provider_features (a_provider) & GOA_PROVIDER_FEATURE_BRANDED) > 0; + b_branded = (goa_provider_get_provider_features (b_provider) & GOA_PROVIDER_FEATURE_BRANDED) > 0; Same thing about using inequality. @@ +197,3 @@ + + if (a_branded != b_branded) + return b_branded - a_branded; Again, I'll sleep better if you opened this code up a bit. In C, any non-zero counts as 'true'. So both negative and positive values will evaluate as a logical true expression.
Created attachment 345808 [details] [review] online-accounts: Add a bottom row to display non-branded providers Fixed the above issues.
Review of attachment 345641 [details] [review]: I can't test this because NetworkManager is not correctly reflecting my connectivity status. It thinks I am connected even if I have switched off all my interfaces. However, the patch is quite straightforward, so I'll assume that it works. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +451,3 @@ goa_provider_get_all (get_all_providers_cb, panel); + gtk_widget_show (GTK_WIDGET (panel)); Setting GtkWidget:no-show-all on the GtkLabel might be slightly safer, but if it works, then fine. ::: panels/online-accounts/online-accounts.ui @@ +97,3 @@ + <property name="can_focus">False</property> + <property name="wrap">True</property> + <property name="label" translatable="yes">No internet connection - connect to setup new online accounts</property> Would be nicer to use the Unicode em-dash ('—') instead.
Created attachment 345825 [details] [review] online-accounts: Add an offline message label Changed it to use the Unicode em-dash.
Review of attachment 345642 [details] [review]: ++
Review of attachment 345643 [details] [review]: Needs a few fixes to make it work the undoable remove. ::: panels/online-accounts/cc-online-accounts-panel.c @@ +716,3 @@ gtk_container_add (GTK_CONTAINER (self->accounts_listbox), row); gtk_widget_show_all (row); + gtk_widget_show (self->accounts_frame); We need to do the same in on_undo_button_clicked ... @@ +760,3 @@ + /* Hide the list if we removed the last account */ + gtk_widget_set_visible (self->accounts_frame, children != NULL); ... and the same in on_remove_button_clicked. The comment is a bit redundant, but whatever.
Review of attachment 345644 [details] [review]: ++
Review of attachment 345643 [details] [review]: Another problem is that when we hide the accounts list, the row spacing between the heading and the providers list gets doubled. We need to avoid this for any row whose visibility might get toggled. One option could be to use GtkWidget:margin-{bottom,top} instead of GtkGrid:row-spacing for spacing. This might (I can't test) also affect the label that we added to indicate the absence of network connectivity.
(In reply to Debarshi Ray from comment #103) > Review of attachment 345641 [details] [review] [review]: > > I can't test this because NetworkManager is not correctly reflecting my > connectivity status. It thinks I am connected even if I have switched off > all my interfaces. However, the patch is quite straightforward, so I'll > assume that it works. That's bug 778688 for the record.
What is supposed to happen if an account needs attention? Right now it opens a second pop-up to let the user interact. (In reply to Debarshi Ray from comment #61) > In a broader sense, I think it might be easier to destroy the GtkDialog than > trying to re-use the same instance across multiple provider and account > types. eg., if you try to add an ownCloud account after adding a Google > account, the dialog is taller than it needs to be. This problem is still present. > Some of the GoaProvider sub-classes don't disconnect their signal handlers > from the dialog when the add_account vfunc ends. This means that they can > run into a memory error next time the dialog is used. Ideally, they should > disconnect, but destroying the dialog is more defensive. That's bug 778353
Created attachment 345934 [details] [review] online-accounts: Align the panel widgets in the middle The current implementation of the Online Accounts panel allows 2 states: either the widgets of the panel fill the whole horizontal space, or they shrink and fill only the absolutely minimum. The ideal solution, however, is to make them grow with the panel. Fix that by turn the main box into a GtkGrid, and adding stub widgets that expand horizontally and pull the main widgets to the middle, allowing them to cover at most 1/3 of the screen. The widgets themselves are inside a GtkBox, so that hiding them automatically removes the spacing in between.
Created attachment 345935 [details] [review] online-accounts: Only show the accounts list when there are accounts Updated to match the rework.
Created attachment 345936 [details] [review] online-accounts: always keep editor dialog size the minimum When adding and editing accounts, the different options that Online Accounts provides may require different sizes of the dialog. There is, however, a specific issue where the dialog can't resize because a provider widget is interefering with the others. Fix that by making the dialog's stack non-homogeneous, and making sure to always reset the dialog size before showing it.
Created attachment 345937 [details] [review] online-accounts: add margins to the rows' icons Per Allan's request, add more margin to the rows' icons.
Review of attachment 345937 [details] [review]: ++
Review of attachment 345936 [details] [review]: ++ Let's get this in. Preventing the dialog from getting expanding under certain situations isn't really a UI freeze break. It's a bug fix.
(In reply to Debarshi Ray from comment #116) > Review of attachment 345936 [details] [review] [review]: > > ++ > > Let's get this in. Preventing the dialog from getting expanding under > certain situations isn't really a UI freeze break. It's a bug fix. "from expanding"
Comment on attachment 345937 [details] [review] online-accounts: add margins to the rows' icons From #release-team on GIMPNet: 16:22 <rishi> Do I need to ask from freeze break exceptions for things like adding margin around icons? 16:22 <rishi> *ask for 16:22 <mcatanzaro> rishi: No 16:23 <mcatanzaro> It's not *that* strict :P
Review of attachment 345935 [details] [review]: As mentioned in comment 106, I think: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +721,3 @@ gtk_container_add (GTK_CONTAINER (self->accounts_listbox), row); gtk_widget_show_all (row); + gtk_widget_show (self->accounts_frame); We need to do the same in on_undo_button_clicked ... @@ +765,3 @@ + /* Hide the list if we removed the last account */ + gtk_widget_set_visible (self->accounts_frame, children != NULL); ... and the same in on_remove_button_clicked. The comment is a bit redundant, but whatever.
Review of attachment 345934 [details] [review]: Looks better. Using a GtkBox instead of a GtkGrid to hold the column of widgets seems to have fixed the problem of left-over row-spacing when hiding rows. However, let's till the other patch is nailed down to avoid needless to and fro at this late hour.
Created attachment 346255 [details] [review] online-accounts: Only show the accounts list when there are accounts When the user has no account set, the current implementation of the Online Accounts panel shows a weird 1px line that is the empty list. Fix that by only showing the list when there are accounts available.
Review of attachment 346255 [details] [review]: ::: panels/online-accounts/cc-online-accounts-panel.c @@ +945,3 @@ + children = gtk_container_get_children (GTK_CONTAINER (panel->accounts_listbox)); + gtk_widget_set_visible (panel->accounts_frame, children != NULL); This doesn't work because hiding a child won't remove it from the list of children.
Created attachment 346430 [details] [review] online-accounts: Shuffle some code around I spotted an opportunity for some clean-ups and took the liberty to fix the above issue.
Created attachment 346431 [details] [review] online-accounts: Consolidate the row modification code a bit
Created attachment 346432 [details] [review] online-accounts: Only show the accounts list when there are accounts
Attachment 345644 [details] pushed as 4662c5b - online-accounts: Make the account editor box vertical Attachment 345808 [details] pushed as e9198bd - online-accounts: Add a bottom row to display non-branded providers Attachment 345825 [details] pushed as 1e580c6 - online-accounts: Add an offline message label Attachment 345934 [details] pushed as 1594d8c - online-accounts: Align the panel widgets in the middle Attachment 346430 [details] pushed as 25f97f1 - online-accounts: Shuffle some code around Attachment 346431 [details] pushed as af7c214 - online-accounts: Consolidate the row modification code a bit Attachment 346432 [details] pushed as fa81d16 - online-accounts: Only show the accounts list when there are accounts Thanks Debarshi for this extensive work of review, and all the corrections you did throughout the process. Lets follow up this work with eventual bugfixes.