Merge lp:~elementary-pantheon/pantheon-greeter/loginbox-ui-creation into lp:~elementary-pantheon/pantheon-greeter/trunk

[Danielle Foré]

LoginBox:
* Use grid row and column spacing instead of widget margins
* Code style cleanups
* Don't set label opacity because we either pack or don't
* Remove settings_dummy
* Remove unnecessary cairo drawing code
* Remove obvious comments
* Update license header
* Connect signals in a method
* Use Entry.activate instead of listening for specific keys
* Rename variables to make them more obvious
* Use bind instead of connect to show/hide popover

Trying to make this code more legible T_T

Instead of having 100 different methods to create widgets, just create them all as normal.

Something I have an open question about is if doing the if/else logic to pack widgets there makes sense or if there should be a method like "get_login_widget" so there is only one pack and the if/else happens far away.

Try to make some of these variable names more obvious. names like "label" and "pop" suck.

There's a lot of extra and unnecessary stuff here that is just handled by using Gtk instead of drawing everything manually in Clutter or Cairo. One of these is setting the label opacity. Since that's Gtk now, we either just pack it into the grid or we pack the entry. We don't have to worry about one overlapping the other.

[lp:~elementary-pantheon/pantheon-greeter/loginbox-ui-creation]

433. By Danielle Foré on 2016-04-18

use listbox for menuitems like the indicators

[Corentin Noël (tintou)]

I'm okay with the changes, just a few tips in the inline diff

[lp:~elementary-pantheon/pantheon-greeter/loginbox-ui-creation]

434. By Danielle Foré on 2016-04-18

update with Corentin's comments

435. By Danielle Foré on 2016-04-18

use bind instead of connect to show/hide popover

436. By Danielle Foré on 2016-04-18

no need to declare variables in global scope

[Corentin Noël (tintou)]

Everything is fine on my end, very nice cleanup

review: Approve

=== modified file 'src/CredentialsArea.vala'
--- src/CredentialsArea.vala	2016-04-13 00:49:19 +0000
+++ src/CredentialsArea.vala	2016-04-18 22:21:59 +0000
@@ -36,7 +36,6 @@
 
     void create_password_field () {
         password = new Entry ();
-        password.margin_top = 24;
 
         password.set_icon_from_icon_name (Gtk.EntryIconPosition.PRIMARY, 
                 "dialog-password-symbolic");
@@ -83,8 +82,6 @@
             replied ("");
         });
 
-        login_btn.margin_top = 32;
-
         login_btn.get_style_context ().add_class ("suggested-action");
 
         attach (login_btn, 0, 1, 1, 1);

=== modified file 'src/LoginBox.vala'
--- src/LoginBox.vala	2016-04-18 18:23:18 +0000
+++ src/LoginBox.vala	2016-04-18 22:21:59 +0000
@@ -1,30 +1,29 @@
 // -*- Mode: vala; indent-tabs-mode: nil; tab-width: 4 -*-
-/***
-    BEGIN LICENSE
-
-    Copyright (C) 2011-2014 elementary Developers
-
-    This program is free software: you can redistribute it and/or modify it
-    under the terms of the GNU Lesser General Public License version 3, as published
-    by the Free Software Foundation.
-
-    This program is distributed in the hope that it will be useful, but
-    WITHOUT ANY WARRANTY; without even the implied warranties of
-    MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-    PURPOSE.  See the GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License along
-    with this program.  If not, see <http://www.gnu.org/licenses/>
-
-    END LICENSE
-***/
+/*
+* Copyright (c) 2011-2016 elementary LLC. (http://launchpad.net/pantheon-greeter)
+*
+* This program is free software; you can redistribute it and/or
+* modify it under the terms of the GNU General Public
+* License as published by the Free Software Foundation; either
+* version 2 of the License, or (at your option) any later version.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+* General Public License for more details.
+*
+* You should have received a copy of the GNU General Public
+* License along with this program; if not, write to the
+* Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+* Boston, MA 02111-1307, USA.
+*
+*/
 
 public class LoginBox : GtkClutter.Actor, LoginMask {
 
     SelectableAvatar avatar = null;
 
     CredentialsAreaActor credentials_actor;
-    Gtk.Label label;
 
     LoginOption user;
 
@@ -39,23 +38,15 @@
             credentials_actor.remove_credentials ();
             if (value) {
                 opacity = 255;
-                if (avatar != null)
+                if (avatar != null) {
                     avatar.select ();
-
-                // LoginOption is not providing a name, so the CredentialsArea
-                // will display a Gtk.Entry for that and we need to hide
-                // the label that would otherwise be at the same position
-                // as the mentioned entry.
-                if (!user.provides_login_name) {
-                    label.set_opacity (0);
                 }
+
             } else {
 
                 if (avatar != null) {
                     avatar.deselect ();
                 }
-
-                label.set_opacity (255);
             }
 
             credentials_actor.save_easing_state ();
@@ -108,14 +99,16 @@
     }
 
     void update_avatar () {
-        if (avatar != null)
+        if (avatar != null) {
             avatar.dismiss ();
+        }
 
         avatar = new SelectableAvatar (user);
         add_child (avatar);
 
-        if (selected)
+        if (selected) {
             avatar.select ();
+        }
     }
 
     public void pass_focus () {
@@ -202,14 +195,11 @@
          * LoginGateway.
          */
         public signal void replied (string text);
-
         public signal void entered_login_name (string name);
 
         Gtk.Entry? login_name_entry = null;
-        Gtk.ToggleButton settings;
-
-        // Grid that contains all elements of the ui
         Gtk.Grid grid;
+        Gtk.ListBox settings_list;
 
         LoginBox login_box;
 
@@ -225,53 +215,74 @@
             height = 188;
             credentials = null;
 
+            var login_name_label = new Gtk.Label (login_option.get_markup ());
+            login_name_label.get_style_context ().add_class ("h2");
+            login_name_label.set_xalign (0);
+            login_name_label.width_request = 260;
+
+            login_name_entry = new Gtk.Entry ();
+            login_name_entry.halign = Gtk.Align.START;
+            login_name_entry.set_icon_from_icon_name (Gtk.EntryIconPosition.PRIMARY, "avatar-default-symbolic");
+            login_name_entry.set_icon_from_icon_name (Gtk.EntryIconPosition.SECONDARY, "go-jump-symbolic");
+            login_name_entry.width_request = 260;
+
+            var settings = new Gtk.ToggleButton ();
+            settings.get_style_context ().add_class (Gtk.STYLE_CLASS_FLAT);
+            settings.image = new Gtk.Image.from_icon_name ("application-menu-symbolic", Gtk.IconSize.MENU);
+            settings.set_size_request (32, 32);
+            settings.valign = Gtk.Align.CENTER;
+
+            settings_list = new Gtk.ListBox ();
+            settings_list.margin_bottom = 3;
+            settings_list.margin_top = 3;
+
+            var settings_popover = new Gtk.Popover (settings);
+            settings_popover.position = Gtk.PositionType.BOTTOM;
+            settings_popover.add (settings_list);
+            settings_popover.bind_property ("visible", settings, "active", GLib.BindingFlags.BIDIRECTIONAL);
+
             grid = new Gtk.Grid ();
-            grid.width_request = 260;
+            grid.column_spacing = 6;
+            grid.row_spacing = 12;
 
-            // If the login option doesn't provice a login name, we have to
-            // show a entry for the user to enter one.
-            // This is for example used in the manual login.
             if (login_option.provides_login_name) {
-                var label = new Gtk.Label (login_option.get_markup ());
-                label.get_style_context ().add_class ("h2");
-                label.halign = Gtk.Align.START;
-
-                grid.attach (label, 0, 0, 1, 1);
+                grid.attach (login_name_label, 0, 0, 1, 1);
             } else {
-                create_login_name_entry ();
+                grid.attach (login_name_entry, 0, 0, 1, 1);
             }
 
-            // Only show settings if we actually have more than one session
-            // to select from
             if (LightDM.get_sessions ().length () > 1) {
-                create_settings ();
-            } else {
-                // Dummy for the settings-button or the grid-layout goes apeshit
-                create_settings_dummy ();
+                create_settings_items ();
+                grid.attach (settings, 1, 0, 1, 1);
             }
 
-            var w = -1; var h = -1;
-            this.get_widget ().size_allocate.connect (() => {
-                w = this.get_widget ().get_allocated_width ();
-                h = this.get_widget ().get_allocated_height ();
-            });
-
-            // We override the draw call and just paint a transparent
-            // rectangle. TODO: can we also just draw nothing?
-            // Shouldn't change anything...
-            this.get_widget ().draw.connect ((ctx) => {
-                ctx.rectangle (0, 0, w, h);
-                ctx.set_operator (Cairo.Operator.SOURCE);
-                ctx.set_source_rgba (0, 0, 0, 0);
-                ctx.fill ();
-
-                return false;
-            });
+            connect_signals ();
 
             ((Gtk.Container) this.get_widget ()).add (grid);
             this.get_widget ().show_all ();
         }
 
+        void connect_signals () {
+            login_name_entry.activate.connect (() => {
+                entered_login_name (login_name_entry.text);
+            });
+
+            login_name_entry.focus_in_event.connect ((e) => {
+                remove_credentials ();
+                return false;
+            });
+
+            login_name_entry.icon_press.connect ((pos, event) => {
+                if (pos == Gtk.EntryIconPosition.SECONDARY) {
+                    entered_login_name (login_name_entry.text);
+                }
+            });
+
+            replied.connect ((answer) => {
+                login_name_entry.sensitive = false;
+            });
+        }
+
         public void remove_credentials () {
             if (credentials != null) {
                 grid.remove (credentials);
@@ -323,109 +334,43 @@
                 login_name_entry.sensitive = true;
         }
 
-        void create_login_name_entry () {
-            replied.connect ((answer) => {
-                login_name_entry.sensitive = false;
-            });
-            login_name_entry = new Gtk.Entry ();
-            login_name_entry.set_icon_from_icon_name (Gtk.EntryIconPosition.PRIMARY,
-                    "avatar-default-symbolic");
-            login_name_entry.hexpand = true;
-            login_name_entry.margin_top = 8;
-            login_name_entry.set_icon_from_icon_name (Gtk.EntryIconPosition.SECONDARY, "go-jump-symbolic");
-            login_name_entry.icon_press.connect ((pos, event) => {
-                if (pos == Gtk.EntryIconPosition.SECONDARY) {
-                    entered_login_name (login_name_entry.text);
-                }
-            });
-            login_name_entry.key_release_event.connect ((e) => {
-                if (e.keyval == Gdk.Key.Return || e.keyval == Gdk.Key.KP_Enter) {
-                    entered_login_name (login_name_entry.text);
-                    return true;
-                } else {
-                    return false;
-                }
-            });
-            login_name_entry.focus_in_event.connect ((e) => {
-                remove_credentials ();
-                return false;
-            });
-            grid.attach (login_name_entry, 0, 0, 1, 1);
-        }
-
-        void create_settings () {
-            settings = new Gtk.ToggleButton ();
-            settings.margin_left = 6;
-            settings.margin_top = 6;
-            settings.get_style_context ().add_class (Gtk.STYLE_CLASS_FLAT);
-            settings.add (new Gtk.Image.from_icon_name ("application-menu-symbolic", Gtk.IconSize.MENU));
-            settings.valign = Gtk.Align.START;
-            settings.set_size_request (30, 30);
-            grid.attach (settings, 1, 0, 1, 1);
-            create_popup ();
-        }
-
-        /**
-         * Creates a invisible settings-dummy that has
-         * the dimension of the settings-button but is invisible.
-         * Prevents that the layout is different when there are is
-         * no settings-menu.
-         */
-        void create_settings_dummy () {
-            var dummy_grid = new Gtk.Grid ();
-            dummy_grid.set_size_request (30, 30);
-            grid.attach (dummy_grid, 1, 0, 1, 1);
-        }
-
-        void create_popup () {
-            Gtk.Popover pop = null;
-            /*session choose popover*/
-            this.settings.toggled.connect (() => {
-
-                if (!settings.active) {
-                    pop.destroy ();
-                    return;
-                }
-
-                pop = new Gtk.Popover (settings);
-                pop.set_position (Gtk.PositionType.BOTTOM);
-
-                var grid = new Gtk.Grid ();
-                grid.margin_bottom = 3;
-                grid.margin_top = 3;
-                grid.orientation = Gtk.Orientation.VERTICAL;
-                pop.add (grid);
-
-                var but = new Gtk.RadioButton.with_label (null, LightDM.get_sessions ().nth_data (0).name);
-                but.get_style_context ().add_class (Gtk.STYLE_CLASS_MENUITEM);
-                grid.add (but);
-                but.active = LightDM.get_sessions ().nth_data (0).key == current_session;
-
-                but.toggled.connect (() => {
-                    if (but.active)
-                        current_session = LightDM.get_sessions ().nth_data (0).key;
-                });
-
-                for (var i = 1;i < LightDM.get_sessions ().length (); i++) {
-                    var rad = new Gtk.RadioButton.with_label_from_widget (but, LightDM.get_sessions ().nth_data (i).name);
-                    rad.get_style_context ().add_class (Gtk.STYLE_CLASS_MENUITEM);
-                    grid.add (rad);
-
-                    rad.active = LightDM.get_sessions ().nth_data (i).key == current_session;
-                    var identifier = LightDM.get_sessions ().nth_data (i).key;
-                    rad.toggled.connect ( () => {
-                        if (rad.active)
-                            current_session = identifier;
-                    });
-                }
-
-                pop.show_all ();
-
-                pop.closed.connect (() => {
-                    settings.active = false;
-                });
-            });
-        }
-
+        void create_settings_items () {
+            var button = new Gtk.RadioButton.with_label (null, LightDM.get_sessions ().nth_data (0).name);
+            button.margin_left = 6;
+            button.margin_right = 6;
+            button.active = LightDM.get_sessions ().nth_data (0).key == current_session;
+
+            button.toggled.connect (() => {
+                if (button.active) {
+                    current_session = LightDM.get_sessions ().nth_data (0).key;
+                }
+            });
+
+            var button_row = new Gtk.ListBoxRow ();
+            button_row.get_style_context ().add_class (Gtk.STYLE_CLASS_MENUITEM);
+            button_row.add (button);
+            settings_list.add (button_row);
+
+            for (var i = 1; i < LightDM.get_sessions ().length (); i++) {
+                var radio = new Gtk.RadioButton.with_label_from_widget (button, LightDM.get_sessions ().nth_data (i).name);
+                radio.margin_left = 6;
+                radio.margin_right = 6;
+
+                var radio_row = new Gtk.ListBoxRow ();
+                radio_row.get_style_context ().add_class (Gtk.STYLE_CLASS_MENUITEM);
+                radio_row.add (radio);
+                settings_list.add (radio_row);
+
+                var identifier = LightDM.get_sessions ().nth_data (i).key;
+                radio.active = identifier == current_session;
+                radio.toggled.connect ( () => {
+                    if (radio.active) {
+                        current_session = identifier;
+                    }
+                });
+            }
+
+            settings_list.show_all ();
+        }
     }
 }