Merge lp:~jeremywootten/pantheon-files/fix-1046336 into lp:~elementary-apps/pantheon-files/trunk

[Jeremy Wootten]

Fix and enable the function to rename a bookmark to fix bug #108376. Fix the situation where some users were not able to drag folders to the sidebar to create bookmarks to fix bug #1046336. In the last situation, also fix not being able to reorder bookmarks by dragging them around.

1) Create bookmarks by dragging and dropping folders onto the side bar.
2) Re-order bookmarks by dragging and dropping within the sidebar
3) Right-clicking on bookmark gives options to remove or rename.
4) Bookmarks that point to non-existent folders are automatically permanently removed NOTE: It should be considered whether this is the desired behaviour. Alternative options are to hide the bookmark or leave it alone.

[Julián Unrrein (junrrein)]

Hi Jeremy

After some testing I can say the following about the implemented features:

1) Was this feature not working for you before? How did you test this? Should we get people from the bug report to test this?

2) Works as intended. No problems.

3) Works as intended too.

4) Bookmarks pointing to non-existent folders are removed only when the bookmark list changes (on bookmark creation and deletion).
I guess your other branch (lp:~jeremywootten/pantheon-files/fix-1207655) solves this properly.
We still have to get confirmation from the design team that this is the intended behavior.

Overall, these are nice little things that were sorely needed!

Code review is coming later.

PS: Come on Jeremy, 4 fixes packed in 1 branch? You know our guidelines are against that. We will let you get by, one more time :)

[Julián Unrrein (junrrein)]

2) I didn't notice this before, but I can re-order bookmarks in current trunk code. Was this not working for you?

[Julián Unrrein (junrrein)]

Only code comment:

Diff line 189: Why was PlaceType changed to gint?

[Jeremy Wootten (jeremywootten)]

Thanks for the review Julián. No, none of this works for me in the current
trunk (last tried last week-end - revision 1280). I noticed in the comment
to the bug that bookmark creation worked for some and not others -
something to do with 32 bit vs 64 bit users?? I am still on 32 bit :(

I changed PlaceType to gint because the treestore holds the data as gint
and I was getting strange errors comparing the retrieved data with the
placetype otherwise. That's why I introduced the boolean is_bookmark - to
avoid keep making the comparison.

Sorry to have included too many changes - I'll break them up more in future.

On 6 August 2013 20:38, Julián Unrrein <email address hidden> wrote:

> Hi Jeremy
>
> After some testing I can say the following about the implemented features:
>
> 1) Was this feature not working for you before? How did you test this?
> Should we get people from the bug report to test this?
>
> 2) Works as intended. No problems.
>
> 3) Works as intended too.
>
> 4) Bookmarks pointing to non-existent folders are removed only when the
> bookmark list changes (on bookmark creation and deletion).
> I guess your other branch (lp:~jeremywootten/pantheon-files/fix-1207655)
> solves this properly.
> We still have to get confirmation from the design team that this is the
> intended behavior.
>
> Overall, these are nice little things that were sorely needed!
>
> Code review is coming later.
>
> PS: Come on Jeremy, 4 fixes packed in 1 branch? You know our guidelines
> are against that. We will let you get by, one more time :)
>
>
> --
>
> https://code.launchpad.net/~jeremywootten/pantheon-files/fix-1046336/+merge/178420
> You are the owner of lp:~jeremywootten/pantheon-files/fix-1046336.
>

--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD

[lp:~jeremywootten/pantheon-files/fix-1046336]

1281. By Jeremy Wootten on 2013-08-07

Disable automatic removal of bookmarks

[Jeremy Wootten (jeremywootten)]

On reflection I decided to disable the automatic removal of "non-existent"
bookmarks from the bookmark list, at least for the time being, because it
may not be appropriate to remove bookmarks linked to folders on unmounted
filesystems and, at present, the code does not distinguish between folders
that have truly been deleted and those that are temporarily inaccessible.
 Such bookmarks are not hidden now either so that they are not forgotten. I
think it is better to let the used decide whether to keep or remove a
bookmark. I have pushed a new revision.

I'll change the other branch (lp:~jeremywootten/pantheon-files/fix-1207655)
accordingly later.

On 6 August 2013 20:38, Julián Unrrein <email address hidden> wrote:

> Hi Jeremy
>
> After some testing I can say the following about the implemented features:
>
> 1) Was this feature not working for you before? How did you test this?
> Should we get people from the bug report to test this?
>
> 2) Works as intended. No problems.
>
> 3) Works as intended too.
>
> 4) Bookmarks pointing to non-existent folders are removed only when the
> bookmark list changes (on bookmark creation and deletion).
> I guess your other branch (lp:~jeremywootten/pantheon-files/fix-1207655)
> solves this properly.
> We still have to get confirmation from the design team that this is the
> intended behavior.
>
> Overall, these are nice little things that were sorely needed!
>
> Code review is coming later.
>
> PS: Come on Jeremy, 4 fixes packed in 1 branch? You know our guidelines
> are against that. We will let you get by, one more time :)
>
>
> --
>
> https://code.launchpad.net/~jeremywootten/pantheon-files/fix-1046336/+merge/178420
> You are the owner of lp:~jeremywootten/pantheon-files/fix-1046336.
>

--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD

[Julián Unrrein (junrrein)]

The issue of not being able to drag bookmarks to the sidebar has been here for a lot of time. As you saw in the bug report, that affects some (but not all) of the users, independently of architecture. The fact that you found out have to fix this is awesome. The PlaceType thing you explained me might be related to the issue.

About the automatic bookmark removal, I understand your concern, but that change doesn't belong in this branch. There shouldn't be anything else aside from fixing the linked bugs.

Please reinstate the existing behavior (diff lines 79-81) and we will leave this issue to the other branch.

[Cody Garver (codygarver)]

This is ready to go?

[Julián Unrrein (junrrein)]

No. From my previous comment:

"Please reinstate the existing behavior (diff lines 79-81) and we will leave this issue to the other branch."

[lp:~jeremywootten/pantheon-files/fix-1046336]

1282. By Jeremy Wootten on 2013-08-10

Reinstate hiding of unavailable bookmarks

[Jeremy Wootten (jeremywootten)]

OK, I have reinstated the existing behaviour.

This branch now only address the adding of bookmarks by drag and drop and
the manual removal and editing of bookmarks using the context menu.

There are still flaws in the behaviour when a folder is renamed. The
corresponding bookmark name does not immediately change although it does
change if you restart the program. Also if you reorder the bookmark list
after renaming a folder (but before restarting the program) the
corresponding bookmark will show garbage for about a second and then be
correctly renamed after the automatic saving and reloading of the bookmark
list has completed. Bookmarks that have been manually renamed behave
differently - they become "unavailable" so they are hidden when the
bookmark list is reordered or reloaded.

I will try to address these issues in the other branch.

On 7 August 2013 22:31, Julián Unrrein <email address hidden> wrote:

> The issue of not being able to drag bookmarks to the sidebar has been here
> for a lot of time. As you saw in the bug report, that affects some (but not
> all) of the users, independently of architecture. The fact that you found
> out have to fix this is awesome. The PlaceType thing you explained me might
> be related to the issue.
>
> About the automatic bookmark removal, I understand your concern, but that
> change doesn't belong in this branch. There shouldn't be anything else
> aside from fixing the linked bugs.
>
> Please reinstate the existing behavior (diff lines 79-81) and we will
> leave this issue to the other branch.
> --
>
> https://code.launchpad.net/~jeremywootten/pantheon-files/fix-1046336/+merge/178420
> You are the owner of lp:~jeremywootten/pantheon-files/fix-1046336.
>

--
Jeremy Wootten
GPG Key ID CB585BCD
Key Fingerprint 37C0 3C2A A6D4 E45B BA7C 4328 2DF2 1882 CB58 5BCD

[Julián Unrrein (junrrein)]

I tested again. My comments about the "rename folder" functionality:

When a folder is renamed, the bookmark name doesn't change, and clicking on it leads to a non-existent folder.
However, the bookmark item is updated under the conditions you described (restarting Files, or reordering bookmarks).

I agree with you that this is faulty behavior, but the tradeoffs are very much worth it.

Thanks for your work!

review: Approve

=== modified file 'libcore/marlin-abstract-sidebar.c'
--- libcore/marlin-abstract-sidebar.c	2012-11-06 17:21:39 +0000
+++ libcore/marlin-abstract-sidebar.c	2013-08-10 16:49:01 +0000
@@ -60,19 +60,19 @@
 {
     /* this is required to set the category cells to bold and higher than the other ones */
     self->store = gtk_tree_store_new ((gint) PLACES_SIDEBAR_COLUMN_COUNT,
-                                      G_TYPE_INT, 
-                                      G_TYPE_STRING,
+                                      G_TYPE_INT,       /* row type */
+                                      G_TYPE_STRING,    /* uri */
                                       G_TYPE_DRIVE,
                                       G_TYPE_VOLUME,
                                       G_TYPE_MOUNT,
-                                      G_TYPE_STRING,
-                                      G_TYPE_ICON,    /* Primary icon */
-                                      G_TYPE_INT,
-                                      G_TYPE_BOOLEAN,
-                                      G_TYPE_BOOLEAN,
-                                      G_TYPE_BOOLEAN,
-                                      G_TYPE_STRING,
-                                      G_TYPE_ICON,    /* Action icon (e.g. eject button) */
-                                      G_TYPE_UINT64,  /* Free space */
-                                      G_TYPE_UINT64); /* For disks, total size */
+                                      G_TYPE_STRING,    /* name */
+                                      G_TYPE_ICON,      /* Primary icon */
+                                      G_TYPE_INT,       /* index */
+                                      G_TYPE_BOOLEAN,   /* eject */
+                                      G_TYPE_BOOLEAN,   /* no eject */
+                                      G_TYPE_BOOLEAN,   /* is bookmark */
+                                      G_TYPE_STRING,    /* tool tip */
+                                      G_TYPE_ICON,      /* Action icon (e.g. eject button) */
+                                      G_TYPE_UINT64,    /* Free space */
+                                      G_TYPE_UINT64);   /* For disks, total size */
 }

=== modified file 'src/marlin-bookmark-list.c'
--- src/marlin-bookmark-list.c	2011-11-01 12:09:26 +0000
+++ src/marlin-bookmark-list.c	2013-08-10 16:49:01 +0000
@@ -520,7 +520,6 @@
         GList *files = marlin_bookmark_list_get_gof_files (bookmarks);
         gof_call_when_ready_new (files, bookmark_list_content_changed, bookmarks);
         g_list_free (files);
-
         g_signal_emit (bookmarks, signals[CONTENTS_CHANGED], 0);
     } else if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) {
         g_warning ("Could not load bookmark file: %s\n", error->message);

=== modified file 'src/marlin-places-sidebar.c'
--- src/marlin-places-sidebar.c	2013-08-01 22:11:56 +0000
+++ src/marlin-places-sidebar.c	2013-08-10 16:49:01 +0000
@@ -154,7 +154,7 @@
     MarlinZoomLevel     zoom;
     gboolean show_eject, show_unmount;
     gboolean show_eject_button;
-
+    
     g_object_get (sidebar, "zoom-level", &zoom, NULL);
 
     GtkIconSize stock_size = marlin_zoom_level_to_stock_icon_size (zoom);
@@ -190,12 +190,12 @@
     GError* error = NULL;
     gchar* converted_name = g_locale_to_utf8 (name, -1, NULL, NULL, &error);
     if (error != NULL) {
-        g_warning ("Couldn't convert the bookmark name: %s With error: %s", name, error->message);
+        g_warning ("%s - Couldn't convert the bookmark name: %s With error: %s", G_STRFUNC, name, error->message);
         converted_name = name;
     }
 
     gtk_tree_store_append (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store, &iter, parent);
-    gtk_tree_store_set (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store, &iter,
+    gtk_tree_store_set (MARLIN_ABSTRACT_SIDEBAR (sidebar)->store, &iter,
                         PLACES_SIDEBAR_COLUMN_ICON, pixbuf,
                         PLACES_SIDEBAR_COLUMN_NAME, converted_name,
                         PLACES_SIDEBAR_COLUMN_URI, uri,
@@ -206,13 +206,12 @@
                         PLACES_SIDEBAR_COLUMN_INDEX, index,
                         PLACES_SIDEBAR_COLUMN_EJECT, show_eject_button,
                         PLACES_SIDEBAR_COLUMN_NO_EJECT, !show_eject_button,
-                        PLACES_SIDEBAR_COLUMN_BOOKMARK, place_type != PLACES_BOOKMARK,
+                        PLACES_SIDEBAR_COLUMN_BOOKMARK, place_type == PLACES_BOOKMARK,
                         PLACES_SIDEBAR_COLUMN_TOOLTIP, tooltip,
                         PLACES_SIDEBAR_COLUMN_EJECT_ICON, eject,
                         PLACES_SIDEBAR_COLUMN_FREE_SPACE, 0,
                         PLACES_SIDEBAR_COLUMN_DISK_SIZE, 0,
                         -1, -1);
-    
     return iter;
 }
 
@@ -321,11 +320,12 @@
     bookmark_count = marlin_bookmark_list_length (sidebar->bookmarks);
     for (index = 0; index < bookmark_count; index++) {
         bookmark = marlin_bookmark_list_item_at (sidebar->bookmarks, index);
-
         if (marlin_bookmark_uri_known_not_to_exist (bookmark)) {
+            /* do not show bookmarks that are (currently) unavailable
+             * note: these remain in the bookmark list
+             */
             continue;
         }
-
         name = marlin_bookmark_get_name (bookmark);
         icon = marlin_bookmark_get_icon (bookmark);
         mount_uri = marlin_bookmark_get_uri (bookmark);
@@ -338,7 +338,7 @@
         compare_for_selection (sidebar,
                                location, mount_uri, last_uri,
                                &last_iter, &select_path);
-        //printf ("bookmark: %d %s\n", index, mount_uri);
+        g_debug ("%s - bookmark: %d %s", G_STRFUNC, index, mount_uri);
         g_free (name);
         g_object_unref (root);
         g_object_unref (icon);
@@ -914,13 +914,13 @@
     /*gtk_tree_model_get_iter_first (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), &iter);
       num_rows = gtk_tree_model_iter_n_children (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), &iter);*/
     //printf ("num rows %d\n", num_rows);
-
+    
     if (!gtk_tree_view_get_dest_row_at_pos (tree_view,
                                             x,
                                             y,
                                             path,
                                             pos)) {
-        //printf ("dest_row_at_pos UNKNOWN\n");
+        g_warning ("%s - dest_row_at_pos UNKNOWN\n", G_STRFUNC);
         //row = num_rows - 1;
         //		*path = gtk_tree_path_new_from_indices (row, -1);
         /*gtk_tree_view_get_path_at_pos(tree_view,  x, y, path, NULL, NULL, NULL);
@@ -929,11 +929,11 @@
         *pos = -1;
         return;
     }
-    //printf ("TEST path %s\n", gtk_tree_path_to_string (*path));
+    g_debug ("%s - TEST path %s", G_STRFUNC, gtk_tree_path_to_string (*path));
     row = *gtk_tree_path_get_indices (*path);
     /*gint *idxs = gtk_tree_path_get_indices (*path);
       row = idxs[1];*/
-    //printf ("row indice %d\n", row);
+    g_debug ("%s - row indice %d", G_STRFUNC, row);
 
     gtk_tree_path_free (*path);
 
@@ -1183,7 +1183,7 @@
                      MarlinPlacesSidebar *sidebar)
 {
     //amtest drag
-    printf ("%s\n", G_STRFUNC);
+    g_debug ("%s", G_STRFUNC);
     free_drag_data (sidebar);
     gtk_tree_view_set_drag_dest_row (tree_view, NULL, GTK_TREE_VIEW_DROP_BEFORE);
     g_signal_stop_emission_by_name (tree_view, "drag-leave");
@@ -1207,7 +1207,7 @@
 
     if (position < 0)
         position = 0;
-    printf ("%s\n", G_STRFUNC);
+   g_debug ("%s - position is %i", G_STRFUNC, position);
 
     for (i = 0; uris[i]; i++) {
         uri = uris[i];
@@ -1241,7 +1241,7 @@
         return FALSE;
     }
     //amtest
-    //printf ("TEST %s: %s\n", G_STRFUNC, gtk_tree_model_get_string_from_iter (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), iter));
+    g_debug ("TEST %s: %s", G_STRFUNC, gtk_tree_model_get_string_from_iter (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR (sidebar)->store), iter));
     return TRUE;
 }
 
@@ -1253,6 +1253,7 @@
     PlaceType type; 
     int old_position;
     GtkTreeSelection *selection;
+    gboolean is_bookmark;
 
     /* Get the selected path */
     if (!get_selected_iter (sidebar, &iter)) {
@@ -1260,14 +1261,14 @@
         //g_assert_not_reached ();
     }
 
-    gtk_tree_model_get (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), &iter,
-                        PLACES_SIDEBAR_COLUMN_ROW_TYPE, &type,
+    gtk_tree_model_get (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR (sidebar)->store), &iter,
+                        PLACES_SIDEBAR_COLUMN_BOOKMARK, &is_bookmark,
                         PLACES_SIDEBAR_COLUMN_INDEX, &old_position,
                         -1);
 
     //printf("%s: old_pos: %d new_pos: %d\n", G_STRFUNC, old_position, new_position);
     old_position = old_position -sidebar->n_builtins_before;
-    if (type != PLACES_BOOKMARK ||
+    if (!is_bookmark ||
         old_position < 0 ||
         old_position >= marlin_bookmark_list_length (sidebar->bookmarks)) {
         return;
@@ -1299,7 +1300,7 @@
     int position;
     GtkTreeModel *model;
     char *drop_uri;
-    PlaceType type; 
+    gint type; 
     gboolean success;
 
     tree_view = GTK_TREE_VIEW (widget);
@@ -1339,7 +1340,7 @@
                             PLACES_SIDEBAR_COLUMN_INDEX, &position,
                             -1);
 
-        if (!(type == PLACES_BOOKMARK || type == PLACES_BUILT_IN)) {
+        if (type >= 0 && !(type == PLACES_BOOKMARK || type == PLACES_BUILT_IN)) {
             goto out;
         }
 
@@ -1541,6 +1542,7 @@
     gboolean show_empty_trash;
     gboolean show_connect_server;
     char *uri = NULL;
+    gboolean is_bookmark;
 
     type = PLACES_BUILT_IN;
 
@@ -1555,19 +1557,20 @@
                             PLACES_SIDEBAR_COLUMN_VOLUME, &volume,
                             PLACES_SIDEBAR_COLUMN_MOUNT, &mount,
                             PLACES_SIDEBAR_COLUMN_URI, &uri,
+                            PLACES_SIDEBAR_COLUMN_BOOKMARK, &is_bookmark,
                             -1);
     }
 
     gtk_widget_show (sidebar->popup_menu_open_in_new_tab_item);
 
-    /*gtk_widget_set_sensitive (sidebar->popup_menu_remove_item, (type == PLACES_BOOKMARK));
-      gtk_widget_set_sensitive (sidebar->popup_menu_rename_item, (type == PLACES_BOOKMARK));*/
-    eel_gtk_widget_set_shown (sidebar->popup_menu_remove_item, (type == PLACES_BOOKMARK));
+    //gtk_widget_set_sensitive (sidebar->popup_menu_remove_item, (type == PLACES_BOOKMARK));
+    //gtk_widget_set_sensitive (sidebar->popup_menu_rename_item, (type == PLACES_BOOKMARK));
+
+
     //TODO add the possibility to rename volume later
-    eel_gtk_widget_set_shown (sidebar->popup_menu_rename_item, (type == PLACES_BOOKMARK));
-    eel_gtk_widget_set_shown (sidebar->popup_menu_separator_item1, (type == PLACES_BOOKMARK));
-
-    gtk_widget_set_sensitive (sidebar->popup_menu_empty_trash_item, !marlin_trash_monitor_is_empty ());
+    eel_gtk_widget_set_shown (sidebar->popup_menu_remove_item, is_bookmark);
+    eel_gtk_widget_set_shown (sidebar->popup_menu_rename_item, is_bookmark);
+    eel_gtk_widget_set_shown (sidebar->popup_menu_separator_item1, is_bookmark);
 
     check_visibility (mount, volume, drive,
                       &show_mount, &show_unmount, &show_eject, &show_rescan, &show_format, &show_start, &show_stop);
@@ -1589,6 +1592,7 @@
           eel_gtk_widget_set_shown (sidebar->popup_menu_start_item, show_start);
           eel_gtk_widget_set_shown (sidebar->popup_menu_stop_item, show_stop);*/
     eel_gtk_widget_set_shown (sidebar->popup_menu_empty_trash_item, show_empty_trash);
+    gtk_widget_set_sensitive (sidebar->popup_menu_empty_trash_item, !marlin_trash_monitor_is_empty ());
     eel_gtk_widget_set_shown (sidebar->popup_menu_connect_server_item, show_connect_server);
 
     //TODO check this
@@ -1832,7 +1836,7 @@
         path = gtk_tree_model_get_path (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), &iter);
         column = gtk_tree_view_get_column (GTK_TREE_VIEW (sidebar->tree_view), 0);
         renderers = gtk_cell_layout_get_cells (GTK_CELL_LAYOUT (column));
-        cell = g_list_nth_data (renderers, 4);
+        cell = g_list_nth_data (renderers, 5);
         g_list_free (renderers);
         g_object_set (cell, "editable", TRUE, NULL);
         gtk_tree_view_set_cursor_on_cell (GTK_TREE_VIEW (sidebar->tree_view),
@@ -1852,21 +1856,20 @@
 static void
 remove_selected_bookmarks (MarlinPlacesSidebar *sidebar)
 {
-    GtkTreeIter iter;
-    PlaceType type; 
-    int index;
+    GtkTreeIter iter; 
+    gint index;
+    gboolean is_bookmark;
 
     if (!get_selected_iter (sidebar, &iter)) {
         return;
     }
 
     gtk_tree_model_get (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), &iter,
-                        PLACES_SIDEBAR_COLUMN_ROW_TYPE, &type,
+                        PLACES_SIDEBAR_COLUMN_BOOKMARK, &is_bookmark,
                         -1);
 
-    if (type != PLACES_BOOKMARK) {
+    if (!is_bookmark)
         return;
-    }
 
     gtk_tree_model_get (GTK_TREE_MODEL (MARLIN_ABSTRACT_SIDEBAR(sidebar)->store), &iter,
                         PLACES_SIDEBAR_COLUMN_INDEX, &index,
@@ -2427,8 +2430,7 @@
     g_signal_connect (item, "activate",
                       G_CALLBACK (rename_shortcut_cb), sidebar);
     gtk_widget_show (item);
-    // Omit broken "Rename..." from the sidebar
-    // gtk_menu_shell_append (GTK_MENU_SHELL (sidebar->popup_menu), item);
+    gtk_menu_shell_append (GTK_MENU_SHELL (sidebar->popup_menu), item);
 
     /* Mount/Unmount/Eject menu items */
 
@@ -2464,8 +2466,6 @@
     gtk_widget_show (item);
     gtk_menu_shell_append (GTK_MENU_SHELL (sidebar->popup_menu), item);
 
-    bookmarks_check_popup_sensitivity (sidebar);
-
     /* Connect to server menu item */
     item = gtk_menu_item_new_with_mnemonic (_("Connect to Server…"));
     sidebar->popup_menu_connect_server_item = item;