Merge lp:~hyuchia/pantheon-files/fix-1124209 into lp:~elementary-apps/pantheon-files/trunk

[Diego Islas Ocampo]

Modified the Permissions Labels for directories while keeping the previous ones for files.

[Jeremy Wootten (jeremywootten)]

Some minor code style issues (see inline) comments. Also needs merging with latest trunk to remove irrelevant diff contents.

The code is otherwise OK for this solution, however UX input is needed on the wording and whether this is an acceptable interim solution. The word "Modify" is problematic - it implies you can modify the files inside but you can only create and delete them. I am not sure what single word would convey this and "Create/Delete files" would be too long. Ideally, the buttons need replacing with comboboxes as per Dan's mockup in the bug report. This would allow space for more meaningful descriptions of the permissions on directories. However, that is not strictly within the scope of the bitesize bug. Maybe use tooltips for the buttons?

review: Needs Fixing (code)

[Diego Islas Ocampo (hyuchia)]

Ok so using combo boxes instead of buttons, the options required would be:
- Read
- Write
- Execute
- Read and Write
- Read and Execute
- Write and Execute
- Read, Write and Execute

Is that right? Oh and you are right about the code style issues, my bad.

[Jeremy Wootten (jeremywootten)]

Hi Diego, you would need a "No access" option as well and the exact wording would have to be decided by the design team. For example "Read only" might be preferred. Also you would need a different set for folders. I'll ask someone from the design for comment.

[lp:~hyuchia/pantheon-files/fix-1124209]

2509. By Diego Islas Ocampo on 2017-03-01

Replace permission grid buttons with comboboxes

2510. By Diego Islas Ocampo on 2017-03-01

Fix po merging issues

2511. By Diego Islas Ocampo on 2017-03-01

Merge from trunk

2512. By Diego Islas Ocampo on 2017-03-01

Fix directory determination

2513. By Diego Islas Ocampo on 2017-03-01

Remove explicit 'this' markers

[Diego Islas Ocampo (hyuchia)]

All right, so I managed to replace the grid buttons with the combo boxes and make them actually work, as well as merging with the latest trunk. Now I'll wait for the UX team to take a look since the wording and order of the options is up to them I guess. Codewise I'm not sure if that's the best solution but I think it's ok. Thanks!

[Jeremy Wootten (jeremywootten)]

A few inline comments.

[lp:~hyuchia/pantheon-files/fix-1124209]

2514. By Diego Islas Ocampo on 2017-03-04

Remove unnecessary unowned declarations and improve no access labels

[Jeremy Wootten (jeremywootten)]

Another small inline comment but basically waiting for UX team approval of wording now.

[lp:~hyuchia/pantheon-files/fix-1124209]

2515. By Diego Islas Ocampo on 2017-03-07

Keep consistency on permission labels

[Jeremy Wootten (jeremywootten)]

When a mixture of files and folders is selected, the wording for permissions is that for files - but I cannot see any better alternative.

review: Approve (code)

[Danielle Foré (danrabbit)]

It seems like we have a problem where we've made it easier to read which permissions are being applied but we made it much harder to set those permissions. I think this probably needs from more thought from UX team. In the interest of moving to GitHub in a timely manner, I'm going to reject this branch for now. Please re-propose there

Unmerged revisions

2515. By Diego Islas Ocampo on 2017-03-07

Keep consistency on permission labels

2514. By Diego Islas Ocampo on 2017-03-04

Remove unnecessary unowned declarations and improve no access labels

2513. By Diego Islas Ocampo on 2017-03-01

Remove explicit 'this' markers

2512. By Diego Islas Ocampo on 2017-03-01

Fix directory determination

2511. By Diego Islas Ocampo on 2017-03-01

Merge from trunk

2510. By Diego Islas Ocampo on 2017-03-01

Fix po merging issues

2509. By Diego Islas Ocampo on 2017-03-01

Replace permission grid buttons with comboboxes

2508. By Diego Islas Ocampo on 2017-02-25

Remove unnecessary parameters

2507. By Diego Islas Ocampo on 2017-02-23

Add different permission labels to folders

=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2017-02-01 14:42:37 +0000
+++ src/CMakeLists.txt	2017-03-07 02:41:21 +0000
@@ -105,7 +105,7 @@
         View/Widgets/LocationBar.vala
         View/Widgets/MultiLineEditableLabel.vala
         View/Widgets/OverlayBar.vala
-        View/Widgets/PermissionButton.vala
+        View/Widgets/PermissionComboBox.vala
         View/Widgets/ProgressInfoWidget.vala
         View/Widgets/SingleLineEditableLabel.vala
         View/Widgets/TopMenu.vala
@@ -171,7 +171,7 @@
         View/Widgets/LocationBar.vala
         View/Widgets/MultiLineEditableLabel.vala
         View/Widgets/OverlayBar.vala
-        View/Widgets/PermissionButton.vala
+        View/Widgets/PermissionComboBox.vala
         View/Widgets/ProgressInfoWidget.vala
         View/Widgets/SingleLineEditableLabel.vala
         View/Widgets/TopMenu.vala

=== modified file 'src/Dialogs/PropertiesWindow.vala'
--- src/Dialogs/PropertiesWindow.vala	2017-02-10 19:35:18 +0000
+++ src/Dialogs/PropertiesWindow.vala	2017-03-07 02:41:21 +0000
@@ -27,9 +27,9 @@
     private bool perm_code_should_update = true;
     private Gtk.Label l_perm;
 
-    private PermissionButton perm_button_user;
-    private PermissionButton perm_button_group;
-    private PermissionButton perm_button_other;
+    private PermissionComboBox perm_combo_user;
+    private PermissionComboBox perm_combo_group;
+    private PermissionComboBox perm_combo_other;
 
     private Gtk.ListStore store_users;
     private Gtk.ListStore store_groups;
@@ -183,7 +183,7 @@
         construct_info_panel (goffile);
         cancellable = new GLib.Cancellable ();
 
-        update_selection_size (); /* Start counting first to get number of selected files and folders */    
+        update_selection_size (); /* Start counting first to get number of selected files and folders */
 
         /* create some widgets first (may be hidden by update_selection_size ()) */
         var file_pix = goffile.get_icon_pixbuf (48, false, GOF.FileIconFlags.NONE);
@@ -650,41 +650,60 @@
         return false;
     }
 
-    private void update_perm_codes (Permissions.Type pt, int val, int mult) {
+    private void update_perm_codes (Permissions.Type pt, int val) {
         switch (pt) {
-        case Permissions.Type.USER:
-            owner_perm_code += mult*val;
-            break;
-        case Permissions.Type.GROUP:
-            group_perm_code += mult*val;
-            break;
-        case Permissions.Type.OTHER:
-            everyone_perm_code += mult*val;
-            break;
+            case Permissions.Type.USER:
+                owner_perm_code = val;
+                break;
+            case Permissions.Type.GROUP:
+                group_perm_code = val;
+                break;
+            case Permissions.Type.OTHER:
+                everyone_perm_code = val;
+                break;
         }
     }
 
-    private void permission_button_toggle (Gtk.ToggleButton btn) {
-        unowned Permissions.Type pt = btn.get_data ("permissiontype");
-        unowned Permissions.Value permission_value = btn.get_data ("permissionvalue");
-        int mult = 1;
+    private void combo_permissions_changed (Gtk.ComboBox combo) {
+        Permissions.Type pt = ((PermissionComboBox) combo).permission_type;
+        int active_perm = combo.get_active ();
 
         reset_and_cancel_perm_timeout ();
 
-        if (!btn.get_active ()) {
-            mult = -1;
-        }
+        switch (active_perm) {
+            // Read, Write and Execute is selected
+            case 0:
+                update_perm_codes (pt, 7);
+                break;
+            // Write and Execute is selected
+            case 1:
+                update_perm_codes (pt, 3);
+                break;
+            // Read and Execute is selected
+           case 2:
+               update_perm_codes (pt, 5);
+               break;
+           // Read and Write is selected
+           case 3:
+               update_perm_codes (pt, 6);
+               break;
+           // Execute is selected
+           case 4:
+               update_perm_codes (pt, 1);
+               break;
+           // Write is selected
+           case 5:
+               update_perm_codes (pt, 2);
+               break;
+           // Read is selected
+           case 6:
+               update_perm_codes (pt, 4);
+               break;
+           // No permissions selected
+           case 7:
+               update_perm_codes (pt, 0);
+               break;
 
-        switch (permission_value) {
-            case Permissions.Value.READ:
-                update_perm_codes (pt, 4, mult);
-                break;
-            case Permissions.Value.WRITE:
-                update_perm_codes (pt, 2, mult);
-                break;
-            case Permissions.Value.EXE:
-                update_perm_codes (pt, 1, mult);
-                break;
         }
 
         if (perm_code_should_update) {
@@ -692,12 +711,10 @@
         }
     }
 
-    private PermissionButton create_perm_choice (Permissions.Type pt) {
-        var permission_button = new PermissionButton (pt);
-        permission_button.btn_read.toggled.connect (permission_button_toggle);
-        permission_button.btn_write.toggled.connect (permission_button_toggle);
-        permission_button.btn_exe.toggled.connect (permission_button_toggle);
-        return permission_button;
+    private PermissionComboBox create_perm_choice (Permissions.Type pt) {
+        var permission_combo = new PermissionComboBox (pt, goffile.is_directory);
+        permission_combo.changed.connect (combo_permissions_changed);
+        return permission_combo;
     }
 
     private uint32 get_perm_from_chmod_unit (uint32 vfs_perm, int nb,
@@ -737,9 +754,9 @@
 
 
     private void update_perm_grid_toggle_states (uint32 permissions) {
-        perm_button_user.update_buttons (permissions);
-        perm_button_group.update_buttons (permissions);
-        perm_button_other.update_buttons (permissions);
+        perm_combo_user.update (permissions);
+        perm_combo_group.update (permissions);
+        perm_combo_other.update (permissions);
     }
 
     private void reset_and_cancel_perm_timeout () {
@@ -872,13 +889,13 @@
         group_combo.margin_bottom = 12;
 
         var owner_label = new KeyLabel (_("Owner:"));
-        perm_button_user = create_perm_choice (Permissions.Type.USER);
+        perm_combo_user = create_perm_choice (Permissions.Type.USER);
 
         var group_label = new KeyLabel (_("Group:"));
-        perm_button_group = create_perm_choice (Permissions.Type.GROUP);
+        perm_combo_group = create_perm_choice (Permissions.Type.GROUP);
 
         var other_label = new KeyLabel (_("Everyone:"));
-        perm_button_other = create_perm_choice (Permissions.Type.OTHER);
+        perm_combo_other = create_perm_choice (Permissions.Type.OTHER);
 
         perm_code = new Gtk.Entry ();
         perm_code.text = "000";
@@ -896,12 +913,14 @@
         perm_grid.attach (owner_user_choice, 1, 1, 2, 1);
         perm_grid.attach (group_combo_label, 0, 2, 1, 1);
         perm_grid.attach (group_combo, 1, 2, 2, 1);
+
+
         perm_grid.attach (owner_label, 0, 3, 1, 1);
-        perm_grid.attach (perm_button_user, 1, 3, 2, 1);
+        perm_grid.attach (perm_combo_user, 1, 3, 2, 1);
         perm_grid.attach (group_label, 0, 4, 1, 1);
-        perm_grid.attach (perm_button_group, 1, 4, 2, 1);
+        perm_grid.attach (perm_combo_group, 1, 4, 2, 1);
         perm_grid.attach (other_label, 0, 5, 1, 1);
-        perm_grid.attach (perm_button_other, 1, 5, 2, 1);
+        perm_grid.attach (perm_combo_other, 1, 5, 2, 1);
         perm_grid.attach (l_perm, 1, 6, 1, 1);
         perm_grid.attach (perm_code, 2, 6, 1, 1);
 

=== removed file 'src/View/Widgets/PermissionButton.vala'
--- src/View/Widgets/PermissionButton.vala	2017-02-10 19:35:18 +0000
+++ src/View/Widgets/PermissionButton.vala	1970-01-01 00:00:00 +0000
@@ -1,76 +0,0 @@
-/*
-* Copyright (c) 2016-2017 elementary LLC. (http://launchpad.net/pantheon-files)
-*
-* 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, Inc.,; 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., 51 Franklin Street, Fifth Floor,
-* Boston, MA 02110-1335 USA.
-*/
-
-public class PermissionButton : Gtk.Grid {
-    public Gtk.ToggleButton btn_read;
-    public Gtk.ToggleButton btn_write;
-    public Gtk.ToggleButton btn_exe;
-
-    public Permissions.Type permission_type { get; construct; }
-
-    private Posix.mode_t[,] vfs_perms = {
-        { Posix.S_IRUSR, Posix.S_IWUSR, Posix.S_IXUSR },
-        { Posix.S_IRGRP, Posix.S_IWGRP, Posix.S_IXGRP },
-        { Posix.S_IROTH, Posix.S_IWOTH, Posix.S_IXOTH }
-    };
-
-    public PermissionButton (Permissions.Type permission_type) {
-        Object (permission_type: permission_type);
-    }
-
-    construct {
-        btn_read = new Gtk.ToggleButton.with_label (_("Read"));
-        btn_read.set_data ("permissiontype", permission_type);
-        btn_read.set_data ("permissionvalue", Permissions.Value.READ);
-
-        btn_write = new Gtk.ToggleButton.with_label (_("Write"));
-        btn_write.set_data ("permissiontype", permission_type);
-        btn_write.set_data ("permissionvalue", Permissions.Value.WRITE);
-
-        btn_exe = new Gtk.ToggleButton.with_label (_("Execute"));
-        btn_exe.set_data ("permissiontype", permission_type);
-        btn_exe.set_data ("permissionvalue", Permissions.Value.EXE);
-
-        column_homogeneous = true;
-        get_style_context ().add_class (Gtk.STYLE_CLASS_LINKED);
-        add (btn_read);
-        add (btn_write);
-        add (btn_exe);
-    }
-
-    public void update_buttons (uint32 permissions) {
-        if ((permissions & vfs_perms[permission_type, 0]) != 0) {
-            btn_read.active = true;
-        } else {
-            btn_read.active = false;
-        }
-
-        if ((permissions & vfs_perms[permission_type, 1]) != 0) {
-            btn_write.active = true;
-        } else {
-            btn_write.active = false;
-        }
-
-        if ((permissions & vfs_perms[permission_type, 2]) != 0) {
-            btn_exe.active = true;
-        } else {
-            btn_exe.active = false;
-        }
-    }
-}

=== added file 'src/View/Widgets/PermissionComboBox.vala'
--- src/View/Widgets/PermissionComboBox.vala	1970-01-01 00:00:00 +0000
+++ src/View/Widgets/PermissionComboBox.vala	2017-03-07 02:41:21 +0000
@@ -0,0 +1,87 @@
+/*
+* Copyright (c) 2016-2017 elementary LLC. (http://launchpad.net/pantheon-files)
+*
+* 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, Inc.,; 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., 51 Franklin Street, Fifth Floor,
+* Boston, MA 02110-1335 USA.
+*/
+
+public class PermissionComboBox : Gtk.ComboBoxText {
+
+    private bool perm_read;
+    private bool perm_write;
+    private bool perm_exe;
+
+    public bool is_directory { get; construct; }
+
+    public Permissions.Type permission_type { get; construct; }
+
+    private Posix.mode_t[,] vfs_perms = {
+        { Posix.S_IRUSR, Posix.S_IWUSR, Posix.S_IXUSR },
+        { Posix.S_IRGRP, Posix.S_IWGRP, Posix.S_IXGRP },
+        { Posix.S_IROTH, Posix.S_IWOTH, Posix.S_IXOTH }
+    };
+
+    public PermissionComboBox (Permissions.Type permission_type, bool is_directory) {
+        Object (permission_type: permission_type, is_directory: is_directory);
+    }
+
+    construct {
+        if (is_directory){
+             append_text (_("List, Write and Access"));
+             append_text (_("Write and Access"));
+             append_text (_("List and Access"));
+             append_text (_("List and Write"));
+             append_text (_("Access Only"));
+             append_text (_("Write Only"));
+             append_text (_("List Only"));
+             append_text (_("No Permissions"));
+        } else {
+            append_text (_("Read, Write and Execute"));
+            append_text (_("Write and Execute"));
+            append_text (_("Read and Execute"));
+            append_text (_("Read and Write"));
+            append_text (_("Execute Only"));
+            append_text (_("Write Only"));
+            append_text (_("Read Only"));
+            append_text (_("No Permissions"));
+        }
+
+       get_style_context ().add_class (Gtk.STYLE_CLASS_LINKED);
+
+    }
+
+    public void update (uint32 permissions) {
+        perm_read = (permissions & vfs_perms[permission_type, 0]) != 0;
+        perm_write = (permissions & vfs_perms[permission_type, 1]) != 0;
+        perm_exe = (permissions & vfs_perms[permission_type, 2]) != 0;
+        if (perm_read && perm_write && perm_exe) {
+            active = 0;
+        } else if (perm_write && perm_exe) {
+            active = 1;
+        } else if (perm_read && perm_exe) {
+            active = 2;
+        } else if (perm_read && perm_write) {
+            active = 3;
+        } else if (perm_exe) {
+            active = 4;
+        }  else if (perm_write) {
+             active = 5;
+        }  else if (perm_read) {
+              active = 6;
+        } else {
+            active = 7;
+        }
+    }
+}