After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767065 - Redesign User Accounts Panel
Redesign User Accounts Panel
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: User Accounts
git master
Other Linux
: Normal enhancement
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-31 13:00 UTC by Felipe Borges
Modified: 2017-02-09 18:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
user-accounts: update strings to new design (7.09 KB, patch)
2016-05-31 13:03 UTC, Felipe Borges
committed Details | Review
user-accounts: move spinner to header in Add User dialog (2.79 KB, patch)
2016-05-31 13:04 UTC, Felipe Borges
none Details | Review
user-accounts: redesign offline mode for enterprise login (2.84 KB, patch)
2016-05-31 13:05 UTC, Felipe Borges
committed Details | Review
user-accounts: move arrows to start in login history dialog (3.90 KB, patch)
2016-05-31 13:05 UTC, Felipe Borges
committed Details | Review
user-accounts: use radio buttons for account type in Add User dialog (6.01 KB, patch)
2016-05-31 13:05 UTC, Felipe Borges
none Details | Review
user-accounts: align entries in Add User dialog (2.36 KB, patch)
2016-05-31 13:06 UTC, Felipe Borges
committed Details | Review
user-accounts: use radio buttons for account type (8.75 KB, patch)
2016-05-31 13:06 UTC, Felipe Borges
none Details | Review
user-accounts: drop "Login Options" label (5.65 KB, patch)
2016-05-31 13:07 UTC, Felipe Borges
committed Details | Review
user-accounts: use a label for Account Type when user isn't admin (9.12 KB, patch)
2016-05-31 13:07 UTC, Felipe Borges
needs-work Details | Review
user-accounts: use radio buttons for account type (11.09 KB, patch)
2016-06-01 10:15 UTC, Felipe Borges
committed Details | Review
user-accounts: use radio buttons for account type in Add User dialog (6.01 KB, patch)
2016-06-01 10:29 UTC, Felipe Borges
committed Details | Review
user-accounts: split join-dialog from account-dialog (25.90 KB, patch)
2016-06-10 11:29 UTC, Felipe Borges
none Details | Review
user-accounts: port Add User dialog to gtk+ composite widget templates (30.41 KB, patch)
2016-06-10 11:29 UTC, Felipe Borges
none Details | Review
user-accounts: move Add User dialog buttons to ui file (4.36 KB, patch)
2016-06-10 11:29 UTC, Felipe Borges
committed Details | Review
user-accounts: move spinner to header in Add User dialog (1.90 KB, patch)
2016-06-10 11:29 UTC, Felipe Borges
committed Details | Review
user-accounts: use Password Login instead of Automatic Login (8.31 KB, patch)
2016-06-13 11:57 UTC, Felipe Borges
none Details | Review
user-accounts: hide language settings for current user (1.36 KB, patch)
2016-06-13 12:39 UTC, Felipe Borges
committed Details | Review
user-accounts: add space between rows in um-user-panel (1.19 KB, patch)
2016-06-13 12:50 UTC, Felipe Borges
committed Details | Review
user-accounts: split join-dialog from account-dialog (26.13 KB, patch)
2016-06-14 09:27 UTC, Felipe Borges
committed Details | Review
user-accounts: port Add User dialog to gtk+ composite widget templates (30.45 KB, patch)
2016-06-14 10:53 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop unused account-type-model object (1.34 KB, patch)
2016-06-21 14:22 UTC, Felipe Borges
committed Details | Review
user-accounts: Rename "History" button label to "Account Activity" (1.25 KB, patch)
2016-06-21 14:22 UTC, Felipe Borges
committed Details | Review
user-accounts: Rename "History" dialog title to "Account Activity" (1.66 KB, patch)
2016-06-21 14:22 UTC, Felipe Borges
none Details | Review
user-accounts: Prepend user real name to "Account Activity" dialog title (1.37 KB, patch)
2016-06-21 14:22 UTC, Felipe Borges
none Details | Review
user-accounts: Make the "Account Activity" wider (983 bytes, patch)
2016-06-21 14:22 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Change "Account Activity" granularity to days (7.99 KB, patch)
2016-06-21 14:22 UTC, Felipe Borges
rejected Details | Review
user-accounts: Drop date from "Account Activity" entries (2.10 KB, patch)
2016-06-21 14:22 UTC, Felipe Borges
rejected Details | Review
user-accounts: Reorder items in "Account Activity" entries (1.64 KB, patch)
2016-06-21 14:23 UTC, Felipe Borges
committed Details | Review
user-accounts: Rename "History" dialog title to "Account Activity" (1.47 KB, patch)
2016-07-11 19:41 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Drop unused subtitle in "Account Activity" dialog (1.04 KB, patch)
2016-07-11 19:42 UTC, Felipe Borges
none Details | Review
user-accounts: Prepend user real name to "Account Activity" dialog title (1.49 KB, patch)
2016-07-11 19:42 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce users list carousel (UmCarousel) (55.35 KB, patch)
2016-07-11 19:54 UTC, Felipe Borges
none Details | Review
user-accounts: Drop "Lock" button and add "Add User" button (2.26 KB, patch)
2016-07-11 19:55 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Prevent bottom buttons from expanding (1.69 KB, patch)
2016-07-11 19:55 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Drop unnecessary grid containing last-login label (2.47 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Introduce arrow frame (UmArrowFrame) (19.79 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
none Details | Review
user-accounts: Drop user image border and background (1.53 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
none Details | Review
user-accounts: Add missing license to um-carousel.[c/h] (2.50 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
none Details | Review
user-accounts: Prevent the removal of own account and only-admin (1.19 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Update panel after changes in ActUser object (2.72 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Handle the info update just on the show_user method (3.74 KB, patch)
2016-07-11 19:56 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Properly align account type buttons (2.18 KB, patch)
2016-07-12 15:55 UTC, Felipe Borges
committed Details | Review
user-accounts: Change sensitiveness of back/forward Carousel buttons (2.70 KB, patch)
2016-07-12 15:56 UTC, Felipe Borges
none Details | Review
user-accounts: Remove Carousel item when removing account (2.11 KB, patch)
2016-07-12 15:56 UTC, Felipe Borges
none Details | Review
user-accounts: Select user after account is created (1.56 KB, patch)
2016-07-12 15:56 UTC, Felipe Borges
none Details | Review
user-accounts: Always select the first item in a page on the Carousel (1.86 KB, patch)
2016-07-12 15:56 UTC, Felipe Borges
none Details | Review
user-accounts: Centralize post page change events in single function (4.18 KB, patch)
2016-07-12 15:56 UTC, Felipe Borges
none Details | Review
user-accounts: Remove unnecessary autologin-box (3.07 KB, patch)
2016-07-13 10:02 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Do not show autologin option when it is impossible (1.56 KB, patch)
2016-07-13 10:02 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Reposition fingerprint-login-button to correct position (1.17 KB, patch)
2016-07-13 10:02 UTC, Felipe Borges
committed Details | Review
screenshot of a bug (39.29 KB, image/png)
2016-07-13 11:32 UTC, Ondrej Holy
  Details
user-accounts: Drop unused subtitle in "Account Activity" dialog (1005 bytes, patch)
2016-07-13 14:47 UTC, Felipe Borges
committed Details | Review
user-accounts: Prepend user real name to "Account Activity" dialog title (1.42 KB, patch)
2016-07-13 14:47 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop overwritten title of "Account Activity" dialog (1.53 KB, patch)
2016-07-13 14:47 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop "Last Login" entry (9.33 KB, patch)
2016-07-13 15:50 UTC, Felipe Borges
none Details | Review
user-accounts: Show "Automatic Login" option only when supported (4.63 KB, patch)
2016-07-13 15:50 UTC, Felipe Borges
none Details | Review
user-accounts: Make the "Account Activity" dialog wider (1.23 KB, patch)
2016-07-13 16:59 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop "Last Login" entry (9.15 KB, patch)
2016-07-14 08:50 UTC, Felipe Borges
none Details | Review
user-accounts: Set normal relief for UmEditableButtons (1.18 KB, patch)
2016-07-15 11:56 UTC, Felipe Borges
rejected Details | Review
user-accounts: Properly align option buttons (2.08 KB, patch)
2016-07-15 11:56 UTC, Felipe Borges
none Details | Review
user-accounts: Drop "Account Activity" button (6.96 KB, patch)
2016-07-15 11:56 UTC, Felipe Borges
none Details | Review
user-accounts: Use sensitivity to denote changes in "Account Type" (9.96 KB, patch)
2016-07-15 11:56 UTC, Felipe Borges
none Details | Review
user-accounts: Drop UmEditableButton, use GtkButtons instead (7.37 KB, patch)
2016-07-15 11:56 UTC, Felipe Borges
none Details | Review
user-accounts: Reposition "Remove Account" button (2.72 KB, patch)
2016-07-15 12:42 UTC, Felipe Borges
committed Details | Review
user-accounts: Do not show "Remove Account" button for only-admin (1.24 KB, patch)
2016-07-15 12:42 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Properly align option buttons (1.59 KB, patch)
2016-07-18 09:52 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop "Account Activity" button (6.95 KB, patch)
2016-07-18 09:52 UTC, Felipe Borges
none Details | Review
user-accounts: Use sensitivity to denote changes in "Account Type" (9.96 KB, patch)
2016-07-18 09:52 UTC, Felipe Borges
none Details | Review
user-accounts: Drop UmEditableButton, use GtkButtons instead (27.77 KB, patch)
2016-07-18 09:52 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Show "Automatic Login" option only when supported (1.83 KB, patch)
2016-07-18 12:28 UTC, Felipe Borges
none Details | Review
user-accounts: Use sensitivity to denote changes in "Account Type" (10.02 KB, patch)
2016-07-20 09:49 UTC, Felipe Borges
none Details | Review
user-accounts: Drop usage of UmEditableButtons (9.40 KB, patch)
2016-07-20 09:49 UTC, Felipe Borges
none Details | Review
user-accounts: Remove UmEditableButton object (18.75 KB, patch)
2016-07-20 09:49 UTC, Felipe Borges
committed Details | Review
user-accounts: Use GtkEntry instead of CcEditableEntry (4.25 KB, patch)
2016-07-20 09:49 UTC, Felipe Borges
none Details | Review
user-accounts: Show "Automatic Login" option only when supported (2.51 KB, patch)
2016-07-20 09:49 UTC, Felipe Borges
none Details | Review
user-accounts: Move "Add User" button to header bar (6.14 KB, patch)
2016-07-20 12:57 UTC, Felipe Borges
committed Details | Review
user-accounts: Unify size of headerbar buttons (1.51 KB, patch)
2016-07-21 11:31 UTC, Ondrej Holy
committed Details | Review
user-accounts: Use sensitivity to denote changes in "Account Type" (9.34 KB, patch)
2016-07-21 15:29 UTC, Felipe Borges
none Details | Review
user-accounts: Show "Automatic Login" option only when supported (2.15 KB, patch)
2016-07-21 15:29 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Remove UmEditableCombo class (20.51 KB, patch)
2016-07-21 15:29 UTC, Felipe Borges
committed Details | Review
Screencast of autologin label bug (180.15 KB, video/webm)
2016-07-22 08:59 UTC, Ondrej Holy
  Details
user-accounts: Use GtkEntry instead of CcEditableEntry (4.74 KB, patch)
2016-07-26 12:03 UTC, Felipe Borges
committed Details | Review
Screencast of "remove accout" bug (108.33 KB, video/webm)
2016-07-27 07:48 UTC, Ondrej Holy
  Details
user-accounts: Drop usage of UmEditableButtons (10.45 KB, patch)
2016-07-27 09:25 UTC, Felipe Borges
none Details | Review
user-accounts: Do not access already removed toolbar (1.26 KB, patch)
2016-07-27 12:49 UTC, Ondrej Holy
committed Details | Review
user-accounts: Drop usage of UmEditableButtons (11.33 KB, patch)
2016-07-28 09:13 UTC, Felipe Borges
none Details | Review
user-accounts: Drop "Account Activity" button (7.27 KB, patch)
2016-07-28 13:30 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop usage of UmEditableButtons (10.78 KB, patch)
2016-07-28 13:32 UTC, Felipe Borges
committed Details | Review
user-accounts: Use sensitivity to denote changes in "Account Type" (10.37 KB, patch)
2016-07-29 13:26 UTC, Felipe Borges
committed Details | Review
user-accounts: Fix history dialog height (1.80 KB, patch)
2016-07-29 14:17 UTC, Ondrej Holy
committed Details | Review
user-accounts: Introduce arrow frame (UmArrowFrame) (4.67 KB, patch)
2016-08-01 08:28 UTC, Felipe Borges
none Details | Review
user-accounts: Fix horizontal alignment of "Account Type" buttons (1.65 KB, patch)
2016-08-01 08:29 UTC, Felipe Borges
committed Details | Review
user-accounts: Drop CcEditableEntry references from um-user-panel (1.18 KB, patch)
2016-08-01 08:29 UTC, Felipe Borges
committed Details | Review
user-accounts: Introduce arrow frame (UmArrowFrame) (6.74 KB, patch)
2016-08-01 12:43 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Introduce carousel (UmCarousel) (97.23 KB, patch)
2016-08-05 12:08 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Hide UmCarousel when there's just a single user (2.98 KB, patch)
2016-08-05 12:08 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Just set arrow-frame's carousel once (1.02 KB, patch)
2016-08-05 12:08 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Use user_uid to index users instead of position (3.61 KB, patch)
2016-08-07 11:30 UTC, Felipe Borges
needs-work Details | Review
Screencast of carousel glitches (340.65 KB, video/webm)
2016-08-10 08:42 UTC, Ondrej Holy
  Details
user-accounts: Introduce UmCarousel (41.37 KB, patch)
2016-11-15 13:42 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (43.00 KB, patch)
2016-11-15 18:07 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (43.06 KB, patch)
2016-11-16 15:36 UTC, Felipe Borges
none Details | Review
user-accounts: Don't show_restart_notification when changing Language (2.17 KB, patch)
2016-11-27 16:04 UTC, Felipe Borges
committed Details | Review
user-accounts: Introduce UmCarousel (48.96 KB, patch)
2016-12-02 11:26 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (49.12 KB, patch)
2016-12-02 16:49 UTC, Felipe Borges
none Details | Review
user-accounts: Align user options and the "Remove Account" button (2.26 KB, patch)
2016-12-02 16:50 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Set user options sensitive based on the permissions (1.02 KB, patch)
2016-12-05 15:42 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Introduce UmCarousel (85.97 KB, patch)
2016-12-05 15:42 UTC, Felipe Borges
none Details | Review
user-accounts: Reorganize the user-options container (1.26 KB, patch)
2016-12-05 15:43 UTC, Felipe Borges
none Details | Review
user-accounts: Draw arrow in UmCarousel (9.12 KB, patch)
2016-12-10 21:53 UTC, Felipe Borges
none Details | Review
user-accounts: Select the first user also in the UmCarousel (1.22 KB, patch)
2016-12-10 21:53 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Select main user in the carousel when user_removed (2.32 KB, patch)
2016-12-10 22:10 UTC, Felipe Borges
needs-work Details | Review
user-accounts: Do not draw the arrow when the Carousel is hidden (2.56 KB, patch)
2016-12-10 22:31 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Introduce UmCarousel (48.62 KB, patch)
2016-12-18 12:59 UTC, Felipe Borges
none Details | Review
user-accounts: Reorganize the user-options container (39.48 KB, patch)
2016-12-18 13:00 UTC, Felipe Borges
none Details | Review
user-accounts: Draw arrow in UmCarousel (14.25 KB, patch)
2016-12-18 13:00 UTC, Felipe Borges
none Details | Review
user-accounts: Make the user name label bold in the UmCarouselItem (1.41 KB, patch)
2016-12-18 13:50 UTC, Felipe Borges
none Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (1.20 KB, patch)
2016-12-18 13:50 UTC, Felipe Borges
none Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (1.28 KB, patch)
2016-12-18 14:01 UTC, Felipe Borges
none Details | Review
user-accounts: Align widgets in UmCarouselItem individually (1.51 KB, patch)
2016-12-18 14:05 UTC, Felipe Borges
reviewed Details | Review
user-accounts: Introduce UmCarousel (48.62 KB, patch)
2016-12-20 17:54 UTC, Felipe Borges
none Details | Review
user-accounts: Reorganize the user-options container (38.53 KB, patch)
2016-12-20 17:54 UTC, Felipe Borges
none Details | Review
user-accounts: Draw arrow in UmCarousel (13.80 KB, patch)
2016-12-20 17:55 UTC, Felipe Borges
none Details | Review
user-accounts: Make the user name label bold in the UmCarouselItem (1.41 KB, patch)
2016-12-20 17:55 UTC, Felipe Borges
none Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (2.06 KB, patch)
2016-12-20 17:55 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (47.98 KB, patch)
2016-12-21 15:01 UTC, Felipe Borges
none Details | Review
user-accounts: Reorganize the user-options container (38.53 KB, patch)
2016-12-21 15:01 UTC, Felipe Borges
none Details | Review
user-accounts: Draw arrow in UmCarousel (13.79 KB, patch)
2016-12-21 15:02 UTC, Felipe Borges
none Details | Review
user-accounts: Make the user name label bold in the UmCarouselItem (1.41 KB, patch)
2016-12-21 15:02 UTC, Felipe Borges
none Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (2.05 KB, patch)
2016-12-21 15:02 UTC, Felipe Borges
none Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.44 KB, patch)
2016-12-21 15:02 UTC, Felipe Borges
none Details | Review
user-accounts: Fix last login button sensitivity (1.63 KB, patch)
2016-12-22 10:30 UTC, Ondrej Holy
committed Details | Review
user-accounts: Introduce UmCarousel (47.98 KB, patch)
2016-12-23 21:37 UTC, Felipe Borges
none Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.49 KB, patch)
2016-12-23 21:38 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (48.68 KB, patch)
2016-12-23 22:51 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (48.68 KB, patch)
2016-12-23 22:58 UTC, Felipe Borges
none Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.46 KB, patch)
2016-12-23 22:59 UTC, Felipe Borges
none Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.50 KB, patch)
2016-12-23 23:02 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (48.21 KB, patch)
2017-02-08 13:24 UTC, Felipe Borges
none Details | Review
user-accounts: Reorganize the user-options container (38.53 KB, patch)
2017-02-08 13:24 UTC, Felipe Borges
none Details | Review
user-accounts: Draw arrow in UmCarousel (13.79 KB, patch)
2017-02-08 13:24 UTC, Felipe Borges
none Details | Review
user-accounts: Make the user name label bold in the UmCarouselItem (1.41 KB, patch)
2017-02-08 13:24 UTC, Felipe Borges
none Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (2.05 KB, patch)
2017-02-08 13:25 UTC, Felipe Borges
none Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.50 KB, patch)
2017-02-08 13:25 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (48.43 KB, patch)
2017-02-08 15:32 UTC, Felipe Borges
none Details | Review
user-accounts: Reorganize the user-options container (38.53 KB, patch)
2017-02-08 15:32 UTC, Felipe Borges
none Details | Review
user-accounts: Draw arrow in UmCarousel (13.79 KB, patch)
2017-02-08 15:32 UTC, Felipe Borges
none Details | Review
user-accounts: Make the user name label bold in the UmCarouselItem (1.41 KB, patch)
2017-02-08 15:32 UTC, Felipe Borges
none Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (2.05 KB, patch)
2017-02-08 15:33 UTC, Felipe Borges
none Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.50 KB, patch)
2017-02-08 15:33 UTC, Felipe Borges
none Details | Review
user-accounts: Introduce UmCarousel (48.48 KB, patch)
2017-02-09 13:37 UTC, Felipe Borges
committed Details | Review
user-accounts: Reorganize the user-options container (38.53 KB, patch)
2017-02-09 13:37 UTC, Felipe Borges
committed Details | Review
user-accounts: Draw arrow in UmCarousel (13.88 KB, patch)
2017-02-09 13:38 UTC, Felipe Borges
committed Details | Review
user-accounts: Make the user name label bold in the UmCarouselItem (1.41 KB, patch)
2017-02-09 13:39 UTC, Felipe Borges
committed Details | Review
user-accounts: Add "Your account" label below proper UmCarouselItem (2.05 KB, patch)
2017-02-09 13:39 UTC, Felipe Borges
committed Details | Review
user-accounts: Make the UmCarouselItems look the same on hover (1.50 KB, patch)
2017-02-09 13:40 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2016-05-31 13:00:24 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.
Comment 1 Felipe Borges 2016-05-31 13:03:33 UTC
Created attachment 328792 [details] [review]
user-accounts: update strings to new design

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 2 Felipe Borges 2016-05-31 13:04:28 UTC
Created attachment 328793 [details] [review]
user-accounts: move spinner to header in Add User dialog

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 3 Felipe Borges 2016-05-31 13:05:14 UTC
Created attachment 328794 [details] [review]
user-accounts: redesign offline mode for enterprise login

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 4 Felipe Borges 2016-05-31 13:05:37 UTC
Created attachment 328795 [details] [review]
user-accounts: move arrows to start in login history dialog

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 5 Felipe Borges 2016-05-31 13:05:59 UTC
Created attachment 328796 [details] [review]
user-accounts: use radio buttons for account type in Add User dialog

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 6 Felipe Borges 2016-05-31 13:06:27 UTC
Created attachment 328797 [details] [review]
user-accounts: align entries in Add User dialog

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 7 Felipe Borges 2016-05-31 13:06:50 UTC
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
Comment 8 Felipe Borges 2016-05-31 13:07:12 UTC
Created attachment 328799 [details] [review]
user-accounts: drop "Login Options" label

According to the mockups at
https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 9 Felipe Borges 2016-05-31 13:07:35 UTC
Created attachment 328800 [details] [review]
user-accounts: use a label for Account Type when user isn't admin
Comment 10 Bastien Nocera 2016-05-31 13:15:54 UTC
Review of attachment 328792 [details] [review]:

Sure.
Comment 11 Bastien Nocera 2016-05-31 13:18:05 UTC
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?
Comment 12 Bastien Nocera 2016-05-31 13:20:00 UTC
Review of attachment 328794 [details] [review]:

No code changes necessary to make this work?
Comment 13 Bastien Nocera 2016-05-31 13:22:09 UTC
Review of attachment 328795 [details] [review]:

Looks good.
Comment 14 Bastien Nocera 2016-05-31 13:24:12 UTC
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?
Comment 15 Bastien Nocera 2016-05-31 13:25:47 UTC
Review of attachment 328797 [details] [review]:

Sure.
Comment 16 Bastien Nocera 2016-05-31 13:27:15 UTC
Review of attachment 328798 [details] [review]:

Looks fine.
Comment 17 Bastien Nocera 2016-05-31 13:40:27 UTC
Review of attachment 328799 [details] [review]:

OK.
Comment 18 Bastien Nocera 2016-05-31 13:42:04 UTC
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 19 Felipe Borges 2016-06-01 09:43:59 UTC
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 20 Felipe Borges 2016-06-01 09:48:04 UTC
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 21 Felipe Borges 2016-06-01 09:50:51 UTC
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 22 Felipe Borges 2016-06-01 09:58:30 UTC
Comment on attachment 328799 [details] [review]
user-accounts: drop "Login Options" label

Attachment 328799 [details] pushed as 941282a - user-accounts: drop "Login Options" label
Comment 23 Felipe Borges 2016-06-01 10:15:51 UTC
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 24 Felipe Borges 2016-06-01 10:17:42 UTC
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].
Comment 25 Felipe Borges 2016-06-01 10:29:14 UTC
Created attachment 328872 [details] [review]
user-accounts: use radio buttons for account type in Add User dialog

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 26 Felipe Borges 2016-06-01 14:29:24 UTC
(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.
Comment 27 Bastien Nocera 2016-06-01 14:32:05 UTC
Review of attachment 328794 [details] [review]:

Ok then.
Comment 28 Bastien Nocera 2016-06-01 14:34:40 UTC
Review of attachment 328871 [details] [review]:

Looks fine.
Comment 29 Bastien Nocera 2016-06-01 14:36:32 UTC
Review of attachment 328872 [details] [review]:

Looks fine.
Comment 30 Felipe Borges 2016-06-01 15:12:30 UTC
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
Comment 31 Felipe Borges 2016-06-10 11:29:30 UTC
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.
Comment 32 Felipe Borges 2016-06-10 11:29:41 UTC
Created attachment 329561 [details] [review]
user-accounts: port Add User dialog to gtk+ composite widget templates
Comment 33 Felipe Borges 2016-06-10 11:29:50 UTC
Created attachment 329562 [details] [review]
user-accounts: move Add User dialog buttons to ui file
Comment 34 Felipe Borges 2016-06-10 11:29:59 UTC
Created attachment 329563 [details] [review]
user-accounts: move spinner to header in Add User dialog

https://wiki.gnome.org/Design/SystemSettings/UserAccounts
Comment 35 Felipe Borges 2016-06-13 11:57:45 UTC
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
Comment 36 Felipe Borges 2016-06-13 12:00:07 UTC
(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 37 Felipe Borges 2016-06-13 12:30:05 UTC
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
Comment 38 Felipe Borges 2016-06-13 12:39:32 UTC
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
Comment 39 Felipe Borges 2016-06-13 12:50:23 UTC
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
Comment 40 Bastien Nocera 2016-06-13 13:26:44 UTC
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?
Comment 41 Bastien Nocera 2016-06-13 13:29:46 UTC
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.
Comment 42 Bastien Nocera 2016-06-13 13:31:24 UTC
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?
Comment 43 Bastien Nocera 2016-06-13 13:32:01 UTC
Review of attachment 329563 [details] [review]:

Sure.
Comment 44 Bastien Nocera 2016-06-13 13:33:55 UTC
Review of attachment 329691 [details] [review]:

Sure.
Comment 45 Bastien Nocera 2016-06-13 13:34:24 UTC
Review of attachment 329692 [details] [review]:

Sure.
Comment 46 Felipe Borges 2016-06-13 15:10:43 UTC
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 47 Felipe Borges 2016-06-13 15:18:59 UTC
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
Comment 48 Felipe Borges 2016-06-14 09:27:33 UTC
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.
Comment 49 Bastien Nocera 2016-06-14 10:42:42 UTC
Review of attachment 329764 [details] [review]:

Looks fine.
Comment 50 Felipe Borges 2016-06-14 10:53:43 UTC
Created attachment 329774 [details] [review]
user-accounts: port Add User dialog to gtk+ composite widget templates
Comment 51 Felipe Borges 2016-06-14 10:56:53 UTC
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
Comment 52 Bastien Nocera 2016-06-14 11:05:19 UTC
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
Comment 53 Felipe Borges 2016-06-14 11:15:52 UTC
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
Comment 54 Felipe Borges 2016-06-21 14:22:14 UTC
Created attachment 330135 [details] [review]
user-accounts: Drop unused account-type-model object
Comment 55 Felipe Borges 2016-06-21 14:22:22 UTC
Created attachment 330136 [details] [review]
user-accounts: Rename "History" button label to "Account Activity"
Comment 56 Felipe Borges 2016-06-21 14:22:31 UTC
Created attachment 330137 [details] [review]
user-accounts: Rename "History" dialog title to "Account Activity"
Comment 57 Felipe Borges 2016-06-21 14:22:38 UTC
Created attachment 330138 [details] [review]
user-accounts: Prepend user real name to "Account Activity" dialog title
Comment 58 Felipe Borges 2016-06-21 14:22:45 UTC
Created attachment 330139 [details] [review]
user-accounts: Make the "Account Activity" wider
Comment 59 Felipe Borges 2016-06-21 14:22:52 UTC
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
Comment 60 Felipe Borges 2016-06-21 14:22:59 UTC
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.
Comment 61 Felipe Borges 2016-06-21 14:23:07 UTC
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
Comment 62 Bastien Nocera 2016-06-24 12:22:03 UTC
Review of attachment 330135 [details] [review]:

Sure.
Comment 63 Bastien Nocera 2016-06-24 12:23:26 UTC
Review of attachment 330136 [details] [review]:

Yep.
Comment 64 Bastien Nocera 2016-06-24 12:25:31 UTC
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"?
Comment 65 Bastien Nocera 2016-06-24 12:27:07 UTC
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"
Comment 66 Bastien Nocera 2016-06-24 12:32:08 UTC
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.
Comment 67 Bastien Nocera 2016-06-24 12:37:55 UTC
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.
Comment 68 Bastien Nocera 2016-06-24 12:39:29 UTC
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.
Comment 69 Bastien Nocera 2016-06-24 12:40:23 UTC
Review of attachment 330142 [details] [review]:

Sure.
Comment 70 Felipe Borges 2016-06-24 14:15:44 UTC
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
Comment 71 Felipe Borges 2016-07-11 19:41:14 UTC
Created attachment 331263 [details] [review]
user-accounts: Rename "History" dialog title to "Account Activity"
Comment 72 Felipe Borges 2016-07-11 19:42:25 UTC
Created attachment 331264 [details] [review]
user-accounts: Drop unused subtitle in "Account Activity" dialog
Comment 73 Felipe Borges 2016-07-11 19:42:32 UTC
Created attachment 331265 [details] [review]
user-accounts: Prepend user real name to "Account Activity" dialog title
Comment 74 Felipe Borges 2016-07-11 19:54:53 UTC
Created attachment 331266 [details] [review]
user-accounts: Introduce users list carousel (UmCarousel)
Comment 75 Felipe Borges 2016-07-11 19:55:19 UTC
Created attachment 331267 [details] [review]
user-accounts: Drop "Lock" button and add "Add User" button
Comment 76 Felipe Borges 2016-07-11 19:55:54 UTC
Created attachment 331268 [details] [review]
user-accounts: Prevent bottom buttons from expanding

"Delete Account" and "Account Activity" buttons.
Comment 77 Felipe Borges 2016-07-11 19:56:03 UTC
Created attachment 331269 [details] [review]
user-accounts: Drop unnecessary grid containing last-login label
Comment 78 Felipe Borges 2016-07-11 19:56:12 UTC
Created attachment 331270 [details] [review]
user-accounts: Introduce arrow frame (UmArrowFrame)

Type of GtkFrame which points to an item in an UmCarousel.
Comment 79 Felipe Borges 2016-07-11 19:56:19 UTC
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
Comment 80 Felipe Borges 2016-07-11 19:56:28 UTC
Created attachment 331272 [details] [review]
user-accounts: Add missing license to um-carousel.[c/h]
Comment 81 Felipe Borges 2016-07-11 19:56:36 UTC
Created attachment 331273 [details] [review]
user-accounts: Prevent the removal of own account and only-admin
Comment 82 Felipe Borges 2016-07-11 19:56:46 UTC
Created attachment 331274 [details] [review]
user-accounts: Update panel after changes in ActUser object
Comment 83 Felipe Borges 2016-07-11 19:56:54 UTC
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.
Comment 84 Felipe Borges 2016-07-12 15:55:54 UTC
Created attachment 331365 [details] [review]
user-accounts: Properly align account type buttons

These two buttons should have the same size.
Comment 85 Felipe Borges 2016-07-12 15:56:01 UTC
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.
Comment 86 Felipe Borges 2016-07-12 15:56:09 UTC
Created attachment 331367 [details] [review]
user-accounts: Remove Carousel item when removing account
Comment 87 Felipe Borges 2016-07-12 15:56:16 UTC
Created attachment 331368 [details] [review]
user-accounts: Select user after account is created
Comment 88 Felipe Borges 2016-07-12 15:56:24 UTC
Created attachment 331369 [details] [review]
user-accounts: Always select the first item in a page on the Carousel
Comment 89 Felipe Borges 2016-07-12 15:56:31 UTC
Created attachment 331370 [details] [review]
user-accounts: Centralize post page change events in single function
Comment 90 Felipe Borges 2016-07-13 10:02:27 UTC
Created attachment 331394 [details] [review]
user-accounts: Remove unnecessary autologin-box
Comment 91 Felipe Borges 2016-07-13 10:02:41 UTC
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.
Comment 92 Felipe Borges 2016-07-13 10:02:55 UTC
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.
Comment 93 Ondrej Holy 2016-07-13 11:32:37 UTC
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...
Comment 94 Ondrej Holy 2016-07-13 12:14:12 UTC
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...
Comment 95 Ondrej Holy 2016-07-13 12:33:02 UTC
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...
Comment 96 Ondrej Holy 2016-07-13 12:33:14 UTC
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...
Comment 97 Ondrej Holy 2016-07-13 12:33:19 UTC
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
Comment 98 Ondrej Holy 2016-07-13 12:53:27 UTC
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...
Comment 99 Ondrej Holy 2016-07-13 12:53:39 UTC
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
Comment 100 Ondrej Holy 2016-07-13 12:57:19 UTC
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?
Comment 101 Ondrej Holy 2016-07-13 13:10:36 UTC
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...
Comment 102 Ondrej Holy 2016-07-13 13:11:54 UTC
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...
Comment 103 Ondrej Holy 2016-07-13 13:19:04 UTC
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?
Comment 104 Ondrej Holy 2016-07-13 13:19:56 UTC
Review of attachment 331365 [details] [review]:

Looks good!
Comment 105 Ondrej Holy 2016-07-13 13:29:46 UTC
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?
Comment 106 Ondrej Holy 2016-07-13 13:32:31 UTC
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...
Comment 107 Ondrej Holy 2016-07-13 13:34:08 UTC
Review of attachment 331396 [details] [review]:

Looks good...
Comment 108 Ondrej Holy 2016-07-13 13:40:01 UTC
Review of attachment 331395 [details] [review]:

Not sure we want this... worth to discuss with aday.
Comment 109 Felipe Borges 2016-07-13 14:01:50 UTC
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
Comment 110 Felipe Borges 2016-07-13 14:47:06 UTC
Created attachment 331415 [details] [review]
user-accounts: Drop unused subtitle in "Account Activity" dialog
Comment 111 Felipe Borges 2016-07-13 14:47:15 UTC
Created attachment 331416 [details] [review]
user-accounts: Prepend user real name to "Account Activity" dialog title
Comment 112 Felipe Borges 2016-07-13 14:47:25 UTC
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 113 Felipe Borges 2016-07-13 14:49:39 UTC
Comment on attachment 331263 [details] [review]
user-accounts: Rename "History" dialog title to "Account Activity"

obsoleted by attachment 331417 [details] [review].
Comment 114 Felipe Borges 2016-07-13 15:50:39 UTC
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.
Comment 115 Felipe Borges 2016-07-13 15:50:48 UTC
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.
Comment 116 Felipe Borges 2016-07-13 15:54:42 UTC
(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 117 Felipe Borges 2016-07-13 15:55:42 UTC
Comment on attachment 331395 [details] [review]
user-accounts: Do not show autologin option when it is impossible

obsoleted by attachment 331423 [details] [review].
Comment 118 Felipe Borges 2016-07-13 16:01:50 UTC
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 119 Felipe Borges 2016-07-13 16:32:31 UTC
Comment on attachment 331269 [details] [review]
user-accounts: Drop unnecessary grid containing last-login label

obsoleted by attachment 331422 [details] [review].
Comment 120 Felipe Borges 2016-07-13 16:46:38 UTC
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.
Comment 121 Felipe Borges 2016-07-13 16:59:24 UTC
Created attachment 331437 [details] [review]
user-accounts: Make the "Account Activity" dialog wider

Set it to 60% of the parent window.
Comment 122 Felipe Borges 2016-07-13 17:00:06 UTC
Comment on attachment 330139 [details] [review]
user-accounts: Make the "Account Activity" wider

obsoleted by attachment 331437 [details] [review].
Comment 123 Ondrej Holy 2016-07-13 19:09:29 UTC
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].
Comment 124 Ondrej Holy 2016-07-13 19:09:46 UTC
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...
Comment 125 Ondrej Holy 2016-07-13 19:09:55 UTC
Review of attachment 331417 [details] [review]:

Looks good!
Comment 126 Ondrej Holy 2016-07-13 19:10:45 UTC
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...
Comment 127 Ondrej Holy 2016-07-13 19:11:53 UTC
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...
Comment 128 Ondrej Holy 2016-07-13 19:13:38 UTC
Review of attachment 331437 [details] [review]:

Why not if it is designers decision.
Comment 129 Ondrej Holy 2016-07-13 19:24:10 UTC
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...
Comment 130 Felipe Borges 2016-07-14 08:37:07 UTC
(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.
Comment 131 Felipe Borges 2016-07-14 08:44:57 UTC
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
Comment 132 Felipe Borges 2016-07-14 08:50:58 UTC
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 133 Felipe Borges 2016-07-15 10:14:00 UTC
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.
Comment 134 Felipe Borges 2016-07-15 11:56:05 UTC
Created attachment 331568 [details] [review]
user-accounts: Set normal relief for UmEditableButtons

According to the latest mockups, the option buttons should have
relief.
Comment 135 Felipe Borges 2016-07-15 11:56:16 UTC
Created attachment 331569 [details] [review]
user-accounts: Properly align option buttons
Comment 136 Felipe Borges 2016-07-15 11:56:25 UTC
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.
Comment 137 Felipe Borges 2016-07-15 11:56:36 UTC
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.
Comment 138 Felipe Borges 2016-07-15 11:56:46 UTC
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.
Comment 139 Felipe Borges 2016-07-15 12:42:20 UTC
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.
Comment 140 Felipe Borges 2016-07-15 12:42:29 UTC
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.
Comment 141 Ondrej Holy 2016-07-15 14:53:26 UTC
Review of attachment 331568 [details] [review]:

This doesn't make sense if UmEditableButtons is replaced by GtkButton in the following patch...
Comment 142 Ondrej Holy 2016-07-15 14:53:40 UTC
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...
Comment 143 Ondrej Holy 2016-07-15 14:53:53 UTC
Review of attachment 331570 [details] [review]:

This change seems pretty confusing to me, do designers really wants this?
Comment 144 Ondrej Holy 2016-07-15 14:54:06 UTC
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.
Comment 145 Ondrej Holy 2016-07-15 14:54:23 UTC
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!
Comment 146 Ondrej Holy 2016-07-15 14:54:35 UTC
Review of attachment 331578 [details] [review]:

Doesn't make sense without the carousel... also the panel is changing its width when switching between users!
Comment 147 Ondrej Holy 2016-07-15 14:54:44 UTC
Review of attachment 331579 [details] [review]:

Doesn't make sense without the previous patch...
Comment 148 Felipe Borges 2016-07-17 12:34:05 UTC
(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.
Comment 149 Felipe Borges 2016-07-17 12:37:30 UTC
(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.
Comment 150 Ondrej Holy 2016-07-18 07:21:08 UTC
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
Comment 151 Ondrej Holy 2016-07-18 07:26:34 UTC
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...
Comment 152 Ondrej Holy 2016-07-18 07:26:38 UTC
Review of attachment 331579 [details] [review]:

Looks good, although the visibility changes are related to width changes...
Comment 153 Ondrej Holy 2016-07-18 07:34:18 UTC
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...
Comment 154 Felipe Borges 2016-07-18 08:36:14 UTC
(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 ().
Comment 155 Ondrej Holy 2016-07-18 08:57:55 UTC
(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?
Comment 156 Felipe Borges 2016-07-18 09:20:23 UTC
(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.
Comment 157 Felipe Borges 2016-07-18 09:52:07 UTC
Created attachment 331681 [details] [review]
user-accounts: Properly align option buttons
Comment 158 Felipe Borges 2016-07-18 09:52:16 UTC
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.
Comment 159 Felipe Borges 2016-07-18 09:52:24 UTC
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.
Comment 160 Felipe Borges 2016-07-18 09:52:32 UTC
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.
Comment 161 Felipe Borges 2016-07-18 12:28:56 UTC
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.
Comment 162 Felipe Borges 2016-07-18 12:30:37 UTC
(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.
Comment 163 Ondrej Holy 2016-07-18 12:47:45 UTC
Review of attachment 331681 [details] [review]:

Looks good, but would be nice to push together with other patches...
Comment 164 Ondrej Holy 2016-07-18 12:47:59 UTC
Review of attachment 331682 [details] [review]:

Looks good, but would be nice to push together with other patches...
Comment 165 Ondrej Holy 2016-07-18 12:48:12 UTC
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
Comment 166 Ondrej Holy 2016-07-18 12:48:16 UTC
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...
Comment 167 Ondrej Holy 2016-07-18 12:53:53 UTC
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...
Comment 168 Felipe Borges 2016-07-18 13:08:46 UTC
(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! :)
Comment 169 Ondrej Holy 2016-07-18 13:10:13 UTC
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);
-
Comment 170 Felipe Borges 2016-07-20 09:49:00 UTC
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.
Comment 171 Felipe Borges 2016-07-20 09:49:09 UTC
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.
Comment 172 Felipe Borges 2016-07-20 09:49:18 UTC
Created attachment 331806 [details] [review]
user-accounts: Remove UmEditableButton object

To bring it back, revert this commit.
Comment 173 Felipe Borges 2016-07-20 09:49:25 UTC
Created attachment 331807 [details] [review]
user-accounts: Use GtkEntry instead of CcEditableEntry
Comment 174 Felipe Borges 2016-07-20 09:49:34 UTC
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 175 Felipe Borges 2016-07-20 09:50:43 UTC
Comment on attachment 331684 [details] [review]
user-accounts: Drop UmEditableButton, use GtkButtons instead

obsoleted by attachment 331805 [details] [review] and attachment 331806 [details] [review].
Comment 176 Felipe Borges 2016-07-20 12:57:47 UTC
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.
Comment 177 Felipe Borges 2016-07-20 12:58:52 UTC
(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.
Comment 178 Ondrej Holy 2016-07-20 15:11:10 UTC
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...
Comment 179 Ondrej Holy 2016-07-20 15:11:14 UTC
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...
Comment 180 Ondrej Holy 2016-07-20 15:11:17 UTC
Review of attachment 331806 [details] [review]:

Looks good!
Comment 181 Ondrej Holy 2016-07-20 15:11:20 UTC
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...
Comment 182 Ondrej Holy 2016-07-20 15:11:22 UTC
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...
Comment 183 Ondrej Holy 2016-07-21 11:31:41 UTC
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.
Comment 184 Felipe Borges 2016-07-21 15:29:24 UTC
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.
Comment 185 Felipe Borges 2016-07-21 15:29:32 UTC
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.
Comment 186 Felipe Borges 2016-07-21 15:29:41 UTC
Created attachment 331892 [details] [review]
user-accounts: Remove UmEditableCombo class

Revert this commit in order to bring the UmEditableCombo class back.
Comment 187 Felipe Borges 2016-07-21 15:31:07 UTC
(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.
Comment 188 Ondrej Holy 2016-07-22 08:58:51 UTC
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))), ...);
Comment 189 Ondrej Holy 2016-07-22 08:59:00 UTC
Review of attachment 331819 [details] [review]:

Looks good to me, but would be nice to push together with "Remove button" patches...
Comment 190 Ondrej Holy 2016-07-22 08:59:03 UTC
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.
Comment 191 Ondrej Holy 2016-07-22 08:59:06 UTC
Review of attachment 331891 [details] [review]:

Please see Comment 182, there is some problem with size groups probably, will attach screencast.
Comment 192 Ondrej Holy 2016-07-22 08:59:12 UTC
Review of attachment 331892 [details] [review]:

Looks good!
Comment 193 Ondrej Holy 2016-07-22 08:59:59 UTC
Created attachment 331967 [details]
Screencast of autologin label bug
Comment 194 Felipe Borges 2016-07-25 13:23:45 UTC
Comment on attachment 331892 [details] [review]
user-accounts: Remove UmEditableCombo class

Attachment 331892 [details] pushed as 1345cf3 - user-accounts: Remove UmEditableCombo class
Comment 195 Felipe Borges 2016-07-26 11:43:52 UTC
(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.
Comment 196 Felipe Borges 2016-07-26 12:03:56 UTC
Created attachment 332140 [details] [review]
user-accounts: Use GtkEntry instead of CcEditableEntry
Comment 197 Felipe Borges 2016-07-26 13:09:30 UTC
(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].
Comment 198 Felipe Borges 2016-07-26 13:10:38 UTC
Review of attachment 331869 [details] [review]:

Sure.
Comment 199 Felipe Borges 2016-07-26 13:12:46 UTC
(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.
Comment 200 Ondrej Holy 2016-07-27 07:28:05 UTC
(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 201 Ondrej Holy 2016-07-27 07:28:14 UTC
Comment on attachment 331869 [details] [review]
user-accounts: Unify size of headerbar buttons

commit 90d6f3b622165bbb868756704a9c22d894cdfa6c
Comment 202 Ondrej Holy 2016-07-27 07:48:33 UTC
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].
Comment 203 Ondrej Holy 2016-07-27 08:07:20 UTC
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...
Comment 204 Ondrej Holy 2016-07-27 08:19:45 UTC
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 205 Ondrej Holy 2016-07-27 08:22:02 UTC
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...
Comment 206 Felipe Borges 2016-07-27 09:25:52 UTC
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.
Comment 207 Felipe Borges 2016-07-27 09:52:36 UTC
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
Comment 208 Ondrej Holy 2016-07-27 12:49:19 UTC
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
Comment 209 Ondrej Holy 2016-07-27 13:07:28 UTC
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.
Comment 210 Felipe Borges 2016-07-28 08:48:57 UTC
Review of attachment 332212 [details] [review]:

sure sure, sorry for leaving this behind.
Comment 211 Felipe Borges 2016-07-28 08:55:27 UTC
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
Comment 212 Felipe Borges 2016-07-28 09:13:11 UTC
(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.
Comment 213 Felipe Borges 2016-07-28 09:13:46 UTC
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.
Comment 214 Ondrej Holy 2016-07-28 10:42:10 UTC
Review of attachment 331682 [details] [review]:

I didn't realize it at the first time, but the button label is wrongly aligned also here... :-/
Comment 215 Ondrej Holy 2016-07-28 10:44:38 UTC
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].
Comment 216 Ondrej Holy 2016-07-28 12:02:35 UTC
(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...
Comment 217 Ondrej Holy 2016-07-28 13:29:47 UTC
(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."
Comment 218 Felipe Borges 2016-07-28 13:30:10 UTC
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.
Comment 219 Felipe Borges 2016-07-28 13:32:07 UTC
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.
Comment 220 Ondrej Holy 2016-07-28 14:30:26 UTC
(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
Comment 221 Ondrej Holy 2016-07-28 14:44:22 UTC
Review of attachment 332276 [details] [review]:

Looks good, please push once the attachment 331890 [details] [review] is ready.
Comment 222 Ondrej Holy 2016-07-28 14:44:25 UTC
Review of attachment 332277 [details] [review]:

Look good!
Comment 223 Felipe Borges 2016-07-29 13:26:24 UTC
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.
Comment 224 Ondrej Holy 2016-07-29 14:07:48 UTC
Review of attachment 332351 [details] [review]:

Looks good, I think we can now push it also with other patches...
Comment 225 Ondrej Holy 2016-07-29 14:10:43 UTC
Although it would be nice to propose patch for the user icon also ;-)

Also wasn't there patch expanding the account type buttons?
Comment 226 Ondrej Holy 2016-07-29 14:17:29 UTC
Created attachment 332353 [details] [review]
user-accounts: Fix history dialog height

Here is a workaround for the history dialog height...
Comment 227 Ondrej Holy 2016-07-29 14:22:13 UTC
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...
Comment 228 Felipe Borges 2016-08-01 07:23:15 UTC
Attachment 331806 [details] pushed as a053750 - user-accounts: Remove UmEditableButton object
Attachment 332277 [details] pushed as e2d3b47 - user-accounts: Drop usage of UmEditableButtons
Comment 229 Felipe Borges 2016-08-01 07:31:51 UTC
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 230 Felipe Borges 2016-08-01 07:36:43 UTC
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 231 Felipe Borges 2016-08-01 07:40:01 UTC
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"
Comment 232 Felipe Borges 2016-08-01 08:28:36 UTC
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).
Comment 233 Felipe Borges 2016-08-01 08:29:04 UTC
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.
Comment 234 Felipe Borges 2016-08-01 08:29:21 UTC
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.
Comment 235 Felipe Borges 2016-08-01 08:31:33 UTC
(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.
Comment 236 Ondrej Holy 2016-08-01 12:21:51 UTC
Review of attachment 332463 [details] [review]:

um-arrow-frame.c is missing in the patch...
Comment 237 Ondrej Holy 2016-08-01 12:21:53 UTC
Review of attachment 332464 [details] [review]:

Looks good!
Comment 238 Ondrej Holy 2016-08-01 12:21:57 UTC
Review of attachment 332465 [details] [review]:

Looks good!
Comment 239 Felipe Borges 2016-08-01 12:43:59 UTC
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).
Comment 240 Felipe Borges 2016-08-01 12:53:30 UTC
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
Comment 241 Georges Basile Stavracas Neto 2016-08-01 22:07:09 UTC
(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.
Comment 242 Felipe Borges 2016-08-04 15:13:07 UTC
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 243 Felipe Borges 2016-08-04 15:15:01 UTC
Comment on attachment 332353 [details] [review]
user-accounts: Fix history dialog height

Attachment 332353 [details] pushed as 77c26aa - user-accounts: Fix history dialog height
Comment 244 Felipe Borges 2016-08-05 12:08:33 UTC
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.
Comment 245 Felipe Borges 2016-08-05 12:08:45 UTC
Created attachment 332796 [details] [review]
user-accounts: Hide UmCarousel when there's just a single user
Comment 246 Felipe Borges 2016-08-05 12:08:57 UTC
Created attachment 332797 [details] [review]
user-accounts: Just set arrow-frame's carousel once
Comment 247 Felipe Borges 2016-08-07 11:30:32 UTC
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.
Comment 248 Ondrej Holy 2016-08-10 08:35:49 UTC
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>?
Comment 249 Ondrej Holy 2016-08-10 08:35:56 UTC
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...
Comment 250 Ondrej Holy 2016-08-10 08:36:02 UTC
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
Comment 251 Ondrej Holy 2016-08-10 08:36:07 UTC
Review of attachment 332797 [details] [review]:

This should be merged into the previous patch.
Comment 252 Ondrej Holy 2016-08-10 08:36:10 UTC
Review of attachment 332883 [details] [review]:

You should merge this into the initial UmCarousel patch, but it makes sense.
Comment 253 Ondrej Holy 2016-08-10 08:42:33 UTC
Created attachment 333050 [details]
Screencast of carousel glitches
Comment 254 Ondrej Holy 2016-09-06 11:45:25 UTC
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...
Comment 255 Felipe Borges 2016-09-06 14:58:35 UTC
(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!
Comment 256 Felipe Borges 2016-11-15 13:42:56 UTC
Created attachment 339932 [details] [review]
user-accounts: Introduce UmCarousel

UmCarousel is a widget which allows switching between users.
Comment 257 Felipe Borges 2016-11-15 13:44:28 UTC
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.
Comment 258 Felipe Borges 2016-11-15 13:50:28 UTC
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.
Comment 259 Felipe Borges 2016-11-15 18:07:26 UTC
Created attachment 339957 [details] [review]
user-accounts: Introduce UmCarousel

UmCarousel is a widget which allows switching between users.
Comment 260 Ondrej Holy 2016-11-16 15:00:27 UTC
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);
Comment 261 Ondrej Holy 2016-11-16 15:00:44 UTC
Please don't forget that user icon should be also changed as per the mockup...
Comment 262 Felipe Borges 2016-11-16 15:18:35 UTC
(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.
Comment 263 Felipe Borges 2016-11-16 15:36:39 UTC
Created attachment 340021 [details] [review]
user-accounts: Introduce UmCarousel

UmCarousel is a widget which allows switching between users.
Comment 264 Ondrej Holy 2016-11-16 18:43:28 UTC
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...
Comment 265 Felipe Borges 2016-11-25 17:34:46 UTC
(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.
Comment 266 Felipe Borges 2016-11-27 16:04:14 UTC
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.
Comment 267 Ondrej Holy 2016-11-28 08:49:28 UTC
Review of attachment 340847 [details] [review]:

Looks good to me, thanks!
Comment 268 Felipe Borges 2016-11-29 14:23:59 UTC
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
Comment 269 Felipe Borges 2016-12-02 11:26:17 UTC
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.
Comment 270 Felipe Borges 2016-12-02 11:35:11 UTC
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.
Comment 271 Felipe Borges 2016-12-02 11:35:17 UTC
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.
Comment 272 Felipe Borges 2016-12-02 11:35:21 UTC
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.
Comment 273 Felipe Borges 2016-12-02 11:37:36 UTC
In the following patch, I will introduce the Arrow.
Comment 274 Ondrej Holy 2016-12-02 15:34:33 UTC
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...
Comment 275 Felipe Borges 2016-12-02 16:49:00 UTC
(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.
Comment 276 Felipe Borges 2016-12-02 16:49:41 UTC
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
Comment 277 Felipe Borges 2016-12-02 16:50:01 UTC
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
Comment 278 Ondrej Holy 2016-12-05 13:13:23 UTC
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...
Comment 279 Ondrej Holy 2016-12-05 13:13:35 UTC
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...
Comment 280 Felipe Borges 2016-12-05 15:40:09 UTC
(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.
Comment 281 Felipe Borges 2016-12-05 15:42:10 UTC
(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.
Comment 282 Felipe Borges 2016-12-05 15:42:48 UTC
Created attachment 341406 [details] [review]
user-accounts: Set user options sensitive based on the permissions
Comment 283 Felipe Borges 2016-12-05 15:42:58 UTC
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
Comment 284 Felipe Borges 2016-12-05 15:43:08 UTC
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
Comment 285 Felipe Borges 2016-12-05 15:52:07 UTC
(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. :)
Comment 286 Felipe Borges 2016-12-10 21:53:18 UTC
Created attachment 341725 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 287 Felipe Borges 2016-12-10 21:53:34 UTC
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.
Comment 288 Felipe Borges 2016-12-10 22:10:10 UTC
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.
Comment 289 Felipe Borges 2016-12-10 22:31:15 UTC
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.
Comment 290 Ondrej Holy 2016-12-13 10:41:41 UTC
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...
Comment 291 Ondrej Holy 2016-12-13 10:41:59 UTC
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.
Comment 292 Ondrej Holy 2016-12-13 10:42:05 UTC
Review of attachment 341408 [details] [review]:

It seems that the user-accounts-dialog.ui changes are merged in the previous patch by mistake...
Comment 293 Ondrej Holy 2016-12-13 10:42:54 UTC
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...
Comment 294 Ondrej Holy 2016-12-13 10:42:58 UTC
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].
Comment 295 Ondrej Holy 2016-12-13 10:43:10 UTC
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?
Comment 296 Ondrej Holy 2016-12-13 10:43:17 UTC
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?
Comment 297 Felipe Borges 2016-12-13 17:11:20 UTC
(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?
Comment 298 Ondrej Holy 2016-12-14 11:37:49 UTC
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...
Comment 299 Felipe Borges 2016-12-18 12:59:37 UTC
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
Comment 300 Felipe Borges 2016-12-18 13:00:03 UTC
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
Comment 301 Felipe Borges 2016-12-18 13:00:21 UTC
Created attachment 342162 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 302 Felipe Borges 2016-12-18 13:09:19 UTC
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.
Comment 303 Felipe Borges 2016-12-18 13:09:21 UTC
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.
Comment 304 Felipe Borges 2016-12-18 13:50:33 UTC
Created attachment 342164 [details] [review]
user-accounts: Make the user name label bold in the UmCarouselItem
Comment 305 Felipe Borges 2016-12-18 13:50:46 UTC
Created attachment 342165 [details] [review]
user-accounts: Add "Your account" label below proper UmCarouselItem
Comment 306 Felipe Borges 2016-12-18 14:01:11 UTC
Created attachment 342168 [details] [review]
user-accounts: Add "Your account" label below proper UmCarouselItem
Comment 307 Felipe Borges 2016-12-18 14:05:31 UTC
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.
Comment 308 Ondrej Holy 2016-12-20 10:01:50 UTC
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.
Comment 309 Ondrej Holy 2016-12-20 10:02:03 UTC
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...
Comment 310 Ondrej Holy 2016-12-20 10:02:08 UTC
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)...
Comment 311 Ondrej Holy 2016-12-20 10:02:11 UTC
Review of attachment 342164 [details] [review]:

Looks good!
Comment 312 Ondrej Holy 2016-12-20 10:02:16 UTC
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.
Comment 313 Ondrej Holy 2016-12-20 10:02:20 UTC
Review of attachment 342169 [details] [review]:

You should probably merge this with attachment 342168 [details] [review]...
Comment 314 Ondrej Holy 2016-12-20 11:08:53 UTC
(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...
Comment 315 Felipe Borges 2016-12-20 11:10:27 UTC
(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.
Comment 316 Felipe Borges 2016-12-20 14:48:20 UTC
(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.
Comment 317 Felipe Borges 2016-12-20 17:54:45 UTC
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
Comment 318 Felipe Borges 2016-12-20 17:54:59 UTC
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
Comment 319 Felipe Borges 2016-12-20 17:55:13 UTC
Created attachment 342274 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 320 Felipe Borges 2016-12-20 17:55:26 UTC
Created attachment 342275 [details] [review]
user-accounts: Make the user name label bold in the UmCarouselItem
Comment 321 Felipe Borges 2016-12-20 17:55:39 UTC
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.
Comment 322 Felipe Borges 2016-12-20 18:51:30 UTC
Now the ARROW_SIZE and the carousel border_width are equivalent to the mockup (I have measured the mockup image properly).
Comment 323 Ondrej Holy 2016-12-21 12:15:07 UTC
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...
Comment 324 Felipe Borges 2016-12-21 12:59:19 UTC
(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.
Comment 325 Felipe Borges 2016-12-21 15:01:45 UTC
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
Comment 326 Felipe Borges 2016-12-21 15:01:56 UTC
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
Comment 327 Felipe Borges 2016-12-21 15:02:07 UTC
Created attachment 342330 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 328 Felipe Borges 2016-12-21 15:02:17 UTC
Created attachment 342331 [details] [review]
user-accounts: Make the user name label bold in the UmCarouselItem
Comment 329 Felipe Borges 2016-12-21 15:02:29 UTC
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.
Comment 330 Felipe Borges 2016-12-21 15:02:39 UTC
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).
Comment 331 Felipe Borges 2016-12-21 15:06:28 UTC
(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.
Comment 332 Ondrej Holy 2016-12-22 10:26:56 UTC
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);
Comment 333 Ondrej Holy 2016-12-22 10:27:10 UTC
Review of attachment 342329 [details] [review]:

Looks good to me...
Comment 334 Ondrej Holy 2016-12-22 10:27:14 UTC
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);
Comment 335 Ondrej Holy 2016-12-22 10:27:17 UTC
Review of attachment 342331 [details] [review]:

Looks good to me...
Comment 336 Ondrej Holy 2016-12-22 10:27:20 UTC
Review of attachment 342332 [details] [review]:

Looks good to me...
Comment 337 Ondrej Holy 2016-12-22 10:27:24 UTC
Review of attachment 342333 [details] [review]:

Looks good to me...
Comment 338 Ondrej Holy 2016-12-22 10:30:03 UTC
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...
Comment 339 Felipe Borges 2016-12-23 21:37:27 UTC
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
Comment 340 Felipe Borges 2016-12-23 21:38:24 UTC
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).
Comment 341 Felipe Borges 2016-12-23 22:51:26 UTC
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
Comment 342 Felipe Borges 2016-12-23 22:54:41 UTC
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.
Comment 343 Felipe Borges 2016-12-23 22:58:53 UTC
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
Comment 344 Felipe Borges 2016-12-23 22:59:37 UTC
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).
Comment 345 Felipe Borges 2016-12-23 23:02:29 UTC
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).
Comment 346 Felipe Borges 2016-12-23 23:03:34 UTC
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.
Comment 347 Ondrej Holy 2017-01-03 07:40:02 UTC
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...
Comment 348 Ondrej Holy 2017-01-03 07:40:25 UTC
Review of attachment 342434 [details] [review]:

Ok...
Comment 349 Felipe Borges 2017-02-08 13:24:16 UTC
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
Comment 350 Felipe Borges 2017-02-08 13:24:32 UTC
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
Comment 351 Felipe Borges 2017-02-08 13:24:43 UTC
Created attachment 345213 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 352 Felipe Borges 2017-02-08 13:24:54 UTC
Created attachment 345214 [details] [review]
user-accounts: Make the user name label bold in the UmCarouselItem
Comment 353 Felipe Borges 2017-02-08 13:25:05 UTC
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.
Comment 354 Felipe Borges 2017-02-08 13:25:17 UTC
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).
Comment 355 Felipe Borges 2017-02-08 13:30:57 UTC
Review of attachment 342378 [details] [review]:

sure. lgtm
Comment 356 Felipe Borges 2017-02-08 13:31:00 UTC
Review of attachment 342378 [details] [review]:

sure. lgtm
Comment 357 Felipe Borges 2017-02-08 13:32:25 UTC
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!
Comment 358 Ondrej Holy 2017-02-08 14:40:13 UTC
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.
Comment 359 Felipe Borges 2017-02-08 15:32:23 UTC
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
Comment 360 Felipe Borges 2017-02-08 15:32:34 UTC
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
Comment 361 Felipe Borges 2017-02-08 15:32:46 UTC
Created attachment 345242 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 362 Felipe Borges 2017-02-08 15:32:57 UTC
Created attachment 345243 [details] [review]
user-accounts: Make the user name label bold in the UmCarouselItem
Comment 363 Felipe Borges 2017-02-08 15:33:09 UTC
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.
Comment 364 Felipe Borges 2017-02-08 15:33:22 UTC
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).
Comment 365 Ondrej Holy 2017-02-09 08:30:51 UTC
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... :-(
Comment 366 Felipe Borges 2017-02-09 13:37:33 UTC
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
Comment 367 Felipe Borges 2017-02-09 13:37:57 UTC
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
Comment 368 Felipe Borges 2017-02-09 13:38:50 UTC
Created attachment 345315 [details] [review]
user-accounts: Draw arrow in UmCarousel

The arrow is drawn using CSS.
Comment 369 Felipe Borges 2017-02-09 13:39:20 UTC
Created attachment 345316 [details] [review]
user-accounts: Make the user name label bold in the UmCarouselItem
Comment 370 Felipe Borges 2017-02-09 13:39:53 UTC
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.
Comment 371 Felipe Borges 2017-02-09 13:40:14 UTC
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).
Comment 372 Felipe Borges 2017-02-09 13:42:19 UTC
(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.
Comment 373 Ondrej Holy 2017-02-09 17:28:49 UTC
Great! I think that the patches are finally ready to push!
Comment 374 Ondrej Holy 2017-02-09 17:31:57 UTC
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?
Comment 375 Ondrej Holy 2017-02-09 17:36:26 UTC
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.
Comment 376 Ondrej Holy 2017-02-09 17:37:14 UTC
It seems that I set the importance of this bug to high/critical before some time accidentally, resetting to normal/enhancement.
Comment 377 Felipe Borges 2017-02-09 18:30:19 UTC
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
Comment 378 Felipe Borges 2017-02-09 18:31:22 UTC
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