GNOME Bugzilla – Bug 430798
can't drop a document from file-roller to gedit - XDS support
Last modified: 2013-10-28 19:10:28 UTC
The bug has been opened on https://bugs.launchpad.net/ubuntu/+source/gedit/+bug/107016 "Binary package hint: gedit GEdit don't understand the new drag-n-drop protocol from file-roller. ..."
Looks like a problem with file-roller to me... I cannot drag from file roller even to nautilus. I am reassigning.
that's because file-roller uses XDS for dnd now, nautilus changes have been commited to make that work but it still doesn't work with gedit, reassigning back there
Created attachment 141835 [details] [review] xds This patch is unfinished, for some reason file-roller returns as name of the xds.txt so if anybody finds the reason for this and fix it, would be great.
The problem with file-roller is that's not following completely the spec.. because it was only interested in dropping to nautilus it just supplied a fake name just to retrieve the dir where to extract.. but now with the awesome patch from Ignacio we need to fix that last bit in file-roller..
Ok I finally got my butt off and worked on this, and have ready patches.. yay!. As told, this needed support in file-roller, the patches for that are in bug 710546 , and I'm about to attach here the updated gedit patch, which I based on the Ignacio's one and improve upon it by adding support to drop on the GeditView as well and refactoring some code, I also changed the directory destination to be a random named directory inside tmp_dir, so a same filename can be dropped multiple times on gedit without problems. Also to show the patch behaviour, I made a nice screencast where I drag & drop some files between a patched file-roller and gedit, it's in bug 710546 comment 4
Created attachment 257763 [details] [review] Implement XDS dnd support for GeditView and GeditWindow dnd: Implement XDS dnd support for GeditView and GeditWindow So we can open files directly when dropped from file-roller.
Review of attachment 257763 [details] [review]: The patch looks really good I just annotated a couple of style comments and questions. I guess it is better to wait for the File Roller patch to get applied before getting this in. ::: gedit/gedit-utils.c @@ +1457,3 @@ } + do not leave two empty lines @@ +1507,3 @@ + else + { + } can you explain a bit more the permissions here? ::: gedit/gedit-view.c @@ -195,2 +202,3 @@ current_buffer_removed (view); + g_free (view->priv->direct_save_uri); why do we need to wait till finalization to free the string? @@ -415,2 +424,3 @@ gtk_drag_finish (context, TRUE, TRUE, timestamp); } + else if (info == TARGET_XDNDDIRECTSAVE) I guess we should turn this into switch/case
Created attachment 258169 [details] [review] Implement XDS dnd support for GeditView and GeditWindow v2 (In reply to comment #7) > Review of attachment 257763 [details] [review]: > > The patch looks really good I just annotated a couple of style comments and > questions. > > I guess it is better to wait for the File Roller patch to get applied before > getting this in. Thanks Paolo for the quick response and review, yes we have to wait for the file-roller patch, which I still haven't received any feedback.. and sorry for getting back to you after a week but I've been busy lately. > ::: gedit/gedit-utils.c > @@ +1457,3 @@ > } > > + > > do not leave two empty lines Done. > @@ +1507,3 @@ > + else > + { > + } > > can you explain a bit more the permissions here? g_dir_make_tmp() creates a directory with 600 permission, which means only owner can read or write, which is good default security wise if that temp directory is going to be used only by the same application that create it. But in our case we create the directory to be used by other application (file-roller) so with 600 permission file-roller won't be able to place a file inside it when in this two cases: - gedit is running as root, file-roller as another user - gedit is running as normal user, file-roller as another different user These may be rare cases, but possible in a linux system, if you don't care about them I can remove the chmod 777, which by the way it only permits everybody to list the directory. > ::: gedit/gedit-view.c > @@ -195,2 +202,3 @@ > current_buffer_removed (view); > > + g_free (view->priv->direct_save_uri); > > why do we need to wait till finalization to free the string? Yes it was not mandatory, I've changed it to free it after we are done loading the uris in drag_data_received(), anyway I left the extra g_free() before the assigning to it, in drag_drop(), cause it doesn't hurt and could avoid a leak in some rare case (like closing the geditview while file-roller is asking for password of an encrypted file). > @@ -415,2 +424,3 @@ > gtk_drag_finish (context, TRUE, TRUE, timestamp); > } > + else if (info == TARGET_XDNDDIRECTSAVE) > > I guess we should turn this into switch/case Done.
Thanks for the update and do not worry about the delay, this has been sitting here long enough that a few days do not make a difference :-) (In reply to comment #8) > g_dir_make_tmp() creates a directory with 600 permission, which means only > owner can read or write, which is good default security wise if that temp > directory is going to be used only by the same application that create it. > > But in our case we create the directory to be used by other application > (file-roller) so with 600 permission file-roller won't be able to place a file > inside it when in this two cases: > - gedit is running as root, file-roller as another user > - gedit is running as normal user, file-roller as another different user > > These may be rare cases, but possible in a linux system, if you don't care > about them I can remove the chmod 777, which by the way it only permits > everybody to list the directory. > Mmm, I am not sure about this and I am open to be convinced one way or the other. Are the files themselves are only readable to the same user? As a gut feeling I'd say let's start by just allowing the same user to drag and drop.
(In reply to comment #9) [SNIP] > Mmm, I am not sure about this and I am open to be convinced one way or the > other. > > Are the files themselves are only readable to the same user? The files extracted by file-roller are owned by the same user file-roller is running as, and with the permissions specified by the file inside the archive, so they could have different permissions, as an example see these recent drops I made: nelson@cartagena:~$ ls -l /tmp/gedit-drop-UCBM5W/ -rwxrwxr-x. 1 nelson nelson 178207 Oct 27 2003 configure nelson@cartagena:~$ ls -l /tmp/gedit-drop-YUQN5W/ -rw-r--r--. 1 nelson nelson 412922 Sep 2 18:48 brasero-session.log the 'configure' file has execute permissions as it carried those inside the .tar.gz , while the brasero-session.log is a regular text file with the typical 644 permissions. > As a gut feeling I'd say let's start by just allowing the same user to drag and > drop. I just wanted to be more opened to support more usecases, take in mind that the file themselves will still carry the same perms set by file-roller, and by being conservative we will hurt this mentioned use case: - gedit is running as root, file-roller as another user which thinking about it, is not so rare, someone may want to launch gedit as root to edit some file in /etc (eg. /etc/fstab) and then continue using it to later found it fails to open a file dropped from file-roller. Altough I prefer this way, I don't have a strong opinion either, so if you prefer it I'll remove the chmod.
(In reply to comment #10) > Altough I prefer this way, I don't have a strong opinion either, so if you > prefer it I'll remove the chmod. I may a bit paranoid, but basically I do not like the idea of root dragging a file out of his archive and any other user being able to take a peek I'd say lets leave 600 for a start and see if people complain
Created attachment 258182 [details] [review] Implement XDS dnd support for GeditView and GeditWindow (In reply to comment #11) > (In reply to comment #10) > > Altough I prefer this way, I don't have a strong opinion either, so if you > > prefer it I'll remove the chmod. > > I may a bit paranoid, but basically I do not like the idea of root dragging a > file out of his archive and any other user being able to take a peek > > > I'd say lets leave 600 for a start and see if people complain Fair enough, updated patch attached.
Review of attachment 258182 [details] [review]: Thanks a lot, patch looks good to me, feel free to commit (though it may make more sense to first wait for file-roller)
(In reply to comment #13) > Review of attachment 258182 [details] [review]: > > Thanks a lot, patch looks good to me, feel free to commit (though it may make > more sense to first wait for file-roller) Pushed as commit c1c3b568ec6294f49d1ce7e90ae64109c4dc3a93 Thanks!
Hey Nelson, it seems this patch breaks the build: ../../gedit/gedit-view.c: In function 'gedit_view_drag_data_received': ../../gedit/gedit-view.c:383:4: error: a label can only be part of a statement and a declaration is not a statement ../../gedit/gedit-view.c:398:4: error: a label can only be part of a statement and a declaration is not a statement ../../gedit/gedit-view.c:399:4: error: expected expression before 'GtkWidget' ../../gedit/gedit-view.c:414:4: error: 'new_notebook' undeclared (first use in this function) ../../gedit/gedit-view.c:414:4: note: each undeclared identifier is reported only once for each function it appears in ../../gedit/gedit-view.c:429:4: error: a label can only be part of a statement and a declaration is not a statement
Should be fixed by https://git.gnome.org/browse/gedit/commit/?id=d5a21439b6c3903979c8848c03dc89b8a2741354
Sorry for the inconvenience, I forgot to test compiled the patch after changing the switch syntax, thanks Colin for the quick fix!