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 754513 - Dragging files to the sidebar makes the "+new bookmark" row to show up
Dragging files to the sidebar makes the "+new bookmark" row to show up
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: Sidebar
3.26.x
Other Linux
: Normal normal
: future
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-03 11:43 UTC by Lapo Calamandrei
Modified: 2017-09-18 19:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
dnd: make sidebar show "new bookmark" only for folders (5.65 KB, patch)
2017-08-14 13:49 UTC, Nelson Benitez
none Details | Review
Updated patch (4.36 KB, patch)
2017-08-17 14:04 UTC, Nelson Benitez
committed Details | Review
bug is still happening on Arch, nautilus 3.26 (113.63 KB, image/png)
2017-09-18 15:49 UTC, Strangiato
  Details
Dragging a file in Nautilus 3.26 (74.56 KB, image/png)
2017-09-18 19:12 UTC, Piotr Drąg
  Details
Dragging a file over the sidebar in Nautilus 3.26 (75.59 KB, image/png)
2017-09-18 19:12 UTC, Piotr Drąg
  Details

Description Lapo Calamandrei 2015-09-03 11:43:08 UTC
It should appear only when draggin dirs.
Comment 1 rishabh 2015-12-29 17:27:56 UTC
(In reply to Lapo Calamandrei from comment #0)
> It should appear only when draggin dirs.

Hi, I was trying to implement this feature but found that 'New bookmark' row is added by function gtk_places_sidebar_set_drop_targets_visible(). Correct me if I am wrong.

https://developer.gnome.org/gtk3/stable/GtkPlacesSidebar.html#gtk-places-sidebar-set-drop-targets-visible
Comment 2 Carlos Soriano 2016-01-04 11:30:37 UTC
(In reply to rishabh from comment #1)
> (In reply to Lapo Calamandrei from comment #0)
> > It should appear only when draggin dirs.
> 
> Hi, I was trying to implement this feature but found that 'New bookmark' row
> is added by function gtk_places_sidebar_set_drop_targets_visible(). Correct
> me if I am wrong.
> 
> https://developer.gnome.org/gtk3/stable/GtkPlacesSidebar.html#gtk-places-
> sidebar-set-drop-targets-visible

Yes, however you have the drag context as a parameter, so you can ask the application using that function if the source object is a file or folder.
Comment 3 Lapo Calamandrei 2016-03-15 14:15:17 UTC
any news on this one?
Comment 4 Carlos Soriano 2016-09-20 08:15:09 UTC
For newcomers with some experience already.
Comment 5 Akilan Elango 2016-09-26 00:43:12 UTC
Hello,
I was trying out the
Comment 6 Akilan Elango 2016-09-26 00:59:42 UTC
Hello,
I was trying that out, and it seems that it be done easily on the reciever end.(gtk places side bar)(In reply to Carlos Soriano from comment #2)
> (In reply to rishabh from comment #1)
> > (In reply to Lapo Calamandrei from comment #0)
> > > It should appear only when draggin dirs.
> > 
> > Hi, I was trying to implement this feature but found that 'New bookmark' row
> > is added by function gtk_places_sidebar_set_drop_targets_visible(). Correct
> > me if I am wrong.
> > 
> > https://developer.gnome.org/gtk3/stable/GtkPlacesSidebar.html#gtk-places-
> > sidebar-set-drop-targets-visible
> 
> Yes, however you have the drag context as a parameter, so you can ask the
> application using that function if the source object is a file or folder.

I was looking around. Which functions is available for getting the selection list from a GdkDragContext?
Comment 7 Carlos Soriano 2016-10-04 15:28:25 UTC
(In reply to Akilan Elango from comment #6)
> Hello,
> I was trying that out, and it seems that it be done easily on the reciever
> end.(gtk places side bar)(In reply to Carlos Soriano from comment #2)
> > (In reply to rishabh from comment #1)
> > > (In reply to Lapo Calamandrei from comment #0)
> > > > It should appear only when draggin dirs.
> > > 
> > > Hi, I was trying to implement this feature but found that 'New bookmark' row
> > > is added by function gtk_places_sidebar_set_drop_targets_visible(). Correct
> > > me if I am wrong.
> > > 
> > > https://developer.gnome.org/gtk3/stable/GtkPlacesSidebar.html#gtk-places-
> > > sidebar-set-drop-targets-visible
> > 
> > Yes, however you have the drag context as a parameter, so you can ask the
> > application using that function if the source object is a file or folder.
> 
> I was looking around. Which functions is available for getting the selection
> list from a GdkDragContext?

In this case I believe you should just avoid to call gtk_places_sidebar_set_drop_targets_visible (TRUE) from the nautilus side, since we can know if it's a file or a folder just when starting the dnd.

In case the mouse is hovering the sidebar itself, it will go through drag_motion_callback in the gtkplacessidebar, and try to get the drag data with gtk_drag_get_data which will return a list of GFiles where you can see if one of them is a file instead than a directory and therefore not show the new bookmark row.

Sadly, I was mistaken and this is not for newcomers, since it requires changes in gtkplacessidebar too.
Comment 8 Vyas Giridhar 2017-03-02 17:29:55 UTC
diff --git a/gtk/gtkdnd.c b/gtk/gtkdnd.c
index 608dfaa30d..4601064ca2 100644
--- a/gtk/gtkdnd.c
+++ b/gtk/gtkdnd.c
@@ -635,6 +635,18 @@ gtk_drag_get_data (GtkWidget      *widget,
   g_return_if_fail (GTK_IS_WIDGET (widget));
   g_return_if_fail (GDK_IS_DRAG_CONTEXT (context));
 
+  char *fileuri;
+  GFile *f;
+  for (GList *B = gdk_drag_context_list_targets (context); B != NULL; B = l->next)
+  {
+    f = G_FILE(B->data)
+    file_uri = g_file_get_uri(f)
+    if (g_file_test(f,G_FILE_TEST_IS_REGULAR))
+    {
+      return;
+    }
+  }
+
   selection_widget = gtk_drag_get_ipc_widget (widget);
 
   g_object_ref (context);

Will this work in the gtk part?
Comment 9 Vyas Giridhar 2017-03-02 17:37:37 UTC
Or should I use gdk_drag_get_selection?
Comment 10 Carlos Soriano 2017-04-21 12:36:06 UTC
(In reply to Vyas Giridhar from comment #9)
> Or should I use gdk_drag_get_selection?

I think so, yeah. I need to double check, but we should use the dnd API when possible.
Comment 11 Nelson Benitez 2017-08-14 13:49:54 UTC
Created attachment 357559 [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
Comment 12 Nelson Benitez 2017-08-14 13:58:05 UTC
(In reply to Nelson Benitez from comment #11)
> Created attachment 357559 [details] [review] [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

This patch fixes the issue by just checking the items being dragged if they are all folders.

This patch fixes the issue in Nautilus, but would be nice to also have the fixes for GtkPlacesSidebar and Filechooser that I posted on bug 762252 (which included this nautilus patch too).
Comment 13 Carlos Soriano 2017-08-15 19:14:47 UTC
Review of attachment 357559 [details] [review]:

Good progress, some review:

::: src/nautilus-dnd.c
@@ +701,3 @@
+ * are folders, FALSE otherwise */
+gboolean
+nautilus_drag_items_are_all_folders (const GList *list)

not sure a separated function to know if all drag items are folders is necesary. I can imagine having a general "all files are folders" function inside nautilus-file-utilities, then sending to that your list.

I prefer to do a O(2n) method, one that converts to a list of files then one that checks if all of those are folders, rather than this.

We should avoid adding this half-random API, if the cost of it technical wise is not that much.

@@ +707,3 @@
+  {
+      NautilusDragSelectionItem *item = l->data;
+{

remove extra space after !

::: src/nautilus-window.c
@@ +1184,3 @@
 nautilus_window_start_dnd (NautilusWindow *window,
+                           GdkDragContext *context,
+                           gboolean        only_folders)

if the code is just doing nothing if one of the paremeters is TRUE, it doesn't make sense to have it in the first place.

@@ +1191,3 @@
 
+    if (only_folders)
+    {   /* Items being dragged are only folders, so allow places sidebar show "New bookmark" */

I think the comment is unnecesary, since only_folders is already a boolean inside an if, and the API of gtkplacessidebar is precisely "show drop targets visible", so the code already explains that comment.

It doesn't say new bookmark only in the API in pourpose, so it's good to keep it aligned with that.
Comment 14 Nelson Benitez 2017-08-17 14:04:28 UTC
Created attachment 357813 [details] [review]
Updated patch

Updated patch with review comments, except for:

> I can imagine having a general "all files are folders" function inside nautilus-file-utilities"

I made this "all files are folders" function be in nautilus-file.c instead of nautilus-file-utilities.c. As the function iterates over a list of NautilusFile I thought was more natural place.
Comment 15 Carlos Soriano 2017-08-17 14:27:11 UTC
Review of attachment 357813 [details] [review]:

Excellent, thanks Nelson!!

::: src/nautilus-file.c
@@ +4211,3 @@
+  const GList *l;
+
+  for (l = files; l != NULL; l = l->next)

I pushed with a follow up with a nitpick fix, the style of nautilus is 4 spaces.
Comment 17 Nelson Benitez 2017-08-17 14:58:46 UTC
(In reply to Carlos Soriano from comment #16)
> git bz failed. The commits are:
> https://git.gnome.org/browse/nautilus/commit/
> ?id=f0c81e22a3e9c3cbc1a70245e629caa2209c75ba
> https://git.gnome.org/browse/nautilus/commit/
> ?id=1f1da4e495908cf1adb6f7f06659a3c59e851334

Thank you!
Comment 18 Strangiato 2017-09-04 19:32:41 UTC
not fixed in nautilus 3.25.92 on ubuntu 17.10.
Comment 19 Strangiato 2017-09-08 14:02:57 UTC
*** Bug 762252 has been marked as a duplicate of this bug. ***
Comment 20 Strangiato 2017-09-18 15:36:10 UTC
Reopening because it's not fixed on Arch.
Comment 21 Carlos Soriano 2017-09-18 15:38:31 UTC
(In reply to Strangiato from comment #20)
> Reopening because it's not fixed on Arch.

Can you share a screenshot of it?
Comment 22 Strangiato 2017-09-18 15:49:47 UTC
Created attachment 359993 [details]
bug is still happening on Arch, nautilus 3.26

Sure.
Comment 23 Piotr Drąg 2017-09-18 18:06:36 UTC
On nautilus-3.26.0-1.fc27.x86_64, “New Bookmark” appears when you start dragging a folder, but also when you drag a file *over* the sidebar.
Comment 24 Carlos Soriano 2017-09-18 18:59:51 UTC
can you confirm it does not appear when not in the sidebar?
Comment 25 Hussam Al-Tayeb 2017-09-18 19:06:29 UTC
It shows a placesholder (New Bookmark) but nothing happens when I drag it all the way.
Comment 26 Piotr Drąg 2017-09-18 19:12:11 UTC
Created attachment 360011 [details]
Dragging a file in Nautilus 3.26
Comment 27 Piotr Drąg 2017-09-18 19:12:34 UTC
Created attachment 360012 [details]
Dragging a file over the sidebar in Nautilus 3.26
Comment 28 Nelson Benitez 2017-09-18 19:20:12 UTC
(In reply to Strangiato from comment #22)
> Created attachment 359993 [details]
> bug is still happening on Arch, nautilus 3.26
> 
> Sure.

Please re-read my comment 12, the patch committed in this bug it's for Nautilus only, the fix for gtkplacessidebar (a gtk widget nautilus uses as his sidebar) is on bug 762252 along with a patch for filechooser also, those patches are still waiting in their bugs.

Please also remove bug 762252 being a dup of this, instead you could mark it as blocking this, they share the same problem but on different components and with separate patches.