GNOME Bugzilla – Bug 762252
Don't show the "New Bookmark" row when the dragged item is not a folder
Last modified: 2018-05-02 16:57:28 UTC
We don't bookmark files, but folders.
Created attachment 321738 [details] [review] gtkplacessidebar: add signal for enabling new bookmark When a drag is being performed over the sidebar, the "New bookmark" row becomes visible without checking if the dragged items can be bookmarked. This leads to bookmark actions that do nothing. In order to fix this, the client application should be asked if the dragged items can be bookmarked. Add signal for checking if dragged items can be bookmarked. Emit the signal only for distinct GdkDragContext instances, in order to reuse results.
Created attachment 321739 [details] [review] window: add signal handler for enabling new bookmark In Nautilus, the "New bookmark" row is revealed on a drag and drop start. This is done without checking if the dragged items can be bookmarked. In order to fix this, connect to the sidebar signal that asks if items can be bookmarked. Add signal handler for the "can-bookmark" sidebar signal and connect to it.
(In reply to Razvan Chitu from comment #1) > Created attachment 321738 [details] [review] [review] > gtkplacessidebar: add signal for enabling new bookmark > > When a drag is being performed over the sidebar, the "New bookmark" row > becomes > visible without checking if the dragged items can be bookmarked. This leads > to > bookmark actions that do nothing. In order to fix this, the client > application > should be asked if the dragged items can be bookmarked. > > Add signal for checking if dragged items can be bookmarked. Emit the signal > only > for distinct GdkDragContext instances, in order to reuse results. I will add a description for the signal to this patch if you consider it a good solution.
So actually, the drag context is passed to the sidebar in the gtk_places_sidebar_set_drop_targets_visible. I added that on 3.16 to being able to do also this. So you can check for the dragged files being files in the sidebar itself, using just the dnd functions and context.
Created attachment 322682 [details] [review] gtkplacessidebar: check that "New Bookmark" is a valid drop target When a drag is being performed over the sidebar, the "New bookmark" row becomes visible without checking if the dragged items can be bookmarked. This leads to bookmark actions that do nothing. In order to fix this, the client application should be asked if the dragged items can be bookmarked. Reveal the "New Bookmark" row only if it is a valid drop target.
Created attachment 322683 [details] [review] window: check if dragged files can be bookmarked In Nautilus, the "New bookmark" row is revealed on a drag and drop start. This is done without checking if the dragged items can be bookmarked. In order to fix this, check if selected files can be bookmarked. Add function for checking if files can be bookmarked.
Review of attachment 322683 [details] [review]: Looks good to me aside of next nitpick, thanks! ::: src/nautilus-window.c @@ +1128,3 @@ + + if (dest_file == NULL) { + action = can_bookmark_items (items) ? GDK_ACTION_PRIVATE : 0; Use GDK enums, you can use GDK_ACTION_DEFAULT instead of 0.
Review of attachment 322683 [details] [review]: As discussed in IRC, 0 is fine since they are flags. Wait to push until we have the ack from gtk+ devs on the other patch.
Review of attachment 322682 [details] [review]: This one looks good to me, but let's wait for gtk+ devs to review.
Review of attachment 322682 [details] [review]: looks good to me too
But do we need a file chooser patch to handle ::drag-action-requested ? I don't see a handler for that currently
(In reply to Matthias Clasen from comment #11) > But do we need a file chooser patch to handle ::drag-action-requested ? I > don't see a handler for that currently Indeed, I forgot about this.
Created attachment 323016 [details] [review] filechooserwidget: add handler for places sidebar signal The places sidebar has a special row responsible for creating new bookmarks. This row was previously revealed without checking if the dragged items can be bookmarked, which lead to invalid bookmark attempts. The sidebar now requests an action for the row, which can enable or disable it. In order to properly show the row, a handler for this signal should be added. Add flags for checking if a bookmark action is possible. Add signal handler for the "drag-action-requested" sidebar signal.
(In reply to Razvan Chitu from comment #13) > Created attachment 323016 [details] [review] [review] > filechooserwidget: add handler for places sidebar signal > > The places sidebar has a special row responsible for creating new bookmarks. > This row was previously revealed without checking if the dragged items can be > bookmarked, which lead to invalid bookmark attempts. The sidebar now > requests an > action for the row, which can enable or disable it. In order to properly show > the row, a handler for this signal should be added. > > Add flags for checking if a bookmark action is possible. Add signal handler > for > the "drag-action-requested" sidebar signal. An issue that I found while working on this is that the file-chooser allows dragging of insensitive items. Dragging an insensitive item does not select it and it does not clear the previous selection. Is this intended, or is it another issue that needs to be addressed?
Review of attachment 323016 [details] [review]: As discussed on IRC, the code looks a little tricky, but I don't have a better solution that matches what the filechooser expect with insensitive files and allowing to select drag files without unselecting the previous selected files. I wonder if some o those two behaviours are actually wanted or not. In any case, this patch LGTM with a few nitpicks (and Mathias will do the review that actually counts): ::: gtk/gtkfilechooserwidget.c @@ +7803,3 @@ + can_select = is_sensitive && is_folder; + } + else All branches use braces when one does. @@ +7807,3 @@ } + impl->priv->last_selection_valid = can_select; probably this looks better with a line jump
Fixed the above issues, waiting for Mathias' review.
Review of attachment 323016 [details] [review]: I don't particularly like this approach of using extra state in the file chooser widget for this. However, the more serious issue I have with reviewing this patch is that dragging files from the list is currently almost impossible due to the slow-double-click implementation not being careful enough. That needs to be fixed first before we can pick this up. ::: gtk/gtkfilechooserwidget.c @@ +363,3 @@ + /* Drag and drop selection flags */ + gboolean last_selection_valid; + gboolean selection_all_folders; There already a bitfield right above; I would prefer to add these two booleans to it.
(In reply to Matthias Clasen from comment #17) > Review of attachment 323016 [details] [review] [review]: > > I don't particularly like this approach of using extra state in the file > chooser widget for this. However, the more serious issue I have with > reviewing this patch is that dragging files from the list is currently > almost impossible due to the slow-double-click implementation not being > careful enough. That needs to be fixed first before we can pick this up. > > ::: gtk/gtkfilechooserwidget.c > @@ +363,3 @@ > + /* Drag and drop selection flags */ > + gboolean last_selection_valid; > + gboolean selection_all_folders; > > There already a bitfield right above; I would prefer to add these two > booleans to it. Talking about the issue in https://bugzilla.gnome.org/show_bug.cgi?id=763036 ?
yes, bug 763036
a not directly related problem with the 'new bookmark' location: In particular with the file chooser, the window may be small enough that the location where we insert this item is scrolled off, making for a somewhat confusing experience. We should probably scroll the new row into view or at least make the sidebar scroll if you hover the ends during dnd.
It seems attachment 321739 [details] [review] , attachment 323016 [details] [review] and attachment 322682 [details] [review] were never committed. I've tried a simpler, different approach to solve this bug, just checking the items being dragged whether they are bookmarkatable. I'm attaching patches for GtkPlacesSidebar, and also for Filechooser and Nautilus so they don't hint placessidebar to show "new bookmark" when starting dragging if the files they are dragging are not folders.
Created attachment 357556 [details] [review] gtkplacessidebar: check dragged items for "New Bookmark" visibility Don't show the "New Bookmark" row when the items being dragged cannot be bookmarked (like e.g. regular files). https://bugzilla.gnome.org/show_bug.cgi?id=762252
Created attachment 357557 [details] [review] gtkfilechooser: only show "new bookmark" when dragging folders Check that all files being dragged are folders before hinting GtkPlacesSidebar to show "new bookmark", as regular files cannot be bookmarked. https://bugzilla.gnome.org/show_bug.cgi?id=762252
Created attachment 357558 [details] [review] dnd: make sidebar show "new bookmark" only for folders Make sure sidebar shows "new bookmark" only when folders are being dragged. https://bugzilla.gnome.org/show_bug.cgi?id=754513
Currently nautilus shows "new bookmark" when I drag two or more folders, but it's not possible bookmark all of them at the same time. Which will the behavior in this situation be after apply these patches?
(In reply to Strangiato from comment #25) > Currently nautilus shows "new bookmark" when I drag two or more folders, but > it's not possible bookmark all of them at the same time. > Which will the behavior in this situation be after apply these patches? I think you're wrong, that behaviour is already in place, if you drop over "New Bookmark" several folders, all of them will be bookmarked. And this is not affected in any way by my patch.
*** Bug 721321 has been marked as a duplicate of this bug. ***
*** Bug 782845 has been marked as a duplicate of this bug. ***
*** This bug has been marked as a duplicate of bug 754513 ***
Hey Nelson, what's the status of this? Just waiting for review?
(In reply to Carlos Soriano from comment #30) > Hey Nelson, what's the status of this? Just waiting for review? Yes, I have two patches waiting for review here, one for GtkPlacesSidebar and the other for GtkFileChooser. The Nautilus one was committed in: https://gitlab.gnome.org/GNOME/nautilus/commit/f0c81e22a3e9c3cbc1a70245e629caa2209c75ba The Razvan Chitu patches are still attached, they should be marked as obsolete if the simpler approach on my patches is preferred.
Review of attachment 357556 [details] [review]: Looks nice, thanks a lot Nelson. Some nitpicks: ::: gtk/gtkplacessidebar.c @@ +138,3 @@ /* DND */ GList *drag_list; /* list of GFile */ + /* used for g_atomic funcs when traversing drag_list asyncly */ asyncly = asynchronously @@ +1521,3 @@ + +static void +can_be_bookmarked__file_query_info (GObject *object, spurious extra _ @@ +1525,3 @@ + gpointer user_data) +{ + GFile *file = (GFile *) object; use macros, so there is type safety G_FILE (object). @@ +1526,3 @@ +{ + GFile *file = (GFile *) object; + GtkPlacesSidebar *sidebar = (GtkPlacesSidebar *) user_data; ditto @@ +1527,3 @@ + GFile *file = (GFile *) object; + GtkPlacesSidebar *sidebar = (GtkPlacesSidebar *) user_data; + GFileInfo *file_info = NULL; use g_autoptr @@ +1539,3 @@ + + if (_gtk_file_info_consider_as_directory (file_info) && + g_atomic_int_dec_and_test (&sidebar->drag_list_length)) why is this need to be atomic? The thread is the main UI, should be fine. @@ +1554,3 @@ + sidebar->drag_list_length = g_list_length (sidebar->drag_list); + for (l = sidebar->drag_list; l != NULL; l = l->next) + { spurious space before { @@ +1555,3 @@ + for (l = sidebar->drag_list; l != NULL; l = l->next) + { + GFile *file = (GFile *) l->data; use G_FILE @@ +1561,3 @@ + G_PRIORITY_DEFAULT, + NULL, + can_be_bookmarked__file_query_info, you can use the most common callback name pattern here, which is on_(method hint)_(signal name). Here it could be on_reveal_new_bookmark_file_info or something along those lines @@ +1563,3 @@ + can_be_bookmarked__file_query_info, + sidebar); + } identation is off
Review of attachment 357557 [details] [review]: ::: gtk/gtkfilechooserwidget.c @@ +2068,3 @@ +static void +only_folders_foreach__set_boolean (GtkTreeModel *model, the name is quite mouthful (even if I'm usually in that side), I would name it more like "foreach_only_folders" or something like that. Also remember we don't use in all of our function name patterns the double underscore. @@ +2095,3 @@ + seen_only_folders = TRUE; + + gtk_tree_selection_selected_foreach (selection, wouldn't this be easier with a regular for loop? Also you would avoid the whole list if the first item was a folder already... the foreach function code is meh.
Faulty review, I'm afraid. g_autoptr cannot be used in GTK+
Created attachment 363597 [details] [review] gtkplacessidebar: check dragged items for "New Bookmark" visibility Don't show the "New Bookmark" row when the items being dragged cannot be bookmarked (like e.g. regular files). ---- Updated for review comments.
Created attachment 363598 [details] [review] gtkfilechooser: only show "new bookmark" when dragging folders Check that all files being dragged are folders before hinting GtkPlacesSidebar to show "new bookmark", as regular files cannot be bookmarked. ----- Updated for review comments.
what is the status of this bug?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/598.