GNOME Bugzilla – Bug 767065
Redesign User Accounts Panel
Last modified: 2017-02-09 18:32:46 UTC
The following patches tweak the current User Accounts panel to look more like the new mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts#Tentative_Design I would like to keep this bug open until the panel redesign is completely implemented.
Created attachment 328792 [details] [review] user-accounts: update strings to new design https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328793 [details] [review] user-accounts: move spinner to header in Add User dialog https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328794 [details] [review] user-accounts: redesign offline mode for enterprise login https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328795 [details] [review] user-accounts: move arrows to start in login history dialog https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328796 [details] [review] user-accounts: use radio buttons for account type in Add User dialog https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328797 [details] [review] user-accounts: align entries in Add User dialog https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328798 [details] [review] user-accounts: use radio buttons for account type According to the mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328799 [details] [review] user-accounts: drop "Login Options" label According to the mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 328800 [details] [review] user-accounts: use a label for Account Type when user isn't admin
Review of attachment 328792 [details] [review]: Sure.
Review of attachment 328793 [details] [review]: ::: panels/user-accounts/data/account-dialog.ui @@ -670,3 @@ </child> - <child> - <object class="GtkSpinner" id="spinner"> Can't you move it inside the header bar in the .ui file instead?
Review of attachment 328794 [details] [review]: No code changes necessary to make this work?
Review of attachment 328795 [details] [review]: Looks good.
Review of attachment 328796 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +252,3 @@ name = gtk_entry_get_text (GTK_ENTRY (self->local_name)); username = gtk_combo_box_text_get_active_text (GTK_COMBO_BOX_TEXT (self->local_username)); + account_type = (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->local_account_type)) ? 0 : 1); 0, 1? You need to declare and use an enum here. @@ -618,3 @@ - model = gtk_combo_box_get_model (GTK_COMBO_BOX (self->local_username)); - gtk_list_store_clear (GTK_LIST_STORE (model)); - gtk_combo_box_set_active (GTK_COMBO_BOX (self->local_account_type), 0); Don't need to reset the toggle in the new version?
Review of attachment 328797 [details] [review]: Sure.
Review of attachment 328798 [details] [review]: Looks fine.
Review of attachment 328799 [details] [review]: OK.
Review of attachment 328800 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +895,3 @@ + widget = get_widget (d, "account-type-static"); + if (act_user_get_account_type (user) == 1) again with the "1". This needs to be an enum.
Comment on attachment 328792 [details] [review] user-accounts: update strings to new design Attachment 328792 [details] pushed as 6d24ed5 - user-accounts: update strings to new design
Comment on attachment 328795 [details] [review] user-accounts: move arrows to start in login history dialog Attachment 328795 [details] pushed as a70888f - user-accounts: move arrows to start in login history dialog
Comment on attachment 328797 [details] [review] user-accounts: align entries in Add User dialog Attachment 328797 [details] pushed as a848da9 - user-accounts: align entries in Add User dialog
Comment on attachment 328799 [details] [review] user-accounts: drop "Login Options" label Attachment 328799 [details] pushed as 941282a - user-accounts: drop "Login Options" label
Created attachment 328871 [details] [review] user-accounts: use radio buttons for account type According to the mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment on attachment 328800 [details] [review] user-accounts: use a label for Account Type when user isn't admin obsoleted by attachment 328871 [details] [review].
Created attachment 328872 [details] [review] user-accounts: use radio buttons for account type in Add User dialog https://wiki.gnome.org/Design/SystemSettings/UserAccounts
(In reply to Bastien Nocera from comment #12) > Review of attachment 328794 [details] [review] [review]: > > No code changes necessary to make this work? the changes here are pretty much aesthetics. Bigger icon and new label.
Review of attachment 328794 [details] [review]: Ok then.
Review of attachment 328871 [details] [review]: Looks fine.
Review of attachment 328872 [details] [review]: Looks fine.
Attachment 328794 [details] pushed as 5b3a2cc - user-accounts: redesign offline mode for enterprise login Attachment 328871 [details] pushed as 1a6d716 - user-accounts: use radio buttons for account type Attachment 328872 [details] pushed as efc887c - user-accounts: use radio buttons for account type in Add User dialog
Created attachment 329560 [details] [review] user-accounts: split join-dialog from account-dialog The UmAccountDialog class will be ported to gtk widget templates, in doing so, we are decoupling here the gtkbuilder dependend ui code from what will be the class code in the port. Nothing changed in the join-dialog ui code itself.
Created attachment 329561 [details] [review] user-accounts: port Add User dialog to gtk+ composite widget templates
Created attachment 329562 [details] [review] user-accounts: move Add User dialog buttons to ui file
Created attachment 329563 [details] [review] user-accounts: move spinner to header in Add User dialog https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 329687 [details] [review] user-accounts: use Password Login instead of Automatic Login password_login == !automatic_login https://wiki.gnome.org/Design/SystemSettings/UserAccounts
(In reply to Felipe Borges from comment #35) > Created attachment 329687 [details] [review] [review] > user-accounts: use Password Login instead of Automatic Login > > password_login == !automatic_login > https://wiki.gnome.org/Design/SystemSettings/UserAccounts Instead of this patch, I could: 1. keep the autologin* prefixes and just reverse the logic operators; 2. patch accountsservice before; Let me know if you find 1 or 2 to be better than the patch.
Comment on attachment 329687 [details] [review] user-accounts: use Password Login instead of Automatic Login (In reply to Felipe Borges from comment #36) > (In reply to Felipe Borges from comment #35) > > Created attachment 329687 [details] [review] [review] [review] > > user-accounts: use Password Login instead of Automatic Login > > > > password_login == !automatic_login > > https://wiki.gnome.org/Design/SystemSettings/UserAccounts > > Instead of this patch, I could: > 1. keep the autologin* prefixes and just reverse the logic operators; > 2. patch accountsservice before; > > Let me know if you find 1 or 2 to be better than the patch. Moving the discussion to https://bugzilla.gnome.org/show_bug.cgi?id=679745
Created attachment 329691 [details] [review] user-accounts: hide language settings for current user Language settings should not be shown. That's what the Region & Language panel settings are for. See: https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 329692 [details] [review] user-accounts: add space between rows in um-user-panel According to the mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Review of attachment 329560 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +940,3 @@ + GError *error = NULL; + + builder = gtk_builder_new (); leaking the builder object?
Review of attachment 329561 [details] [review]: ::: panels/user-accounts/data/join-dialog.ui @@ -237,3 @@ </object> </interface> - Whitespace change ::: panels/user-accounts/um-account-dialog.c @@ -1470,3 @@ dialog = GTK_DIALOG (self); - content = gtk_dialog_get_content_area (dialog); - gtk_container_set_border_width (GTK_CONTAINER (dialog), 5); You're losing a fair number of properties by moving everything to GtkBuilder files. Make sure the properties and default values (such as the window title) are getting set in the UI file.
Review of attachment 329562 [details] [review]: Looks fine otherwise. ::: panels/user-accounts/data/account-dialog.ui @@ +17,3 @@ + <property name="show_close_button">False</property> + <child> + <object class="GtkButton" id="button1"> Better names for the buttons?
Review of attachment 329563 [details] [review]: Sure.
Review of attachment 329691 [details] [review]: Sure.
Review of attachment 329692 [details] [review]: Sure.
Comment on attachment 329692 [details] [review] user-accounts: add space between rows in um-user-panel Attachment 329692 [details] pushed as d1329f1 - user-accounts: add space between rows in um-user-panel
Comment on attachment 329691 [details] [review] user-accounts: hide language settings for current user Attachment 329691 [details] pushed as 5e2ed8e - user-accounts: hide language settings for current user
Created attachment 329764 [details] [review] user-accounts: split join-dialog from account-dialog The UmAccountDialog class will be ported to gtk widget templates, in doing so, we are decoupling here the gtkbuilder dependend ui code from what will be the class code in the port. Nothing changed in the join-dialog ui code itself.
Review of attachment 329764 [details] [review]: Looks fine.
Created attachment 329774 [details] [review] user-accounts: port Add User dialog to gtk+ composite widget templates
Comment on attachment 329764 [details] [review] user-accounts: split join-dialog from account-dialog Attachment 329764 [details] pushed as de0de51 - user-accounts: split join-dialog from account-dialog
Review of attachment 329774 [details] [review]: Looks good. You need to quote the literal strings in the subject though: user-accounts: Make "Add User" dialog a composite widget
Attachment 329562 [details] pushed as 09cbc05 - user-accounts: move Add User dialog buttons to ui file Attachment 329563 [details] pushed as af13556 - user-accounts: move spinner to header in Add User dialog Attachment 329774 [details] pushed as 1db970e - user-accounts: port "Add User" dialog to gtk+ composite widget templates
Created attachment 330135 [details] [review] user-accounts: Drop unused account-type-model object
Created attachment 330136 [details] [review] user-accounts: Rename "History" button label to "Account Activity"
Created attachment 330137 [details] [review] user-accounts: Rename "History" dialog title to "Account Activity"
Created attachment 330138 [details] [review] user-accounts: Prepend user real name to "Account Activity" dialog title
Created attachment 330139 [details] [review] user-accounts: Make the "Account Activity" wider
Created attachment 330140 [details] [review] user-accounts: Change "Account Activity" granularity to days Before we were grouping the "Account Activity" data in weeks. Now we group them in days. This change is part of the effort towards the redesign specified at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 330141 [details] [review] user-accounts: Drop date from "Account Activity" entries Since we are currently grouping the Account Activity by day, there's no point of showing the date in the activity entries.
Created attachment 330142 [details] [review] user-accounts: Reorder items in "Account Activity" entries According to the mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Review of attachment 330135 [details] [review]: Sure.
Review of attachment 330136 [details] [review]: Yep.
Review of attachment 330137 [details] [review]: ::: panels/user-accounts/data/history-dialog.ui @@ +16,3 @@ <property name="can_focus">False</property> + <property name="title" translatable="yes">Account Activity</property> + <property name="subtitle" translatable="yes">Account Activity</property> This isn't needed, or actually used. In a separate patch, can you remove the "translatable" bit and add a placeholder label that's more likely to happen, like "This Week"?
Review of attachment 330138 [details] [review]: ::: panels/user-accounts/um-history-dialog.c @@ +295,3 @@ + gchar *title; + + title = g_strdup_printf(_("%s - Account Activity"), Needs a "translators comment"
Review of attachment 330139 [details] [review]: ::: panels/user-accounts/data/history-dialog.ui @@ +8,3 @@ <property name="modal">True</property> <property name="window_position">center-on-parent</property> + <property name="width_request">600</property> Could we use a percentage of the parent window's size instead? No strong preference here.
Review of attachment 330140 [details] [review]: We're missing the other half of the redesign for that panel, which would show locks/unlocks. We'd want both in before merging that.
Review of attachment 330141 [details] [review]: Wouldn't that still be necessary if a session started the day before, for example? Rejected as well, as it depends on the previous patch to be useful.
Review of attachment 330142 [details] [review]: Sure.
Attachment 330135 [details] pushed as d1d3919 - user-accounts: Drop unused account-type-model object Attachment 330136 [details] pushed as 2693d41 - user-accounts: Rename "History" button label to "Account Activity" Attachment 330142 [details] pushed as 7d21e69 - user-accounts: Reorder items in "Account Activity" entries
Created attachment 331263 [details] [review] user-accounts: Rename "History" dialog title to "Account Activity"
Created attachment 331264 [details] [review] user-accounts: Drop unused subtitle in "Account Activity" dialog
Created attachment 331265 [details] [review] user-accounts: Prepend user real name to "Account Activity" dialog title
Created attachment 331266 [details] [review] user-accounts: Introduce users list carousel (UmCarousel)
Created attachment 331267 [details] [review] user-accounts: Drop "Lock" button and add "Add User" button
Created attachment 331268 [details] [review] user-accounts: Prevent bottom buttons from expanding "Delete Account" and "Account Activity" buttons.
Created attachment 331269 [details] [review] user-accounts: Drop unnecessary grid containing last-login label
Created attachment 331270 [details] [review] user-accounts: Introduce arrow frame (UmArrowFrame) Type of GtkFrame which points to an item in an UmCarousel.
Created attachment 331271 [details] [review] user-accounts: Drop user image border and background The UmArrowFrame pointing at the user avatar is the indicator of which user is selected. According to the mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 331272 [details] [review] user-accounts: Add missing license to um-carousel.[c/h]
Created attachment 331273 [details] [review] user-accounts: Prevent the removal of own account and only-admin
Created attachment 331274 [details] [review] user-accounts: Update panel after changes in ActUser object
Created attachment 331275 [details] [review] user-accounts: Handle the info update just on the show_user method Instead of setting the new values for the widgets in each of their buttons callbacks, use the show_user method to load what's set in the backend.
Created attachment 331365 [details] [review] user-accounts: Properly align account type buttons These two buttons should have the same size.
Created attachment 331366 [details] [review] user-accounts: Change sensitiveness of back/forward Carousel buttons If there are no more Carousel pages, do not make the back/forward buttons sensitive.
Created attachment 331367 [details] [review] user-accounts: Remove Carousel item when removing account
Created attachment 331368 [details] [review] user-accounts: Select user after account is created
Created attachment 331369 [details] [review] user-accounts: Always select the first item in a page on the Carousel
Created attachment 331370 [details] [review] user-accounts: Centralize post page change events in single function
Created attachment 331394 [details] [review] user-accounts: Remove unnecessary autologin-box
Created attachment 331395 [details] [review] user-accounts: Do not show autologin option when it is impossible The autologin feature is not supported for non-local users and users without password.
Created attachment 331396 [details] [review] user-accounts: Reposition fingerprint-login-button to correct position It was in the same position as the button below it.
Created attachment 331406 [details] screenshot of a bug I've tried wip/feborges/new-users-panel. I did not test overall functionality, but have a few comments for some obvious problems, which would be nice to figure out before reviewing the patches one-by-one: - It doesn't work for just one user account, the dialog is not filled by user details, see attachment. - The buttons "Remove Account" and "Account activity" are not always fully visible, see the attachment also. - Please remove the following unused variables: um-arrow-frame.c: In function ‘um_arrow_frame_compute_child_allocation’: um-arrow-frame.c:260:24: warning: variable ‘priv’ set but not used [-Wunused-but-set-variable] UmArrowFramePrivate *priv; ^~~~ um-carousel.c: In function ‘um_carousel_finalize’: um-carousel.c:340:21: warning: unused variable ‘self’ [-Wunused-variable] UmCarousel *self = (UmCarousel *)object; ^~~~ um-user-panel.c: In function ‘select_created_user’: um-user-panel.c:249:15: warning: variable ‘user_uid’ set but not used [-Wunused-but-set-variable] uid_t user_uid; ^~~~~~~~ um-user-panel.c:247:18: warning: unused variable ‘current’ [-Wunused-variable] ActUser *current; ^~~~~~~ um-user-panel.c: In function ‘on_permission_changed’: um-user-panel.c:1081:18: warning: variable ‘is_authorized’ set but not used [-Wunused-but-set-variable] gboolean is_authorized; ^~~~~~~~~~~~~ - I see the following when deleting user account (it segfaults sometimes): (gnome-control-center:29483): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer (gnome-control-center:29483): GLib-GObject-CRITICAL **: g_signal_handlers_disconnect_matched: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed (gnome-control-center:29483): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-control-center:29483): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed (gnome-control-center:29483): AccountsService-CRITICAL **: act_user_get_object_path: assertion 'ACT_IS_USER (user)' failed (gnome-control-center:29483): AccountsService-CRITICAL **: act_user_get_user_name: assertion 'ACT_IS_USER (user)' failed (gnome-control-center:29483): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed - I see a lot of warnings/criticals (segfaults sometimes), when open user-accounts panel, then switch to e.g. sharing panel and then switch back to the user-accounts panel again... - I see the following when opening "Add user dialog": (gnome-control-center:29655): Gtk-WARNING **: Allocating size to GtkBox 0x2dd8b00 without calling gtk_widget_get_preferred_width/height(). How does the code know the size to allocate? (gnome-control-center:29655): Gtk-WARNING **: GtkEntry 0x2e2f590 is drawn without a current allocation. This should not happen. - Add user dialog: User name hint should have probably reserved at least three lines of vertical space, otherwise the dialog changes its height e.g. when I provide an invalid username. - Account activity: The arrow buttons has a different shapes... - Account activity: Today login info is shown in Yesterday page etc. Also it seems to me also that I miss "Session ended" records in some cases... - Account activity: Dialog is resized and also moved if there is too much records per day. It used to be scrollbar there, so the scrollbar should be restored, or the window should not move, when resizing...
Also I think we want to remove UmEditableCombo and UmEditableButton and replace them by GtkCombo and GtkButton, because those widgets are confusing... worth to discuss with aday...
Review of attachment 331263 [details] [review]: ::: panels/user-accounts/data/history-dialog.ui @@ +4,3 @@ <object class="GtkDialog" id="dialog"> <property name="can_focus">False</property> + <property name="title" translatable="yes">Account Activity</property> I wonder whether we can't remove this at all, because it is set when setting user...
Review of attachment 331264 [details] [review]: ::: panels/user-accounts/data/history-dialog.ui @@ +15,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="subtitle">This Week</property> This should be removed, not changed...
Review of attachment 331265 [details] [review]: ::: panels/user-accounts/um-history-dialog.c @@ +298,3 @@ + /* Translators: This is the title of the "Account Activity" dialog. + The %s is the user real name. */ + title = g_strdup_printf(_("%s - Account Activity"), missing space before opening parenthesis @@ +302,3 @@ + + dialog = get_widget (um, "dialog"); + gtk_window_set_title (GTK_WINDOW (dialog), title); um->dialog
Review of attachment 331267 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +1339,3 @@ + shell = cc_panel_get_shell (CC_PANEL (object)); + + button = gtk_button_new_with_mnemonic (_("_Add User")); Same menmonic is used for Account Activity, so would be nice to use different one I guess...
Review of attachment 331268 [details] [review]: ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +403,3 @@ <property name="visible">True</property> <property name="can_focus">False</property> + <property name="valign">center</property> Is "center" valid? Shouldn't this be GTK_ALIGN_CENTER? However I suppose this should be rather GTK_ALIGN_END, because the buttons should be at the bottom of the dialog... @@ +416,3 @@ <property name="visible">True</property> <property name="can_focus">True</property> + <property name="valign">center</property> dtto
Review of attachment 331269 [details] [review]: Makes sense, because the grid was there due to the "History" button, which was moved away... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +367,3 @@ <property name="can_focus">False</property> + <property name="hexpand">True</property> + <property name="xalign">0</property> xalign is not probably needed, is it?
Review of attachment 331273 [details] [review]: This was removed when introducing carousel, so maybe it should be merged with that patch instead, but this applies on some other patches also, so it is ok probably...
Review of attachment 331274 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +1119,3 @@ } + gtk_widget_show (get_widget (d, "user-icon-button")); This can be probably removed, because is always shown now... @@ +1120,3 @@ + gtk_widget_show (get_widget (d, "user-icon-button")); + gtk_widget_hide (get_widget (d, "user-icon-image")); It seems to me that user-icon-image may be removed completely... @@ +1124,3 @@ + um_editable_button_set_editable (UM_EDITABLE_BUTTON (get_widget (d, "account-language-button")), TRUE); + um_editable_button_set_editable (UM_EDITABLE_BUTTON (get_widget (d, "account-password-button")), TRUE); + um_editable_button_set_editable (UM_EDITABLE_BUTTON (get_widget (d, "account-fingerprint-button")), TRUE); This can be probably removed, or set only once e.g. in .ui file, because it is always editable now...
Review of attachment 331275 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +836,3 @@ act_user_set_account_type (user, account_type); + show_user (user, d); Isn't this handled by user_changed callback?
Review of attachment 331365 [details] [review]: Looks good!
Review of attachment 331394 [details] [review]: AFAIK the box was used to ensure same height with other widgets. It seems to me that the line has slightly different height then others... worth to discuss with aday probably. ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +208,3 @@ <property name="visible">True</property> + <property name="valign">GTK_ALIGN_CENTER</property> + <property name="halign">GTK_ALIGN_START</property> Is this needed?
Also I realized that different user account is selected if I open user-accounts panel, however I would expect that my user account is shown...
Review of attachment 331396 [details] [review]: Looks good...
Review of attachment 331395 [details] [review]: Not sure we want this... worth to discuss with aday.
Comment on attachment 331365 [details] [review] user-accounts: Properly align account type buttons Attachment 331365 [details] pushed as 66cab8a - user-accounts: Properly align account type buttons
Created attachment 331415 [details] [review] user-accounts: Drop unused subtitle in "Account Activity" dialog
Created attachment 331416 [details] [review] user-accounts: Prepend user real name to "Account Activity" dialog title
Created attachment 331417 [details] [review] user-accounts: Drop overwritten title of "Account Activity" dialog Since we are setting the "Account Activity" title by prepending the user real name ("%s - Account Activity") in um-history-dialog.c, there's no need to set the title property for the dialog elsewhere.
Comment on attachment 331263 [details] [review] user-accounts: Rename "History" dialog title to "Account Activity" obsoleted by attachment 331417 [details] [review].
Created attachment 331422 [details] [review] user-accounts: Drop "Last Login" entry This info is already available in the "Account Activity" dialog. No need to duplicate it here.
Created attachment 331423 [details] [review] user-accounts: Show "Automatic Login" option only when supported We do not support "automatic login" for non local user accounts and accounts without a password.
(In reply to Felipe Borges from comment #114) > Created attachment 331422 [details] [review] [review] > user-accounts: Drop "Last Login" entry (In reply to Felipe Borges from comment #115) > Created attachment 331423 [details] [review] [review] > user-accounts: Show "Automatic Login" option only when supported I have Allan Day's ack for these two decisions. attachment 331415 [details] [review], attachment 331416 [details] [review], attachment 331417 [details] [review], attachment 331422 [details] [review], and attachment 331423 [details] [review] apply on top of git master.
Comment on attachment 331395 [details] [review] user-accounts: Do not show autologin option when it is impossible obsoleted by attachment 331423 [details] [review].
Comment on attachment 331396 [details] [review] user-accounts: Reposition fingerprint-login-button to correct position Attachment 331396 [details] pushed as 843c126 - user-accounts: Reposition fingerprint-login-button to correct position
Comment on attachment 331269 [details] [review] user-accounts: Drop unnecessary grid containing last-login label obsoleted by attachment 331422 [details] [review].
I have dropped all the Carousel and ArrowFrame patches because after a UI/UX discussion with Allan Day, we decided to make a few changes.
Created attachment 331437 [details] [review] user-accounts: Make the "Account Activity" dialog wider Set it to 60% of the parent window.
Comment on attachment 330139 [details] [review] user-accounts: Make the "Account Activity" wider obsoleted by attachment 331437 [details] [review].
Review of attachment 331415 [details] [review]: Looks good, though I think it would be better to merge this with attachment 331416 [details] [review] and attachment 331417 [details] [review].
Review of attachment 331416 [details] [review]: Looks good, just the title may be quite long in case of long name, but this is designers choice...
Review of attachment 331417 [details] [review]: Looks good!
Review of attachment 331422 [details] [review]: Ah, I have not realized that this field is missing in the mockup... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +415,3 @@ <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="valign">center</property> GTK_ALIGN_CENTER? @@ +420,3 @@ </object> <packing> + <property name="left_attach">2</property> Wouldn't be better to set this to 1 and set halign to GTK_ALIGN_END instead? ::: panels/user-accounts/um-user-panel.c @@ -832,3 @@ - time = act_user_get_login_time (user); - if (act_user_is_logged_in (user)) { - text = g_strdup (_("Logged in")); This seems doesn't work anyway...
Review of attachment 331423 [details] [review]: There is a problem with unwanted additional vertical space between password line and "Account Activity" button if "Automatic Login" is not shown... ::: panels/user-accounts/um-user-panel.c @@ +881,3 @@ gtk_switch_set_active (GTK_SWITCH (widget), act_user_get_automatic_login (user)); g_signal_handlers_unblock_by_func (widget, autologin_changed, d); + gtk_widget_set_visible (widget, autologin_possible); Hmm, shouldn't this be visible just for current user? Otherwise it can be visible also for other users including remote if d->permissions is NULL... but it is another bug...
Review of attachment 331437 [details] [review]: Why not if it is designers decision.
Just a note that you should probably merge "show_user" and "permission_changed" functions in wip branch, because you are not using "is_authorized" variable anymore...
(In reply to Ondrej Holy from comment #129) > Just a note that you should probably merge "show_user" and > "permission_changed" functions in wip branch, because you are not using > "is_authorized" variable anymore... The changes me and aday discussed include bringing back the Unlock approach.
Attachment 331415 [details] pushed as f56c90b - user-accounts: Drop unused subtitle in "Account Activity" dialog Attachment 331416 [details] pushed as f029fc0 - user-accounts: Prepend user real name to "Account Activity" dialog title Attachment 331417 [details] pushed as 094447f - user-accounts: Drop overwritten title of "Account Activity" dialog Attachment 331437 [details] pushed as 079928d - user-accounts: Make the "Account Activity" dialog wider
Created attachment 331468 [details] [review] user-accounts: Drop "Last Login" entry This info is already available in the "Account Activity" dialog. No need to duplicate it here.
Comment on attachment 331468 [details] [review] user-accounts: Drop "Last Login" entry After discussions with designers, the panel mockup was updated https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/users/future/users-panel-wires.png These changes include the use of the "Last Login" entry to launch the "Account Activity" dialog instead of the "Account Activity" button.
Created attachment 331568 [details] [review] user-accounts: Set normal relief for UmEditableButtons According to the latest mockups, the option buttons should have relief.
Created attachment 331569 [details] [review] user-accounts: Properly align option buttons
Created attachment 331570 [details] [review] user-accounts: Drop "Account Activity" button Now the "Account Activity" dialog is launched by clicking in the "Last Login" option.
Created attachment 331571 [details] [review] user-accounts: Use sensitivity to denote changes in "Account Type" Instead of changing from "Account Type" buttons to label, set the sensitivity of the buttons according to the current users' permission.
Created attachment 331572 [details] [review] user-accounts: Drop UmEditableButton, use GtkButtons instead Since we are denoting the permission to edit an option by setting the option sensitivity.
Created attachment 331578 [details] [review] user-accounts: Reposition "Remove Account" button It used to be at the toolbar in the bottom of the treeview. Now it is fixed in the bottom-right corner of the panel.
Created attachment 331579 [details] [review] user-accounts: Do not show "Remove Account" button for only-admin If there's a single administrator user, hide the "Remove Account" button.
Review of attachment 331568 [details] [review]: This doesn't make sense if UmEditableButtons is replaced by GtkButton in the following patch...
Review of attachment 331569 [details] [review]: ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +112,3 @@ <property name="column_spacing">10</property> <property name="row_spacing">10</property> + <property name="halign">GTK_ALIGN_CENTER</property> It seems it is useless in a current state...
Review of attachment 331570 [details] [review]: This change seems pretty confusing to me, do designers really wants this?
Review of attachment 331571 [details] [review]: Do what you have in a commit description - change sensitivity, not visibility... account type should be always shown, see the mockup.
Review of attachment 331572 [details] [review]: The UmEditableEntry for a real name should be replaced by GtkEntry also and then I think we can finally remove UmEditable* widgets completely... The button labels are wrongly aligned!
Review of attachment 331578 [details] [review]: Doesn't make sense without the carousel... also the panel is changing its width when switching between users!
Review of attachment 331579 [details] [review]: Doesn't make sense without the previous patch...
(In reply to Ondrej Holy from comment #143) > Review of attachment 331570 [details] [review] [review]: > > This change seems pretty confusing to me, do designers really wants this? yes.
(In reply to Ondrej Holy from comment #146) > Review of attachment 331578 [details] [review] [review]: > > Doesn't make sense without the carousel... also the panel is changing its > width when switching between users! The Add User button would be moved to the header bar in the next patch. The point of this commit is to create a transition state towards the new design, so you can review it change by change.
Review of attachment 331570 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +973,1 @@ gtk_widget_set_visible (widget, show); previous two lines are redundant
Review of attachment 331578 [details] [review]: (In reply to Felipe Borges from comment #149) > (In reply to Ondrej Holy from comment #146) > > Review of attachment 331578 [details] [review] [review] [review]: > > > > Doesn't make sense without the carousel... also the panel is changing its > > width when switching between users! > > The Add User button would be moved to the header bar in the next patch. Sorry for the reject, I have just wanted to be sure that you won't push such patches before you propose the following changes... > The point of this commit is to create a transition state towards the new > design, so you can review it change by change. Anyway, the changing width of the panel is unacceptable...
Review of attachment 331579 [details] [review]: Looks good, although the visibility changes are related to width changes...
Just a note that show_user() has to set some reasonable defaults and should not rely on on_permission_changed(). I afraid that the panel doesn't work properly if d->permissions is NULL currently, however I haven't tested it. Would be nice to check and fix if needed...
(In reply to Ondrej Holy from comment #144) > Review of attachment 331571 [details] [review] [review]: > > Do what you have in a commit description - change sensitivity, not > visibility... account type should be always shown, see the mockup. In the up-to-date mockups, we don't show the Account type buttons when the downgrade would_demote_only_admin ().
(In reply to Felipe Borges from comment #154) > (In reply to Ondrej Holy from comment #144) > > Review of attachment 331571 [details] [review] [review] [review]: > > > > Do what you have in a commit description - change sensitivity, not > > visibility... account type should be always shown, see the mockup. > > In the up-to-date mockups, we don't show the Account type buttons when the > downgrade would_demote_only_admin (). Hmm, is somewhere newer mockup than the following? https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/users/future/users-panel-wires.png ...because the account type switcher is always shown there if multiple user accounts are available. It is hidden just only if only one user is there. Am I overlooking something?
(In reply to Ondrej Holy from comment #155) > (In reply to Felipe Borges from comment #154) > > (In reply to Ondrej Holy from comment #144) > > > Review of attachment 331571 [details] [review] [review] [review] [review]: > > > > > > Do what you have in a commit description - change sensitivity, not > > > visibility... account type should be always shown, see the mockup. > > > > In the up-to-date mockups, we don't show the Account type buttons when the > > downgrade would_demote_only_admin (). > > Hmm, is somewhere newer mockup than the following? > https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/ > system-settings/users/future/users-panel-wires.png > > ...because the account type switcher is always shown there if multiple user > accounts are available. It is hidden just only if only one user is there. Am > I overlooking something? Yes, that's pretty much what you can read from the mockups. But I spoke do Allan at the time and he expressed that it makes no sense to show it when it would cause the administrator to downgrade itself.
Created attachment 331681 [details] [review] user-accounts: Properly align option buttons
Created attachment 331682 [details] [review] user-accounts: Drop "Account Activity" button Now the "Account Activity" dialog is launched by clicking in the "Last Login" option.
Created attachment 331683 [details] [review] user-accounts: Use sensitivity to denote changes in "Account Type" Instead of changing from "Account Type" buttons to label, set the sensitivity of the buttons according to the current users' permission.
Created attachment 331684 [details] [review] user-accounts: Drop UmEditableButton, use GtkButtons instead Since we are denoting the permission to edit an option by setting the option sensitivity.
Created attachment 331700 [details] [review] user-accounts: Show "Automatic Login" option only when supported We do not support "automatic login" for non local user accounts and accounts without a password.
(In reply to Felipe Borges from comment #161) > Created attachment 331700 [details] [review] [review] > user-accounts: Show "Automatic Login" option only when supported > > We do not support "automatic login" for non local user accounts > and accounts without a password. (In reply to Ondrej Holy from comment #127) > Review of attachment 331423 [details] [review] [review]: > > There is a problem with unwanted additional vertical space between password > line and "Account Activity" button if "Automatic Login" is not shown... > I guess this problem is obsolete now.
Review of attachment 331681 [details] [review]: Looks good, but would be nice to push together with other patches...
Review of attachment 331682 [details] [review]: Looks good, but would be nice to push together with other patches...
Review of attachment 331683 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +896,3 @@ + label = get_widget (d, "account-type-label"); + widget = get_widget (d, "account-type-box"); + if (act_user_get_account_type (user) == ACT_USER_ACCOUNT_TYPE_ADMINISTRATOR) { I don't understand this if-else code. Why shouldn't administrators see the account type options? The condition should be rather something like the following: if (!act_user_is_local_account (user) || !would_demote_only_admin (user)) and we should also make the widget insensitive by default. @@ +1403,2 @@ if (!act_user_is_local_account (user)) { + gtk_widget_set_sensitive (get_widget (d, "account-type-box"), FALSE); Although I think that the widget is important to tell users, whether they have admin rights, or not... I am willing to accept the designers choice to hide the whole widget if an account type can't be changed for some reason. But then I am convinced that we should hide the widget in all cases, when it is not possible to change the type, so also for remote users... @@ +1410,3 @@ if (would_demote_only_admin (user)) { + gtk_widget_set_visible (get_widget (d, "account-type-label"), FALSE); + gtk_widget_set_visible (get_widget (d, "account-type-box"), FALSE); I think that the visibility should be already set correctly in show_user() function and it is not necessary to change it here... @@ +1413,3 @@ } else { + gtk_widget_set_visible (get_widget (d, "account-type-label"), TRUE); + gtk_widget_set_visible (get_widget (d, "account-type-box"), TRUE); dtto
Review of attachment 331684 [details] [review]: Label in language button has wrong "padding" from some reason... ::: panels/user-accounts/Makefile.am @@ -48,3 @@ run-passwd.c \ - um-editable-button.h \ - um-editable-button.c \ Would be nice to remove the widget in a subsequent commit. This could be useful if we would want to restore the widgets in the future from some reason... ::: panels/user-accounts/um-fingerprint-dialog.c @@ +217,2 @@ } + gtk_widget_set_halign (gtk_bin_get_child (GTK_BIN (button)), GTK_ALIGN_START); Would be better to change this in .ui file... it applies on the all other occurrences...
I would like to see a patch to replace UmEditableEntry by GtkEntry for a real name also, because it would be nice to push it together with your latest four patches...
(In reply to Ondrej Holy from comment #165) > Review of attachment 331683 [details] [review] [review]: > > ::: panels/user-accounts/um-user-panel.c > @@ +896,3 @@ > + label = get_widget (d, "account-type-label"); > + widget = get_widget (d, "account-type-box"); > + if (act_user_get_account_type (user) == > ACT_USER_ACCOUNT_TYPE_ADMINISTRATOR) { > > I don't understand this if-else code. Why shouldn't administrators see the > account type options? The condition should be rather something like the > following: > if (!act_user_is_local_account (user) || !would_demote_only_admin (user)) > > and we should also make the widget insensitive by default. right. my bad. This was a legacy from the previous approach with the stacks. > > @@ +1403,2 @@ > if (!act_user_is_local_account (user)) { > + gtk_widget_set_sensitive (get_widget (d, > "account-type-box"), FALSE); > > Although I think that the widget is important to tell users, whether they > have admin rights, or not... I am willing to accept the designers choice to > hide the whole widget if an account type can't be changed for some reason. > But then I am convinced that we should hide the widget in all cases, when it > is not possible to change the type, so also for remote users... I will discuss it further with the designers. Thanks for the reviews so far! :)
Review of attachment 331700 [details] [review]: (In reply to Felipe Borges from comment #162) > (In reply to Felipe Borges from comment #161) > > Created attachment 331700 [details] [review] [review] [review] > > user-accounts: Show "Automatic Login" option only when supported > > > > We do not support "automatic login" for non local user accounts > > and accounts without a password. > > (In reply to Ondrej Holy from comment #127) > > Review of attachment 331423 [details] [review] [review] [review]: > > > > There is a problem with unwanted additional vertical space between password > > line and "Account Activity" button if "Automatic Login" is not shown... > > > > I guess this problem is obsolete now. I don't think so. Something like the following is needed: @@ -918 +918,2 @@ show_user (ActUser *user, CcUserPanelPrivate *d) - gtk_widget_set_sensitive (widget, autologin_possible); + widget = get_widget (d, "autologin-box"); + gtk_widget_set_visible (widget, autologin_possible); @@ -948,7 +948,0 @@ show_user (ActUser *user, CcUserPanelPrivate *d) - /* Autologin: show when local account */ - widget = get_widget (d, "autologin-box"); - label = get_widget (d, "autologin-label"); - show = act_user_is_local_account (user); - gtk_widget_set_visible (widget, show); - gtk_widget_set_visible (label, show); -
Created attachment 331804 [details] [review] user-accounts: Use sensitivity to denote changes in "Account Type" Instead of changing from "Account Type" buttons to label, set the sensitivity of the buttons according to the current users' permission.
Created attachment 331805 [details] [review] user-accounts: Drop usage of UmEditableButtons Now using GtkButtons instead. Since we are denoting the permission to edit an option by setting the option sensitivity.
Created attachment 331806 [details] [review] user-accounts: Remove UmEditableButton object To bring it back, revert this commit.
Created attachment 331807 [details] [review] user-accounts: Use GtkEntry instead of CcEditableEntry
Created attachment 331808 [details] [review] user-accounts: Show "Automatic Login" option only when supported We do not support "automatic login" for non local user accounts and accounts without a password.
Comment on attachment 331684 [details] [review] user-accounts: Drop UmEditableButton, use GtkButtons instead obsoleted by attachment 331805 [details] [review] and attachment 331806 [details] [review].
Created attachment 331819 [details] [review] user-accounts: Move "Add User" button to header bar This also introduces a change to the Lock/Unlock logic. From now on, Unlocking the panel causes the "Lock" button to turn into the "Add User" button.
(In reply to Felipe Borges from comment #176) > Created attachment 331819 [details] [review] [review] > user-accounts: Move "Add User" button to header bar > > This also introduces a change to the Lock/Unlock logic. From now > on, Unlocking the panel causes the "Lock" button to turn into the > "Add User" button. this approach was decided after discussed with designers.
Review of attachment 331804 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +896,3 @@ + label = get_widget (d, "account-type-label"); + widget = get_widget (d, "account-type-box"); + if (act_user_is_local_account (user) || !would_demote_only_admin (user)) { I am sorry to bother you again, but... Citation from #gnome-design: aday feborges: the idea was to only show it when there's more than one user, and to make it insensitive when the panel is locked My understanding is that it should be hidden on one-user-system only, so it should be done exactly as it is in the mockup. Or was there any other discussion with aday? So it should be something like: if (d->other_accounts == 0)... Then you have to be also sure that show_user is called, when new user is added/removed. @@ +902,3 @@ + gtk_widget_set_visible (label, FALSE); + gtk_widget_set_visible (widget, FALSE); + } You should also set the widget to be insensitive by default probably. @@ +1410,3 @@ if (would_demote_only_admin (user)) { + gtk_widget_set_visible (get_widget (d, "account-type-label"), FALSE); + gtk_widget_set_visible (get_widget (d, "account-type-box"), FALSE); Consequently, you should set only sensitivity here, not visibility...
Review of attachment 331805 [details] [review]: Language button label has wrong "left padding" in some cases, I can debug it further if you can't reproduce... ::: panels/user-accounts/um-fingerprint-dialog.c @@ +217,2 @@ } + gtk_widget_set_halign (gtk_bin_get_child (GTK_BIN (button)), GTK_ALIGN_START); Would be better to change this in .ui file... it applies on the all other occurrences... ::: panels/user-accounts/um-user-panel.c @@ -1723,2 @@ type = cc_editable_entry_get_type (); - type = um_editable_combo_get_type (); This should be probably in a separate patch and would be nice to remove also the UmEditableCombo object in another patch...
Review of attachment 331806 [details] [review]: Looks good!
Review of attachment 331807 [details] [review]: ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +186,3 @@ <property name="width-chars">30</property> <property name="max-width-chars">30</property> + <property name="valign">center</property> I prefer GTK_ALIGN_CENTER, however I've thought that GTK_ALIGN_CENTER is used by Glade by default also, but it isn't, so probably it does not matter, because the file is mix of both approaches... ::: panels/user-accounts/um-user-panel.c @@ +1632,2 @@ button = get_widget (d, "full-name-entry"); + g_signal_connect (button, "changed", G_CALLBACK (change_name_done), d); This causes hangs when writing quickly, it should be called probably on activate and focus-out-event events...
Review of attachment 331808 [details] [review]: Labels on the left side decrease its width a bit if the autologin widget is initially shown and then you switch users and the widget changes its visibility to hidden. It seems that the size group doesn't care about hidden widgets, however it should care as far as I know. The problem is that the width is not changed back after the widget is shown again. Maybe this is bug in GTK+. ::: panels/user-accounts/um-user-panel.c @@ +909,3 @@ gtk_widget_set_sensitive (widget, enable); + autologin_possible = get_autologin_possible (user); Maybe would be better to re-use "show" variable instead of adding another one...
Created attachment 331869 [details] [review] user-accounts: Unify size of headerbar buttons (In reply to Ondrej Holy from comment #93) > (snip) > > - Account activity: The arrow buttons has a different shapes... Attached patch unifies the different sizes. > - Account activity: Dialog is resized and also moved if there is too much > records per day. It used to be scrollbar there, so the scrollbar should be > restored, or the window should not move, when resizing... This is caused by changes in GTK+, see Bug 766569.
Created attachment 331890 [details] [review] user-accounts: Use sensitivity to denote changes in "Account Type" Instead of changing from "Account Type" buttons to label, set the sensitivity of the buttons according to the current users' permission. Now we hide the "Account Type" for a single user account.
Created attachment 331891 [details] [review] user-accounts: Show "Automatic Login" option only when supported We do not support "automatic login" for non local user accounts and accounts without a password.
Created attachment 331892 [details] [review] user-accounts: Remove UmEditableCombo class Revert this commit in order to bring the UmEditableCombo class back.
(In reply to Ondrej Holy from comment #179) > Review of attachment 331805 [details] [review] [review]: > > ::: panels/user-accounts/um-fingerprint-dialog.c > @@ +217,2 @@ > } > + gtk_widget_set_halign (gtk_bin_get_child (GTK_BIN (button)), > GTK_ALIGN_START); > > Would be better to change this in .ui file... it applies on the all other > occurrences... Since we are changing the label on the go, it would get the label defined at the .ui file replaced by the non-aligned one.
You are right, but why the hell gtk_button_set_label doesn't call gtk_label_set_label and replaces the whole widget :-( So I would rather change the widget as the following and set the halign in .ui file anyway if possible: gtk_label_set_label (GTK_LABEL (gtk_bin_get_child (GTK_BIN (button))), ...);
Review of attachment 331819 [details] [review]: Looks good to me, but would be nice to push together with "Remove button" patches...
Review of attachment 331890 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +896,3 @@ + /* Do not show the "Account Type" option when there's a single user account. */ + gtk_widget_set_visible (get_widget (d, "account-type-label"), d->other_accounts != 0); + gtk_widget_set_visible (get_widget (d, "account-type-box"), d->other_accounts != 0); It is almost perfect now! However show_user is called immediately, when first user account is added, so d->other_accounts is 0. Therefore account type widgets are not shown initially, even though there are more user accounts. So show_user has to be probably called always, when new user is added/removed. Also, I've learned from hadess that it is better to use additional variable as the following: show = (d->other_accounts != 0); Because then you can change the logic only on one line in the future if needed.
Review of attachment 331891 [details] [review]: Please see Comment 182, there is some problem with size groups probably, will attach screencast.
Review of attachment 331892 [details] [review]: Looks good!
Created attachment 331967 [details] Screencast of autologin label bug
Comment on attachment 331892 [details] [review] user-accounts: Remove UmEditableCombo class Attachment 331892 [details] pushed as 1345cf3 - user-accounts: Remove UmEditableCombo class
(In reply to Ondrej Holy from comment #188) > You are right, but why the hell gtk_button_set_label doesn't call > gtk_label_set_label and replaces the whole widget :-( So I would rather > change the widget as the following and set the halign in .ui file anyway if > possible: > gtk_label_set_label (GTK_LABEL (gtk_bin_get_child (GTK_BIN (button))), ...); I spoke to gtk+ devs and they don't want this behavior, considering the user could define undesirable label properties and such.
Created attachment 332140 [details] [review] user-accounts: Use GtkEntry instead of CcEditableEntry
(In reply to Ondrej Holy from comment #151) > Review of attachment 331578 [details] [review] [review]: > > (In reply to Felipe Borges from comment #149) > > (In reply to Ondrej Holy from comment #146) > > > Review of attachment 331578 [details] [review] [review] [review] [review]: > > > > > > Doesn't make sense without the carousel... also the panel is changing its > > > width when switching between users! > > > > The Add User button would be moved to the header bar in the next patch. > > Sorry for the reject, I have just wanted to be sure that you won't push such > patches before you propose the following changes... > > > The point of this commit is to create a transition state towards the new > > design, so you can review it change by change. > > Anyway, the changing width of the panel is unacceptable... It doesn't seem to change the width if applied after attachment 331682 [details] [review].
Review of attachment 331869 [details] [review]: Sure.
(In reply to Felipe Borges from comment #195) > (In reply to Ondrej Holy from comment #188) > > You are right, but why the hell gtk_button_set_label doesn't call > > gtk_label_set_label and replaces the whole widget :-( So I would rather > > change the widget as the following and set the halign in .ui file anyway if > > possible: > > gtk_label_set_label (GTK_LABEL (gtk_bin_get_child (GTK_BIN (button))), ...); > > I spoke to gtk+ devs and they don't want this behavior, considering the user > could define undesirable label properties and such. I could make the haligns all in a single patch so we could revert it if we manage to change the behavior of GtkButton in the future.
(In reply to Felipe Borges from comment #199) > (In reply to Felipe Borges from comment #195) > > (In reply to Ondrej Holy from comment #188) > > > You are right, but why the hell gtk_button_set_label doesn't call > > > gtk_label_set_label and replaces the whole widget :-( So I would rather > > > change the widget as the following and set the halign in .ui file anyway if > > > possible: > > > gtk_label_set_label (GTK_LABEL (gtk_bin_get_child (GTK_BIN (button))), ...); > > > > I spoke to gtk+ devs and they don't want this behavior, considering the user > > could define undesirable label properties and such. > > I could make the haligns all in a single patch so we could revert it if we > manage to change the behavior of GtkButton in the future. My understanding is that they don't want to reuse the same label from gtk_button_set_label for sure, because: "mclasen who knows what kind of misshapen label the user put there..." However you can still operate on that label yourself: "mclasen if you want to control the label widget, set a label widget, and operate on that" So you can do what I suggested last time, or even maybe better you can add child label in .ui file and use gtk_label_set_label (get_widget (d, "button-label"), name): <object class="GtkButton" id="button"> ... </object> <child> <object class="GtkLabel" id="button-label"> <property name="xalign">0</property> ... Or am I wrong? I would like just to separate UI from code as much as possible...
Comment on attachment 331869 [details] [review] user-accounts: Unify size of headerbar buttons commit 90d6f3b622165bbb868756704a9c22d894cdfa6c
Created attachment 332203 [details] Screencast of "remove accout" bug (In reply to Felipe Borges from comment #197) > (In reply to Ondrej Holy from comment #151) > > Review of attachment 331578 [details] [review] [review] [review]: > > > > (In reply to Felipe Borges from comment #149) > > > (In reply to Ondrej Holy from comment #146) > > > > Review of attachment 331578 [details] [review] [review] [review] [review] [review]: > > > > > > > > Doesn't make sense without the carousel... also the panel is changing its > > > > width when switching between users! > > > > > > The Add User button would be moved to the header bar in the next patch. > > > > Sorry for the reject, I have just wanted to be sure that you won't push such > > patches before you propose the following changes... > > > > > The point of this commit is to create a transition state towards the new > > > design, so you can review it change by change. > > > > Anyway, the changing width of the panel is unacceptable... > > It doesn't seem to change the width if applied after attachment 331682 [details] [review] > [details] [review]. I would say that attachment 331682 [details] [review] is pretty irrelevant. The problem is that the button is not always visible. So it is also caused by the subsequent attachment 331579 [details] [review].
Review of attachment 332140 [details] [review]: Looks good though the name is not changed if you click on another user. Probably the focus-out-event is called too late. However this is the same behavioral as before, so ok. Please push together with CcEditableButton->GtkButton patch...
Review of attachment 331578 [details] [review]: It seems that the width changes are caused only by the subsequent patch, so I am ok to push this one together with "Add User" patch...
Comment on attachment 331579 [details] [review] user-accounts: Do not show "Remove Account" button for only-admin We can skip this patch for now, or maybe we can change the visibility as per d->other_accounts, similar to account type. So the panel will be resized only when second user is added/removed - this might be acceptable...
Created attachment 332207 [details] [review] user-accounts: Drop usage of UmEditableButtons Now using GtkButtons instead. Since we are denoting the permission to edit an option by setting the option sensitivity.
Attachment 331578 [details] pushed as cbe31d5 - user-accounts: Reposition "Remove Account" button Attachment 331681 [details] pushed as e18c001 - user-accounts: Properly align option buttons Attachment 331819 [details] pushed as eb9c110 - user-accounts: Move "Add User" button to header bar
Created attachment 332212 [details] [review] user-accounts: Do not access already removed toolbar Commit eb9c110 removed add-remove-toolbar, however, some leftovers are in the code which causes the following errors: Gtk-CRITICAL **: gtk_widget_get_style_context: assertion 'GTK_IS_WIDGET (widget)' failed Gtk-CRITICAL **: gtk_style_context_set_junction_sides: assertion 'GTK_IS_STYLE_CONTEXT (context)' failed
Review of attachment 332207 [details] [review]: Unfortunately there are still some issues... I have already figured out, why language button is sometimes wrongly aligned. It happens when we try to set NULL as label (when language is not set yet). We have to add something like the following (this was handled by the UmEditableButton before): @@ -927,3 +927,6 @@ show_user (ActUser *user, CcUserPanelPrivate *d) name = gnome_get_language_from_locale (lang, NULL); + } else { + name = g_strdup ("—"); } + gtk_label_set_label (GTK_LABEL (widget), name); ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +245,2 @@ <property name="visible">True</property> <property name="hexpand">True</property> "text-button" style class needs to be added for those buttons in order to have same "padding" with other buttons. @@ +248,3 @@ + <object class="GtkLabel" id="account-password-button-label"> + <property name="visible">True</property> + <property name="halign">GTK_ALIGN_START</property> I wonder whether xalign shouldn't be used rather than halign... but it is probably "equal" in this case. ::: panels/user-accounts/um-fingerprint-dialog.c @@ +178,2 @@ gboolean +set_fingerprint_label (GtkWidget *button) It would be really nice to pass into just the label... @@ +211,3 @@ if (fingers == NULL || g_variant_iter_n_children (fingers) == 0) { is_disable = FALSE; + gtk_button_set_label (GTK_BUTTON (button), _("Disabled")); ...and use gtk_label_set_label as on other places... @@ +217,2 @@ } + gtk_widget_set_halign (gtk_bin_get_child (GTK_BIN (button)), GTK_ALIGN_START); ...to avoid this.
Review of attachment 332212 [details] [review]: sure sure, sorry for leaving this behind.
Comment on attachment 332212 [details] [review] user-accounts: Do not access already removed toolbar Attachment 332212 [details] pushed as 8e8d005 - user-accounts: Do not access already removed toolbar
(In reply to Ondrej Holy from comment #209) > Review of attachment 332207 [details] [review] [review]: > > ::: panels/user-accounts/um-fingerprint-dialog.c > @@ +178,2 @@ > gboolean > +set_fingerprint_label (GtkWidget *button) > > It would be really nice to pass into just the label... > > @@ +211,3 @@ > if (fingers == NULL || g_variant_iter_n_children (fingers) == 0) { > is_disable = FALSE; > + gtk_button_set_label (GTK_BUTTON (button), _("Disabled")); > > ...and use gtk_label_set_label as on other places... > > @@ +217,2 @@ > } > + gtk_widget_set_halign (gtk_bin_get_child (GTK_BIN (button)), > GTK_ALIGN_START); > > ...to avoid this. I agree. I will do it in another patch.
Created attachment 332263 [details] [review] user-accounts: Drop usage of UmEditableButtons Now using GtkButtons instead. Since we are denoting the permission to edit an option by setting the option sensitivity.
Review of attachment 331682 [details] [review]: I didn't realize it at the first time, but the button label is wrongly aligned also here... :-/
Review of attachment 332263 [details] [review]: Looks good otherwise... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +462,3 @@ + <style> + <class name="text-button"/> + </style> This is probably unwanted, however would be nice to have this as part of attachment 331682 [details] [review].
(In reply to Ondrej Holy from comment #191) > Review of attachment 331891 [details] [review] [review]: > > Please see Comment 182, there is some problem with size groups probably, > will attach screencast. Hmm, a similar problem has appeared with account-fingerprint-label after commit 5e2ed8e. This attachment makes it just worse, because also autologin-label has a wrong size. I don't see anything wrong there, it seems to me that there is a bug in GtkGrid/GtkSizeGroup...
(In reply to Ondrej Holy from comment #216) > (In reply to Ondrej Holy from comment #191) > > Review of attachment 331891 [details] [review] [review] [review]: > > > > Please see Comment 182, there is some problem with size groups probably, > > will attach screencast. > > Hmm, a similar problem has appeared with account-fingerprint-label after > commit 5e2ed8e. This attachment makes it just worse, because also > autologin-label has a wrong size. I don't see anything wrong there, it seems > to me that there is a bug in GtkGrid/GtkSizeGroup... Hmm this is probably duplicate of Bug 767076, which is closed as WONTFIX. ignore-hidden property is broken and should not be used :-( "Deprecated: 3.22: Measuring the size of hidden widgets has not worked reliably for a long time. In most cases, they will report a size of 0 nowadays, and thus, their size will not affect the other size group members. In effect, size groups will always operate as if this property was %TRUE. Use a #GtkStack instead to hide widgets while still having their size taken into account."
Created attachment 332276 [details] [review] user-accounts: Drop "Account Activity" button Now the "Account Activity" dialog is launched by clicking in the "Last Login" option.
Created attachment 332277 [details] [review] user-accounts: Drop usage of UmEditableButtons Now using GtkButtons instead. Since we are denoting the permission to edit an option by setting the option sensitivity.
(In reply to Ondrej Holy from comment #217) > (In reply to Ondrej Holy from comment #216) > > (In reply to Ondrej Holy from comment #191) > > > Review of attachment 331891 [details] [review] [review] [review] [review]: > > > > > > Please see Comment 182, there is some problem with size groups probably, > > > will attach screencast. > > > > Hmm, a similar problem has appeared with account-fingerprint-label after > > commit 5e2ed8e. This attachment makes it just worse, because also > > autologin-label has a wrong size. I don't see anything wrong there, it seems > > to me that there is a bug in GtkGrid/GtkSizeGroup... > > Hmm this is probably duplicate of Bug 767076, which is closed as WONTFIX. > ignore-hidden property is broken and should not be used :-( > > "Deprecated: 3.22: Measuring the size of hidden widgets has not worked > reliably for a long time. In most cases, they will report a size of 0 > nowadays, and thus, their size will not affect the other size group members. > In effect, size groups will always operate as if this property was %TRUE. > Use a #GtkStack instead to hide widgets while still having their size taken > into account." #gtk+: oholy mclasen, I am afraid that we don't want to add several GtkStacks to workaround this... isn't there another way? mclasen oholy: there's no other way around it that I'm aware of
Review of attachment 332276 [details] [review]: Looks good, please push once the attachment 331890 [details] [review] is ready.
Review of attachment 332277 [details] [review]: Look good!
Created attachment 332351 [details] [review] user-accounts: Use sensitivity to denote changes in "Account Type" Instead of changing from "Account Type" buttons to label, set the sensitivity of the buttons according to the current users' permission. Now we hide the "Account Type" for a single user account.
Review of attachment 332351 [details] [review]: Looks good, I think we can now push it also with other patches...
Although it would be nice to propose patch for the user icon also ;-) Also wasn't there patch expanding the account type buttons?
Created attachment 332353 [details] [review] user-accounts: Fix history dialog height Here is a workaround for the history dialog height...
Review of attachment 331365 [details] [review]: ::: panels/user-accounts/data/user-accounts-dialog.ui @@ -72,3 @@ <property name="visible">True</property> <property name="can_focus">True</property> - <property name="hexpand">True</property> Hmm, those hexpands shouldn't have been removed probably...
Attachment 331806 [details] pushed as a053750 - user-accounts: Remove UmEditableButton object Attachment 332277 [details] pushed as e2d3b47 - user-accounts: Drop usage of UmEditableButtons
Comment on attachment 332140 [details] [review] user-accounts: Use GtkEntry instead of CcEditableEntry Attachment 332140 [details] pushed as 1039642 - user-accounts: Use GtkEntry instead of CcEditableEntry
Comment on attachment 332276 [details] [review] user-accounts: Drop "Account Activity" button Attachment 332276 [details] pushed as e36a365 - user-accounts: Drop "Account Activity" button
Comment on attachment 332351 [details] [review] user-accounts: Use sensitivity to denote changes in "Account Type" Attachment 332351 [details] pushed as e3148af - user-accounts: Use sensitivity to denote changes in "Account Type"
Created attachment 332463 [details] [review] user-accounts: Introduce arrow frame (UmArrowFrame) Specialized type of GtkFrame which will point to an item in an UmCarousel (to be introduced).
Created attachment 332464 [details] [review] user-accounts: Fix horizontal alignment of "Account Type" buttons It makes "account-type-standard" and "account-type-admin" hexpand properties to equal TRUE.
Created attachment 332465 [details] [review] user-accounts: Drop CcEditableEntry references from um-user-panel There is no need to use this type in the new user accounts panel.
(In reply to Felipe Borges from comment #232) > Created attachment 332463 [details] [review] [review] > user-accounts: Introduce arrow frame (UmArrowFrame) > > Specialized type of GtkFrame which will point to an item in an > UmCarousel (to be introduced). I made it as simple as possible so we can gradually introduce it. I will add features and api as necessary when adding the carousel.
Review of attachment 332463 [details] [review]: um-arrow-frame.c is missing in the patch...
Review of attachment 332464 [details] [review]: Looks good!
Review of attachment 332465 [details] [review]: Looks good!
Created attachment 332476 [details] [review] user-accounts: Introduce arrow frame (UmArrowFrame) Specialized type of GtkFrame which will point to an item in an UmCarousel (to be introduced).
Attachment 332464 [details] pushed as 5c12abb - user-accounts: Fix horizontal alignment of "Account Type" buttons Attachment 332465 [details] pushed as 5b10194 - user-accounts: Drop CcEditableEntry references from um-user-panel
(In reply to Felipe Borges from comment #239) > Created attachment 332476 [details] [review] [review] > user-accounts: Introduce arrow frame (UmArrowFrame) > > Specialized type of GtkFrame which will point to an item in an > UmCarousel (to be introduced). If the class is final and you define the struct inside the .c file, you don't have to add a private field.
Review of attachment 332353 [details] [review]: Sure. ::: configure.ac @@ +88,2 @@ GLIB_REQUIRED_VERSION=2.44.0 +GTK_REQUIRED_VERSION=3.21.3 commit 13b745aa3df77c347a98ae8b2ef137ed8bc20483 already bumped GTK_REQUIRED_VERSION.
Comment on attachment 332353 [details] [review] user-accounts: Fix history dialog height Attachment 332353 [details] pushed as 77c26aa - user-accounts: Fix history dialog height
Created attachment 332795 [details] [review] user-accounts: Introduce carousel (UmCarousel) UmCarousel is a widget directly tied to UmArrowFrame. It is not designed to be populated directly as a container but to be binded to a GListModel.
Created attachment 332796 [details] [review] user-accounts: Hide UmCarousel when there's just a single user
Created attachment 332797 [details] [review] user-accounts: Just set arrow-frame's carousel once
Created attachment 332883 [details] [review] user-accounts: Use user_uid to index users instead of position This prevents wrong mapping when removing/adding/sorting users.
Review of attachment 332476 [details] [review]: I think that all of the UmArrowFrame functionality (which is independent on UmCarousel at least) from the following patch should be already part of this patch, otherwise it doesn't make sense to me to make this patch separately... More or less generic frame is proposed here, but the following patch makes it carousel specific, so maybe you should rename it to UmCarouselFrame, or make it really generic... ::: panels/user-accounts/um-arrow-frame.c @@ +29,3 @@ + GtkFrame parent; + + UmArrowFramePrivate *priv; This is pretty redundant if you are going to remove it in the following patch... @@ +54,3 @@ + object_class->finalize = um_arrow_frame_finalize; + + gtk_widget_class_set_css_name (widget_class, "arrow-frame"); You should probably include also some css file, otherwise it is redundant, or is this some well-known class? ::: panels/user-accounts/um-arrow-frame.h @@ +16,3 @@ + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * Writen by: Felipe Borges <felipeborges@gnome.org>, Georges Basile Stavracas Neto <georges.stavracas@gmail.com>?
Review of attachment 332795 [details] [review]: I see the following if I remove user account (it seems to me that you are just adding new pages, but not removing them): (gnome-control-center:14622): Gtk-CRITICAL **: gtk_entry_set_text: assertion 'text != NULL' failed (gnome-control-center:14622): AccountsService-WARNING **: ActUserManager: user (null) has no username (uid: 0) Also when I remove the fourth account, the arrows are still sensitive, and it segfaults if I click on them... I see some graphical glitches, I will attach screencast. Name should be updated in the carousel if it is changed. Please ask somebody (from GTK+) to look at those patches, I don't have time to dig into gtk/gdk/cairo internals right now... ::: panels/user-accounts/data/carousel.ui @@ +45,3 @@ + </child> + <signal name="clicked" handler="um_carousel_go_back_button_clicked" object="UmCarousel" swapped="no"/> + <signal name="clicked" handler="um_carousel_page_changed" object="UmCarousel" swapped="no"/> I personally prefer signals in code, not in ui file, however signals in ui files are used also in other panels, so ok... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +62,3 @@ </style> <child> + <object class="GtkBox" id="hbox2"> I am really happy that you want replace deprecated widgets, however would be nice to make it in the separate patch, because this is a bit mess... ::: panels/user-accounts/um-arrow-frame.c @@ +18,3 @@ * Writen by: Felipe Borges <felipeborges@gnome.org>, * Georges Basile Stavracas Neto <georges.stavracas@gmail.com> + * This should be in the initial UmArrowFrame patch... @@ +42,3 @@ + +static gint +um_arrow_frame__get_row_x (UmArrowFrame *frame) The double underscores look a bit weird to me, is it intentional? Is this covered by the gnome coding guidelines? @@ +121,3 @@ + * * Don't allow that gtk_render_background renders + * * anything out of (tip_x, start_y) (end_x, end_y). + * */ The comment is wrongly formatted. @@ +135,3 @@ + if (border_width > 0) + { +G_GNUC_BEGIN_IGNORE_DEPRECATIONS We should not use deprecated functions in the newly written codes... however this section can be fully removed probably, because you have already border_color set... @@ +377,3 @@ + gtk_style_context_add_provider (gtk_widget_get_style_context (GTK_WIDGET (self)), + provider, + GTK_STYLE_PROVIDER_PRIORITY_APPLICATION); Replace the tabs by spaces... @@ +409,3 @@ + if (carousel == NULL) + return; + assert (frame->carousel == NULL)? otherwise you should remove the signal handlers... ::: panels/user-accounts/um-carousel.c @@ +183,3 @@ + gtk_box_pack_end (GTK_BOX (box), item, TRUE, FALSE, 10); + } else { + GSequenceIter *iter; wrong indentation of this block @@ +203,3 @@ + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (self->current_button), TRUE); + + self->current_page = floor (position / ITEMS_PER_PAGE); um-carousel.c: In function ‘um_carousel_select_item’: um-carousel.c:208:26: warning: implicit declaration of function ‘floor’ [-Wimplicit-function-declaration] self->current_page = floor (position / ITEMS_PER_PAGE); ^~~~~ um-carousel.c:208:26: warning: incompatible implicit declaration of built-in function ‘floor’ um-carousel.c:208:26: note: include ‘<math.h>’ or provide a declaration of ‘floor’ @@ +234,3 @@ + widget = self->create_widget_func (item, self->create_widget_func_data); + + if (g_object_is_floating (widget)) um-carousel.c: In function ‘model_changed’: um-carousel.c:241:9: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation] if (g_object_is_floating (widget)) ^~ um-carousel.c:244:13: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’ gtk_widget_show (widget); ^~~~~~~~~~~~~~~ ::: panels/user-accounts/um-user-image.c @@ +68,2 @@ { UmUserImage *image = UM_USER_IMAGE (object); um-user-image.c: In function ‘um_user_image_finalize’: um-user-image.c:69:22: warning: unused variable ‘image’ [-Wunused-variable] UmUserImage *image = UM_USER_IMAGE (object); @@ -69,3 @@ UmUserImage *image = UM_USER_IMAGE (object); - g_clear_object (&image->priv->user); I don't see any reason, why we should remove this.... ::: panels/user-accounts/um-user-panel.c @@ +208,1 @@ + d->other_accounts++; You should rename it to total_accounts if you want change the logic, but I would be more happy if you preserve the previous logic. This variable is used to determine, whether some options should be shown or not and I am not really sure whether there are not some corner cases (e.g. when your user account is not yet added). @@ +249,1 @@ { This function should be renamed currently, because it just finishes the dialog, but do not select the user actually... @@ +260,3 @@ return; + d->selected_user = user; This seems pretty redundant, but I wonder what invokes show_user... @@ +1085,3 @@ + /* Present the first user account (administrator). */ + d->selected_user = ACT_USER (list->data); + show_user (d->selected_user, d); It seems to me that the two previous lines are redundant, because they should be invoked by the following line... @@ +1086,3 @@ + d->selected_user = ACT_USER (list->data); + show_user (d->selected_user, d); + um_carousel_select_item (d->carousel, 0); I am afraid that this code relies on that first added user is our user but I afraid that it shouldn't be...
Review of attachment 332796 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +208,2 @@ d->other_accounts++; + if (d->other_accounts > 1) { other == 1, total == 2 @@ +210,1 @@ um_arrow_frame_set_carousel (d->arrow_frame, d->carousel); Wouldn't be better to set this once in setup and toggle just visibility? @@ +232,3 @@ } + + if (d->other_accounts == 1) { other == 0, total == 1
Review of attachment 332797 [details] [review]: This should be merged into the previous patch.
Review of attachment 332883 [details] [review]: You should merge this into the initial UmCarousel patch, but it makes sense.
Created attachment 333050 [details] Screencast of carousel glitches
Comment on attachment 332353 [details] [review] user-accounts: Fix history dialog height Hmm, it seems that this patch can be reverted currently, I have been too active before...
(In reply to Ondrej Holy from comment #254) > Comment on attachment 332353 [details] [review] [review] > user-accounts: Fix history dialog height > > Hmm, it seems that this patch can be reverted currently, I have been too > active before... Sure. I pushed the revert commit to master as 4f48934b50eaf8ad0bd4735f04d46725544b18c4 Thanks Ondrej!
Created attachment 339932 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is a widget which allows switching between users.
Comment on attachment 332795 [details] [review] user-accounts: Introduce carousel (UmCarousel) I decided to rework the Carousel. It now doesn't touch the model and it is supposed to behave just like a container.
Attachment 339932 [details] might be a bit scary to review, but it is mostly removing the GtkTreeView machinery and plugin in the new Carousel widget. I decided to go for a "lazy" approach for user_removed, user_changed, assuming that these are very corner cases and that most of the systems doesn't have too many users. UmCarousel is supposed to be integrated with UmCarouselFrame, a GtkFrame with a arrow pointing to the current selected-user, but it also works independently.
Created attachment 339957 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is a widget which allows switching between users.
Review of attachment 339957 [details] [review]: Hmm, I've not seen the code yet, however, I tested the patch and see some problems, which would be nice to fix at first: - The whole panel is insensitive if you have only one user account, so you can't change anything... - Similar problem with multiple accounts, the panel is insensitive and you have to select the account in the carousel first. I suppose that such situation should never happen. - The go_back_button should be insensitive on the first stack page and go_next_button on the last stack page per mockup, shouldn't be? - The panel should be "centered" (i.e. free space should be on the left side of the panel). I don't really think that the position of "Remove Account" button is the good idea, but it is designers choice... - I think that the carousel should disappear if you remove a last account, but it doesn't. I would like to see UmCarouselFrame also... ::: panels/user-accounts/um-carousel.c @@ +142,3 @@ +um_carousel_finalize (GObject *object) +{ + UmCarousel *self = UM_CAROUSEL (object); um-carousel.c: In function ‘um_carousel_finalize’: um-carousel.c:144:21: warning: unused variable ‘self’ [-Wunused-variable] UmCarousel *self = UM_CAROUSEL (object);
Please don't forget that user icon should be also changed as per the mockup...
(In reply to Ondrej Holy from comment #260) > Review of attachment 339957 [details] [review] [review]: > > - The go_back_button should be insensitive on the first stack page and > go_next_button on the last stack page per mockup, shouldn't be? The idea here would be to go circularly. When you press the go_back_button in the first stack page, it would go to the last page. > - The panel should be "centered" (i.e. free space should be on the left side > of the panel). I don't really think that the position of "Remove Account" > button is the good idea, but it is designers choice... Right. It will come in the UmCarouselFrame.
Created attachment 340021 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is a widget which allows switching between users.
Review of attachment 340021 [details] [review]: The new user account is shown if you add a new user, but also the Carousel should show the corresponding page with that user, not the first one... ::: panels/user-accounts/um-carousel.c @@ +1,3 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- + * + * Copyright 2016 Red Hat, Inc, nitpick: Copyright (C) 2016 Red Hat, Inc.? @@ +39,3 @@ +G_DEFINE_TYPE (UmCarousel, um_carousel, GTK_TYPE_GRID) + +#define TEMPLATE_UI "/org/gnome/control-center/user-accounts/carousel.ui" It seems redundant to me... @@ +42,3 @@ + +#define ITEMS_PER_PAGE 3 +#define LAST_PAGE (self->num_items / ITEMS_PER_PAGE) The following error is printed if you have 6/9/... users and press the next button on the last page of stack: (gnome-control-center:5746): Gtk-WARNING **: Child name '2' not found in GtkStack I suppose that the LAST_PAGE macro should be modified in the following way: #define LAST_PAGE (self->num_items ? ((self->num_items - 1) / ITEMS_PER_PAGE) : 0); @@ +99,3 @@ + gboolean last_box_is_full; + + if (!GTK_IS_TOGGLE_BUTTON (widget)) { Is this needed for something? @@ +160,3 @@ +um_carousel_init (UmCarousel *self) +{ + self->num_items = 0; This shouldn't be needed... @@ +161,3 @@ +{ + self->num_items = 0; + self->visible_page = 0; dtto ::: panels/user-accounts/um-carousel.h @@ +1,3 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- + * + * Copyright 2016 Red Hat, Inc, dtto @@ +26,3 @@ +G_BEGIN_DECLS + +#define UM_TYPE_CAROUSEL (um_carousel_get_type()) nitpick: missing space before opening parentheses @@ +28,3 @@ +#define UM_TYPE_CAROUSEL (um_carousel_get_type()) + +G_DECLARE_FINAL_TYPE (UmCarousel, um_carousel, UM, CAROUSEL, GtkGrid) Shouldn't this be rather GtkContainer? ::: panels/user-accounts/um-user-panel.c @@ +162,3 @@ + uid_t uid; + + if (d->selected_user != NULL) { You can use g_clear_object instead of this whole segment... @@ +225,3 @@ } +static gint sort_users (gconstpointer a, gconstpointer b); I would move the declaration to the beginning of the file, or move the definition here... @@ -371,3 @@ - - if (current == user) { - show_user (user, d); I think that this still has to be called... @@ +1061,3 @@ gint result; + ua = (ActUser *)a; ACT_USER (a) @@ +1062,3 @@ + ua = (ActUser *)a; + ub = (ActUser *)b; dtto @@ +1066,1 @@ + name1 = g_utf8_collate_key (get_real_or_user_name (ua), -1); I would move this into the else branch... @@ +1079,3 @@ + g_free (name1); + g_free (name2); dtto @@ +1114,3 @@ g_signal_connect (d->um, "user-changed", G_CALLBACK (user_changed), d); g_signal_connect (d->um, "user-is-logged-in-changed", G_CALLBACK (user_changed), d); You can probably call reload_users here instead of the following code... @@ +1182,3 @@ + widget = get_widget (d, "main-user-vbox"); + gtk_widget_set_sensitive (widget, is_authorized); This change seems to be not related to the Carousel, so it should be probably in a separate patch? Does it work also for my user account? @@ +1453,3 @@ } + d->selected_user = NULL; This should not be needed...
(In reply to Ondrej Holy from comment #264) > Review of attachment 340021 [details] [review] [review]: > > @@ +99,3 @@ > + gboolean last_box_is_full; > + > + if (!GTK_IS_TOGGLE_BUTTON (widget)) { > > Is this needed for something? Yep. Gtk+ is actually calling the widget's parent "add" method (which we are overriding here) when defining any children of a widget in the template file. So this handler allows us to use the gtk_container_add method instead of creating an um_carousel_add api.
Created attachment 340847 [details] [review] user-accounts: Don't show_restart_notification when changing Language Since commit 5e2ed8e, the user-accounts panel does not present an option to change the Language for the current user. This should be done in the "Region & Language" panel instead. In doing so, the code for launching the restart notification is never reached in the language_response callback method.
Review of attachment 340847 [details] [review]: Looks good to me, thanks!
Comment on attachment 340847 [details] [review] user-accounts: Don't show_restart_notification when changing Language Attachment 340847 [details] pushed as b995e16 - user-accounts: Don't show_restart_notification when changing Language
Created attachment 341233 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children.
Review of attachment 341233 [details] [review]: Just some clarifications: ::: panels/user-accounts/um-carousel.c @@ +30,3 @@ +}; + +G_DEFINE_TYPE (UmCarouselItem, um_carousel_item, GTK_TYPE_TOGGLE_BUTTON) I choose GtkToggleButton for convenience of recycling the "toggled" signal and the style properties. @@ +139,3 @@ + + update_buttons_visibility (self); +} The um_carousel_find and um_carousel_select_item API are present to cover the corner case of "select_created_user", since UmCarousel doesn't keep track of the user list (because it is just a widget container), this API is useful to find the UmCarouselItem which represents the desired ActUser. @@ +200,3 @@ + GTK_CONTAINER_CLASS (um_carousel_parent_class)->add (container, widget); + return; + } Otherwise the <child> packing in the template would handle the toplevel children as UmCarouselItems. ::: panels/user-accounts/um-user-panel.c @@ +308,2 @@ static void user_changed (ActUserManager *um, ActUser *user, CcUserPanelPrivate *d) Since this happens quite often... @@ +319,2 @@ + child = gtk_bin_get_child (GTK_BIN (item)); + gtk_widget_destroy (child); ... instead of reloading the whole content, we just find the one UmCarouselItem that changed, remove its children... @@ +322,2 @@ + child = create_carousel_entry (d, user); + gtk_container_add (GTK_CONTAINER (item), child); ...and replace it with the new-changed-child.
In the following patch, I will introduce the Arrow.
Review of attachment 341233 [details] [review]: It seems that all the problems which I mentioned last time are fixed. I see another: - The newly added user account appears on the latest position, but I am convinced that the accounts should be sorted as it is done when deleting. - At the top of the panel is a quite a lot of free space if there is only one user account, I am not sure this is what we want...
(In reply to Ondrej Holy from comment #274) > Review of attachment 341233 [details] [review] [review]: > > It seems that all the problems which I mentioned last time are fixed. I see > another: > - The newly added user account appears on the latest position, but I am > convinced that the accounts should be sorted as it is done when deleting. sure. I'm going to reload the list when user_changed gets emitted too. > - At the top of the panel is a quite a lot of free space if there is only > one user account, I am not sure this is what we want... For the new control-center shell, this panel is going to be taller, so the user options should be vertically and horizontally aligned. See following patch.
Created attachment 341270 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 341271 [details] [review] user-accounts: Align user options and the "Remove Account" button These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Review of attachment 341270 [details] [review]: reload_users should be called also from change_name_done at least... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +101,1 @@ <property name="visible">True</property> I wonder why the following patch fixes the free space on the top of the panel, because it seems it is allocated by this widget even if the carousel is empty. Maybe the visibility should be set to False if only one account is shown... ::: panels/user-accounts/um-carousel.c @@ +79,3 @@ + return 0; + + return ((g_list_length (self->children)-1) / ITEMS_PER_PAGE); Nitpick: missing spaces around the minus sign @@ +81,3 @@ + return ((g_list_length (self->children)-1) / ITEMS_PER_PAGE); +} +#define LAST_PAGE get_last_page_number (self) Not sure whether this macro is really needed... @@ +209,3 @@ + g_signal_connect (widget, "toggled", G_CALLBACK (on_item_toggled), self); + + last_box_is_full = ((g_list_length (self->children)-1) % ITEMS_PER_PAGE == 0); Nitpick: missing spaces around the minus sign @@ +267,3 @@ + g_cclosure_marshal_VOID__OBJECT, + G_TYPE_NONE, 1, + UM_TYPE_CAROUSEL_ITEM); Nitpick: It is misaligned. ::: panels/user-accounts/um-user-panel.c @@ +154,3 @@ } +static void show_user (ActUser *user, CcUserPanelPrivate *d); I would move the declaration to the beginning of the file, but ok... @@ +1175,3 @@ + widget = get_widget (d, "main-user-vbox"); + gtk_widget_set_sensitive (widget, is_authorized); See my comment about this from the previous review...
Review of attachment 341271 [details] [review]: It should not be vertically aligned as per the mockup, it should be on the top. Also the placement of the "Remove Account" button isn't same as in the mockup, however, I think it is better to do it this way, otherwise there may be problems for some translations, where the button would be too wide... Please discuss this with designers... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +118,2 @@ <child> <object class="GtkVBox" id="main-user-vbox"> It seems that we can now merge the hbox2, main-user-vbox, and grid1...
(In reply to Ondrej Holy from comment #278) > Review of attachment 341270 [details] [review] [review]: > > reload_users should be called also from change_name_done at least... I think the name change already causes user_changed to be emitted (which does the reloading). > > ::: panels/user-accounts/data/user-accounts-dialog.ui > @@ +101,1 @@ > <property name="visible">True</property> > > I wonder why the following patch fixes the free space on the top of the > panel, because it seems it is allocated by this widget even if the carousel > is empty. Maybe the visibility should be set to False if only one account is > shown... The Carousel is embedded in a GtkRevealer. Setting its visibility would cause the sliding effect not to be visible. The bug here was that the "carousel-revealer" was aligned to fill vertically. Fixed in a following patch. > @@ +1175,3 @@ > > + widget = get_widget (d, "main-user-vbox"); > + gtk_widget_set_sensitive (widget, is_authorized); > > See my comment about this from the previous review... Moved to another patch.
(In reply to Ondrej Holy from comment #279) > Review of attachment 341271 [details] [review] [review]: > > It should not be vertically aligned as per the mockup, it should be on the > top. Also the placement of the "Remove Account" button isn't same as in the > mockup, however, I think it is better to do it this way, otherwise there may > be problems for some translations, where the button would be too wide... > Please discuss this with designers... Sure. I discussed with the designers on IRC and aligning vertically is the best option considering the translation problem that you pointed out and also to be visually more appealing (the fact that the button would grab more attention). > > ::: panels/user-accounts/data/user-accounts-dialog.ui > @@ +118,2 @@ > <child> > <object class="GtkVBox" id="main-user-vbox"> > > It seems that we can now merge the hbox2, main-user-vbox, and grid1... Sure. I merged them in a "user-options" container, implemented in a following patch.
Created attachment 341406 [details] [review] user-accounts: Set user options sensitive based on the permissions
Created attachment 341407 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 341408 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
(In reply to Felipe Borges from comment #284) > Created attachment 341408 [details] [review] [review] > user-accounts: Reorganize the user-options container > > This commit merges the hbox2, main-user-vbox, and grid1 into the > "user-options" container. > > It also replaces deprecated widgets, such as GtkVBox and GtkHBox. > > These changes are according to the new User Accounts panel mockups > at https://wiki.gnome.org/Design/SystemSettings/UserAccounts I am not a big fan of killing the git history of the file, but I guess this comes for good now. :)
Created attachment 341725 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Created attachment 341726 [details] [review] user-accounts: Select the first user also in the UmCarousel This makes the arrow point to the first user in the carousel as soon as the users are loaded.
Created attachment 341727 [details] [review] user-accounts: Select main user in the carousel when user_removed After the removal of a user, reset the carousel to the main user.
Created attachment 341728 [details] [review] user-accounts: Do not draw the arrow when the Carousel is hidden Since we hide the carousel when there's just a single user, do not draw the arrow.
Review of attachment 341406 [details] [review]: I am still missing some explanation why this change is done. We can't view/edit current user stuff without unlocking the panel with this patch, this is a significant change. So, I can't see e.g. my login history without unlocking. I am not really sure we want this, I think that the mockup doesn't show this...
Review of attachment 341407 [details] [review]: (In reply to Felipe Borges from comment #280) > (In reply to Ondrej Holy from comment #278) > > Review of attachment 341270 [details] [review] [review] [review]: > > > > reload_users should be called also from change_name_done at least... > > I think the name change already causes user_changed to be emitted (which > does the reloading). I don't see such behavior, the users are still sorted wrongly, not sure what is wrong... ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +114,3 @@ + </child> + <child> + <object class="GtkGrid" id="user-options"> Hmm... this changes should be probably part of the following patch.
Review of attachment 341408 [details] [review]: It seems that the user-accounts-dialog.ui changes are merged in the previous patch by mistake...
Review of attachment 341725 [details] [review]: Hmm, I don't really like this approach over CSS. I think that attachment 331270 [details] [review] does it better (similar to GtkPoppover). This arrow has to be already implemented somewhere. It is worth to discuss on #gtk+. Don't we have it in some other panel already? It would be nice to reuse the code if possible... ::: panels/user-accounts/data/carousel.css @@ +1,2 @@ +.arrow { + background: linear-gradient(45deg, transparent 50%, #D9D9D7 50%), Isn't the #D9D9D7 color defined somewhere in the theme? ::: panels/user-accounts/data/carousel.ui @@ +11,3 @@ <property name="visible">True</property> <property name="hexpand">True</property> + <property name="valign">GTK_ALIGN_START</property> Shouldn't this be rather GTK_ALIGN_FILL if any? @@ +16,3 @@ <object class="GtkStack" id="stack"> <property name="visible">True</property> + <property name="valign">GTK_ALIGN_START</property> dtto @@ +26,3 @@ <property name="orientation">horizontal</property> <property name="border_width">12</property> + <property name="valign">GTK_ALIGN_START</property> dtto ::: panels/user-accounts/um-carousel.c @@ +144,3 @@ + + gtk_widget_get_allocation (self->arrow, &alloc); + alloc.x = CLAMP (dest_x - ARROW_WIDTH, 0, gtk_widget_get_allocated_width (carousel)); The tip of the arrow doesn't point into the center of a user icon... I suppose that there should be ARROW_WIDTH / 2... ARROW_WIDTH needs to be also updated probably... @@ +269,3 @@ + cairo_t *cr) +{ + UmCarousel *self = UM_CAROUSEL (widget); Wrong indentation...
Review of attachment 341726 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +1124,3 @@ + item = um_carousel_find_item (d->carousel, user, user_compare); + + um_carousel_select_item (d->carousel, item); Wouldn't be better calling this always from show_user (or at least make some helper function for it)? It applies also on attachment 341407 [details] [review].
Review of attachment 341727 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +288,3 @@ g_slist_free (list); + + Unwanted whitespace change... @@ +312,3 @@ + item = um_carousel_find_item (d->carousel, user, user_compare); + um_carousel_select_item (d->carousel, item); + } Wouldn't be better calling this from show_user?
Review of attachment 341728 [details] [review]: ::: panels/user-accounts/um-user-panel.c @@ +312,3 @@ + um_carousel_select_item (d->carousel, item); + gtk_revealer_set_reveal_child (GTK_REVEALER (get_widget (d, "carousel-revealer")), + show_carousel); Why this change is needed?
(In reply to Ondrej Holy from comment #293) > Review of attachment 341725 [details] [review] [review]: > > Hmm, I don't really like this approach over CSS. I think that attachment > 331270 [details] [review] does it better (similar to GtkPoppover). This > arrow has to be already implemented somewhere. It is worth to discuss on > #gtk+. Don't we have it in some other panel already? It would be nice to > reuse the code if possible... The initial implementation was heavily inspired by the GtkPopover implementation. But after some recent discussions on #gtk+, I was informed that that way of implementing it is deprecated. GtkPopover draws its arrow using the whole cairo machinery and deprecated API such as gtk_style_context_get_border_color. See also https://mail.gnome.org/archives/gtk-app-devel-list/2016-December/msg00018.html I was advised to go for a CSS solution instead. Considering your preference for the ArrowFrame instead, I would then use CSS to draw the arrow on the top of it. My preference for handling it all inside the UmCarousel was mainly because I preferred not to overlay the user-options container in the top of the carousel (raising issues like size allocation and the height of the whole panel). What do you think?
Ok, I am not against going this way, I honor gtk+ devs, that's why I suggested discussing this on #gtk+. Sorry, I am not really suitable for reviewing the CSS stuff, please ask somebody else to do a review of this attachment...
Created attachment 342160 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342161 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342162 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Review of attachment 342162 [details] [review]: Some clarifications. ::: panels/user-accounts/data/carousel.css @@ +1,1 @@ +.carousel-arrow-container { This is a container which limits the movement of the arrow. Is just a box with the height of the arrow and the whole width of the carousel. @@ +3,3 @@ +} + +.carousel-inner-arrow { This is the real arrow. @@ +10,3 @@ +} + +.carousel-arrow { This box contains the arrow and allows us to draw the borders and animate the whole bunch. @@ +12,3 @@ +.carousel-arrow { + background: linear-gradient(45deg, transparent 50%, #D9D9D7 50%), + linear-gradient(-45deg, transparent 50%, #D9D9D7 50%), Unfortunately, #D9D9D7 is not defined in the theme but is wide used in classes such as location-bar. ::: panels/user-accounts/um-carousel.c @@ +24,3 @@ #include <gtk/gtk.h> +#define ARROW_WIDTH 28 This ~magic~ number comes from the arrow dimensions (36x36) discounted by the margin and padding of the css drawing. @@ +126,3 @@ + "* {\n" + " animation-name: arrow_keyframes-%d;\n" + Here we update the animation coordinates. @@ +184,3 @@ +static void +on_item_toggled (UmCarouselItem *item, Just moved this function up to avoid duplication of the signal emission. @@ +190,3 @@ + + if (self->selected_item != NULL) + self->arrow_start_x = um_carousel_item_get_x (self->selected_item, self); The animation needs a start and an end (destination). Therefore we save the last item's x position to be the start. If there's no previous item (first item added, or item is deleted leaving just one), we skip the animation and move the arrow directly. @@ +361,3 @@ + g_object_unref (provider); + + gtk_style_context_add_provider_for_screen (gdk_screen_get_default (), Since the allocated size is not computed for the other stack child, we have to update the arrow position when first switching to the other pages.
Created attachment 342164 [details] [review] user-accounts: Make the user name label bold in the UmCarouselItem
Created attachment 342165 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem
Created attachment 342168 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem
Created attachment 342169 [details] [review] user-accounts: Align widgets in UmCarouselItem individually Set a margin for the username label instead of using the container spacing property. In doing so the labels are not so far apart from each other, but still distant from the profile icon/image.
Review of attachment 342160 [details] [review]: Border spacing is missing without the following patch, but ok...... Hmm, the dialog changes its height when switching between users if the dialog is locked (and without the attachment 341406 [details] [review]). I am afraid that GtkStack has to be used also for user details to deal with it and also with issues caused by attachment 331579 [details] [review] and attachment 331891 [details] [review]... :-/ We should probably set enough dialog height to cover this at least for now... Looks good otherwise.
Review of attachment 342161 [details] [review]: The ui file is now much readable than before, good job... just a few notes: ::: panels/user-accounts/data/user-accounts-dialog.ui @@ +180,3 @@ + </child> + <child> + <object class="GtkBox"> I suppose that this box is redundant... @@ +184,3 @@ + <property name="orientation">GTK_ORIENTATION_VERTICAL</property> + <child> + <object class="GtkEntry" id="full-name-entry"> ...and the full-name-entry is vertically misaligned! @@ +302,3 @@ + </child> + <child> + <object class="GtkBox"> We should use GtkStack instead, but we can fix it later together with the planned user icon changes... @@ +306,3 @@ + <property name="orientation">GTK_ORIENTATION_HORIZONTAL</property> + <child> + <object class="GtkLabel" id="label4"> This label is also redundant...
Review of attachment 342162 [details] [review]: Is the animation wanted? It behaves really weird, when switching between pages on carousel for example... will send you screencast if needed... Dotted border is shown around selected carousel item (not sure this is wanted, but don't have problem with it). However, the arrow is too height (respectively the box) and the dotted border is covered by it (probably because the negative margins)...
Review of attachment 342164 [details] [review]: Looks good!
Review of attachment 342168 [details] [review]: This changes the vertical alignment... I think we should maybe add some margin on the top of carousel item (in order to reduce the whitespace between this label and the arrow). See the mockup.
Review of attachment 342169 [details] [review]: You should probably merge this with attachment 342168 [details] [review]...
(In reply to Ondrej Holy from comment #310) > Review of attachment 342162 [details] [review] [review]: > > Is the animation wanted? It behaves really weird, when switching between > pages on carousel for example... will send you screencast if needed... You can probably split out the animation into a separate patch... and push the rest in order to speed it up...
(In reply to Ondrej Holy from comment #308) > Review of attachment 342160 [details] [review] [review]: > > Hmm, the dialog changes its height when switching between users if the > dialog is locked (and without the attachment 341406 [details] [review] [review]). I > am afraid that GtkStack has to be used also for user details to deal with it > and also with issues caused by attachment 331579 [details] [review] [review] and > attachment 331891 [details] [review] [review]... :-/ We should probably set enough > dialog height to cover this at least for now... > > Looks good otherwise. We plan to merge the new control-center shell (shell/gnome-control-center-alt) in the next release, which solves this problem by having a resizable window. Anyhow, I can provide a patch for handling the height properly. (In reply to Ondrej Holy from comment #310) > Review of attachment 342162 [details] [review] [review]: > > Is the animation wanted? It behaves really weird, when switching between > pages on carousel for example... will send you screencast if needed... The first version of this implementation which I have presented to the designers did not have the animation, and I was asked to introduce it. We can tweak the animation delay (could be faster, if designers want it). > > Dotted border is shown around selected carousel item (not sure this is > wanted, but don't have problem with it). That's a gtk+ issue. That dotted border should be presented for accessibility reasons in some specific cases, but due to some changes it gets presented in unwanted places such as these togglebuttons. > However, the arrow is too height (respectively the box) and the dotted border is > covered by it (probably because the negative margins)... I can reduce its size. The negative margins are necessary to make the inner container cover the bottom border and make it look "connected" to the container below. (In reply to Ondrej Holy from comment #314) > (In reply to Ondrej Holy from comment #310) > > Review of attachment 342162 [details] [review] [review] [review]: > > > > Is the animation wanted? It behaves really weird, when switching between > > pages on carousel for example... will send you screencast if needed... > > You can probably split out the animation into a separate patch... and push > the rest in order to speed it up... Sure, I will split it then.
(In reply to Ondrej Holy from comment #309) > Review of attachment 342161 [details] [review] [review]: > > ::: panels/user-accounts/data/user-accounts-dialog.ui > @@ +184,3 @@ > + <property > name="orientation">GTK_ORIENTATION_VERTICAL</property> > + <child> > + <object class="GtkEntry" id="full-name-entry"> > > ...and the full-name-entry is vertically misaligned! In the new mockups, the full-name-entry seems to be vertically centered. So I guess it's expected to be like this.
Created attachment 342272 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342273 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342274 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Created attachment 342275 [details] [review] user-accounts: Make the user name label bold in the UmCarouselItem
Created attachment 342276 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem Set a margin for the username label instead of using the container spacing property. In doing so the labels are not so far apart from each other, but still distant from the profile icon/image.
Now the ARROW_SIZE and the carousel border_width are equivalent to the mockup (I have measured the mockup image properly).
Well done! I think we are almost ready, but still see some issues: I've just realized that the carousel items are toggle buttons and behave like that (it is not visible in Adwaita light theme, but it is visible in other themes). I mean that click toggles the background color (not sure this is wanted) and it doesn't work as expected. We have to be sure, that selected item is active and others are not active. Or we can probably use GtkButton instead? The arrow still jumps to the left corner when changing stack pages, which is probably a consequence of the stack animation... can't we simply hide it during the animation? Maybe we can also split out this animation from the patch for now. The user is not sorted correctly after renaming. It was fixed already and it is broken again. Carousel items on the second page of the stack are vertically aligned differently, it is probably because of missing "Your Account" label, can't we set valign to start for it, or add empty label, or so...? The arrow doesn't work and look correctly in win32 and Raleigh themes, I wonder whether we can hide it in those themes, or fix it somehow... seems they are not part gnome-themes-standard, so probably we don't care about those styles... do we? The arrow border seems a little bit lighter (especially in high contrast themes), but maybe this is just because it is diagonal, so not sure we can do something with it...
(In reply to Ondrej Holy from comment #323) > Well done! I think we are almost ready, but still see some issues: > > The arrow border seems a little bit lighter (especially in high contrast > themes), but maybe this is just because it is diagonal, so not sure we can > do something with it... Yep, it seems to be some anti-aliasing thing going on in there.
Created attachment 342328 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342329 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342330 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Created attachment 342331 [details] [review] user-accounts: Make the user name label bold in the UmCarouselItem
Created attachment 342332 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem Set a margin for the username label instead of using the container spacing property. In doing so the labels are not so far apart from each other, but still distant from the profile icon/image.
Created attachment 342333 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
(In reply to Ondrej Holy from comment #323) > Well done! I think we are almost ready, but still see some issues: > > I've just realized that the carousel items are toggle buttons and behave > like that (it is not visible in Adwaita light theme, but it is visible in > other themes). I mean that click toggles the background color (not sure this > is wanted) and it doesn't work as expected. We have to be sure, that > selected item is active and others are not active. Or we can probably use > GtkButton instead? Well spot. I have converted them in GtkButtons then. Also, attachment 342333 [details] [review] makes them seamless on-hover. I put it on a later patch because it is all defined in CSS (where the carousel.css file was introduced later). > The arrow still jumps to the left corner when changing stack pages, which is > probably a consequence of the stack animation... can't we simply hide it > during the animation? Maybe we can also split out this animation from the > patch for now. right. The animations are gone for now. I will address them in another bug after we are done with this one here. > The user is not sorted correctly after renaming. It was fixed already and it > is broken again. yep, some rebase mess I did. I corrected it now. > Carousel items on the second page of the stack are vertically aligned > differently, it is probably because of missing "Your Account" label, can't > we set valign to start for it, or add empty label, or so...? empty label it is now. > The arrow doesn't work and look correctly in win32 and Raleigh themes, I > wonder whether we can hide it in those themes, or fix it somehow... seems > they are not part gnome-themes-standard, so probably we don't care about > those styles... do we? These themes don't seem to let us draw stuff with overlay features. I guess we can address this issue later.
Review of attachment 342328 [details] [review]: I am sorry, but I am afraid we break something with the GtkButton probably, because newly created user is not selected and my user account is selected after each change of other user account, not sure what is exactly wrong... ::: panels/user-accounts/um-carousel.c @@ +103,3 @@ + * Returns: the found UmCarouselItem, or %NULL if it is not found + */ +UmCarouselItem* nitpick: missing space before star @@ +111,3 @@ + + list = self->children; + while (list!= NULL) nitpick: missing space before exclamation mark @@ +206,3 @@ + gtk_style_context_add_class (gtk_widget_get_style_context (widget), "menu"); + gtk_button_set_relief (GTK_BUTTON (widget), GTK_RELIEF_NONE); + nitpick: it would be nice to use get_last_page_number... page = get_last_page_number () @@ +212,3 @@ + + last_box_is_full = ((g_list_length (self->children) - 1) % ITEMS_PER_PAGE == 0); + if (last_box_is_full) { ...instead of some calculations: if (page != widget->page) ::: panels/user-accounts/um-user-panel.c @@ +209,2 @@ + if (act_user_get_uid (user) != getuid ()) { + d->other_accounts++; Account type is shown for my user sometimes and sometimes not: if (d->other_accounts == 1) show_user (d->selected_user, d);
Review of attachment 342329 [details] [review]: Looks good to me...
Review of attachment 342330 [details] [review]: Looks good to me, but please add end_x initialization before pushing... ::: panels/user-accounts/um-carousel.c @@ +24,3 @@ #include <gtk/gtk.h> +#define ARROW_SIZE 20 nitpick: would be nice to move next to #define ITEMS_PER_PAGE 3 @@ +107,3 @@ + GtkStyleContext *context; + gchar *css; + gint end_x; You should initialize this, otherwise the value might be undefined (also valgrind complains about it)... @@ +121,3 @@ + css = g_strdup_printf ("* {\n" + " margin-left: %dpx;\n" + "}\n", end_x); nitpick: would be probably more readable as oneliner (\n is not needed as far as I know) css = g_strdup_printf ("* { margin-left: %dpx; }\n", end_x);
Review of attachment 342331 [details] [review]: Looks good to me...
Review of attachment 342332 [details] [review]: Looks good to me...
Review of attachment 342333 [details] [review]: Looks good to me...
Created attachment 342378 [details] [review] user-accounts: Fix last login button sensitivity The last login button should be insensitive for other accounts if the panel is not unlocked. This is also wrong in gnome-3-22 and should be fixed there...
Created attachment 342429 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342430 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
Created attachment 342431 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Review of attachment 342431 [details] [review]: (In reply to Ondrej Holy from comment #332) > Review of attachment 342328 [details] [review] [review]: > > Account type is shown for my user sometimes and sometimes not: > if (d->other_accounts == 1) > show_user (d->selected_user, d); Hmm, I couldn't reproduce it. It is supposed to be visible only when there are multiple users (>1) tho. ::: panels/user-accounts/um-carousel.c @@ +25,3 @@ + +struct _UmCarouselItem { + GtkRadioButton parent; Making it a Radio button solves the problem of having just one toggled at the time.
Created attachment 342432 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 342433 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
Created attachment 342434 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
Review of attachment 342434 [details] [review]: ::: panels/user-accounts/data/carousel.css @@ +24,3 @@ + box-shadow: none; + border: none; + color: @theme_fg_color; these changes make it work with all the supported (default) themes.
Review of attachment 342432 [details] [review]: There are still some issues :-( Try changing name for some from other user accounts, the name is updated in carousel correctly after pressing enter, but your account is selected then and the carousel is not sorted as expected. Carousel is not working correctly then and clicking on it causes segfault...
Review of attachment 342434 [details] [review]: Ok...
Created attachment 345211 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 345212 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 345213 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Created attachment 345214 [details] [review] user-accounts: Make the user name label bold in the UmCarouselItem
Created attachment 345215 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem Set a margin for the username label instead of using the container spacing property. In doing so the labels are not so far apart from each other, but still distant from the profile icon/image.
Created attachment 345216 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
Review of attachment 342378 [details] [review]: sure. lgtm
Comment on attachment 342378 [details] [review] user-accounts: Fix last login button sensitivity Attachment 342378 [details] pushed as fc9b455 - user-accounts: Fix last login button sensitivity Thanks!
Review of attachment 345211 [details] [review]: A newly created user should be selected, also renamed user should be still selected... however in both cases my user account is selected instead of them, which I suppose is wrong.
Created attachment 345240 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 345241 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 345242 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Created attachment 345243 [details] [review] user-accounts: Make the user name label bold in the UmCarouselItem
Created attachment 345244 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem Set a margin for the username label instead of using the container spacing property. In doing so the labels are not so far apart from each other, but still distant from the profile icon/image.
Created attachment 345245 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
Review of attachment 345240 [details] [review]: A newly created user or renamed user is now selected correctly, but it is not possible to switch to my user account then after such operation... :-(
Created attachment 345313 [details] [review] user-accounts: Introduce UmCarousel UmCarousel is an horizontal container that contains UmCarouselItem children. These items are paginated 3 at 3 at the time. UmCarousel intents to act as controller for content containers. It emitis the "item-activated" signal whenever an UmCarouselItem gets activated (clicked). It automatically activates the first UmCarouselItem of the current vsible page. The visibility of the go-back and go-next button is automatically set based on the number of children. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 345314 [details] [review] user-accounts: Reorganize the user-options container This commit merges the hbox2, main-user-vbox, and grid1 into the "user-options" container. It also replaces deprecated widgets, such as GtkVBox and GtkHBox. These changes are according to the new User Accounts panel mockups at https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Created attachment 345315 [details] [review] user-accounts: Draw arrow in UmCarousel The arrow is drawn using CSS.
Created attachment 345316 [details] [review] user-accounts: Make the user name label bold in the UmCarouselItem
Created attachment 345317 [details] [review] user-accounts: Add "Your account" label below proper UmCarouselItem Set a margin for the username label instead of using the container spacing property. In doing so the labels are not so far apart from each other, but still distant from the profile icon/image.
Created attachment 345318 [details] [review] user-accounts: Make the UmCarouselItems look the same on hover UmCarouselItem is a button, but since we have now the arrow which points to the selected item, we don't need any other visual feedback (such as hover border/background).
(In reply to Ondrej Holy from comment #365) > Review of attachment 345240 [details] [review] [review]: > > A newly created user or renamed user is now selected correctly, but it is > not possible to switch to my user account then after such operation... :-( oh, sorry. Somehow GtkToggleButton's "toggled" signal wasn't been emitted. "button-press-event" will do the job here.
Great! I think that the patches are finally ready to push!
I think that the attachment 331579 [details] [review] and attachment 331891 [details] [review] may be also acceptable if some reasonable minimal height requirement for the user-option container is set... what do you think?
Please can you propose also patch for the user icon as per the mockup? It would be nice to do it in this cycle. Or fix the following at least, which happens after unlocking: (gnome-control-center:15369): Gtk-WARNING **: Allocating size to UmUserImage 0x1b83920 without calling gtk_widget_get_preferred_width/height(). How does the code know the size to allocate? (gnome-control-center:15369): Gtk-WARNING **: gtk_widget_size_allocate(): attempt to underallocate GtkToggleButton's child UmUserImage 0x1b83920. Allocation is 30x38, but minimum required size is 48x48.
It seems that I set the importance of this bug to high/critical before some time accidentally, resetting to normal/enhancement.
Great! Thanks for the fast and helpful reviews. I have just opened a couple of bugs as a follow up from these patches (so we can start fresh discussions). See https://bugzilla.gnome.org/show_bug.cgi?id=778405 and https://bugzilla.gnome.org/show_bug.cgi?id=778404
Attachment 345313 [details] pushed as d740a9a - user-accounts: Introduce UmCarousel Attachment 345314 [details] pushed as 011cbc0 - user-accounts: Reorganize the user-options container Attachment 345315 [details] pushed as 3a84720 - user-accounts: Draw arrow in UmCarousel Attachment 345316 [details] pushed as 8452beb - user-accounts: Make the user name label bold in the UmCarouselItem Attachment 345317 [details] pushed as d2529a1 - user-accounts: Add "Your account" label below proper UmCarouselItem Attachment 345318 [details] pushed as 1610fe5 - user-accounts: Make the UmCarouselItems look the same on hover