GNOME Bugzilla – Bug 683316
Drag and Drop of attachment adds random characters
Last modified: 2018-05-22 14:41:38 UTC
Created attachment 223402 [details] screenshot of issue Dragging an attachment of a PDF to nautilus adds random characters at the end of the filename. ii libpoppler27:amd64 0.20.3-2ubuntu1 amd64 PDF rendering library ii evince 3.5.90-0ubuntu2 amd64 Document (PostScript, PDF) viewer
In case somebody is searching for a file with an attachment. I did above screenshot with http://www.dante.de/index/Intern/Mitglied/AntragStandard.pdf. However, this applies to any PDF with attachment.
still valid.
It seems the extra characters are added after creating a temporary file, which is missing a clean up once the process is done. For instance, 'Antrag_DANTE_pdftex.tex.SZZJLW' instead of 'Antrag_DANTE_pdftex.tex', which seems to match the template for mktemp().
Indeed. The code is in the callback ev_sidebar_attachments_drag_data_get in http://git.gnome.org/browse/evince/tree/shell/ev-sidebar-attachments.c#n426 A solution might be a similar behaviour of wget. If the file exists, add a counter until there are no chance to overwrite an existent file.
i cant compile evince. Requested 'gtk+-3.0 >= 3.7.5' but version of GTK+ is 3.4.2. 3.7.5 is not available
Atul, you need to build the libraries that Evince depends on. I use https://github.com/gpoo/jhbuild for doing so in Ubuntu 12.04. Other people use something different, depending of their preferences. Other instructions can be found in https://live.gnome.org/Evince/GettingEvince
Created attachment 242884 [details] [review] Patch to create temporary files for drag inside a unique tmp directory, rather than modifying the name of the dragged file to make it unique. The problem as it appears to me, is that in the temporary file that is created, there are some random characters added at the ended. During the drag, a list of uris of these files is sent to the drag receiving application. When dragging to nautilus, nautilus 'copies' or 'moves' these files to the dragged location, depending on the GDK drag action specified in the code. Now, when nautilus places them in the destination, it keeps the filename same as in the source. I have addressed this bug by creating these temporary files inside a directory with a random unique name (made using ev_mkdtemp()). This directory will be inside the evince tmp directory. When we pass the list of uris of these files to the drag receiving application, the application will get the correct file name (without the extra characters). So, when nautilus copies or moves it to a destination, the filename will be correct. I have attached a patch for this -> bug683316.patch. I have tested it on my system, and can confirm that it works. Please try it out and comment.
Created attachment 242901 [details] [review] Updated version of the patch proposed to fix this bug There is a slight problem with the first patch, I missed an '&'. This is the updated patch.
Review of attachment 242901 [details] [review]: I don't like this patch too much... it creates yet another tmp directory (and forgets to clean it up), and it's just a workaround. The real fix would be to make this code use XDS [http://freedesktop.org/wiki/Specifications/XDS] which nautilus supports and was invented for exactly this use case.
Created attachment 243143 [details] [review] Patch to add support for DnD using XDS [work-in-progress] Based on chpe's recommendation, I am working on adding XDS support for drag and drop of sidebar attachments. This patch allows destination applications to use either XDS, or the current uri-list. For XDS supporting applications, the file will be saved correctly, without the random characters. For the rest, it is assumed that they will only be opening the files, not saving them in a new location. So, for them, the attachment is saved in a tmp file (with random characters appended to the name) and a uri-list with uri to the tmp file is passed. However, a few things still remain - 1. Need to handle the case where the destination may already have a file with the same name. 2. Show a progress bar while making saves to non-native/remote location. 3. Presently, while receiving drops, nautilus prefers to use uri-list over XDS. So, it may appear that this patch is not working as expected. I have tried to contact the nautilus developers over IRC, to ask if there is any particular reason for this, and am waiting for a reply. In the meantime, to verify that XDS support is correctly working, one can comment out " {"text/uri-list", 0, EV_DND_TARGET_TEXT_URI_LIST}, " in ev_sidebar_attachments_init (around line numb 653). This work, hence, is currently in progress.
Created attachment 243835 [details] [review] Patch to add support for DnD using XDS Hi, I have finished working on this patch. A confirmation dialog for overwriting now appears if there is already a file with the same name. The ev-window progress bar is also used if the save is to a non-remote location. The issue about nautilus giving priority to uri-list (bug 607427) still remains. I have submitted a patch for it. In the meantime, the working of this XDS patch for evince can be verified by commenting out "{"text/uri-list", 0, EV_DND_TARGET_TEXT_URI_LIST}," at around line 667 and changing "targets, 2," to "targets, 1," at around line 720 in ev-sidebar-attachments.c. It would be great if someone could review this patch and test and report whether it works for them.
Created attachment 243845 [details] [review] Patch to add support for DnD using XDS Updated the patch. Had forgotten to free a thing or two.
Review of attachment 243835 [details] [review]: Thanks for the patch! Looks nice, so had just a few minor complatings. Since the ev_file_overwrite_dialog function is only used in one place, I don't think we should make this public API, but simply add it as a static function to ev-sidebar-attachments.c. ::: libdocument/ev-file-helpers.c @@ +764,3 @@ + gtk_widget_destroy (dialog); + g_object_unref (dialog); + This ref/unref of the dialog is rather strange; what was the purpose? I think you can just remove that. It's rather unfortunate having to use gtk_dialog_run here, but I guess this unavoidable due to how DND works? ::: shell/ev-sidebar-attachments.c @@ +65,3 @@ static guint signals[N_SIGNALS]; +#define XDS_ATOM gdk_atom_intern ("XdndDirectSave0", FALSE) gdk_atom_intern_static_string(). Same below. @@ +407,3 @@ + g_return_val_if_fail (context != NULL, NULL); + + if (gdk_property_get (gdk_drag_context_get_source_window(context), XDS_ATOM, TEXT_ATOM, Code style: space before ( @@ +424,3 @@ +{ + if (value) { + gdk_property_change (gdk_drag_context_get_source_window(context), XDS_ATOM, space before ( @@ +427,3 @@ + TEXT_ATOM, 8, GDK_PROP_MODE_REPLACE, + value, strlen ((const gchar *)value)); + } else on same line as } and use { ... } around the else block too. @@ +445,3 @@ + + EvAttachment *attachment; + GtkTreePath *path; You need to declare all variables at the top of the function, before any code. (-Wdeclaration-after-statement) @@ +525,1 @@ } else on same line @@ +534,3 @@ + if(!uri_recd) { + g_warning ("Remote application wants to use Direct Save, but I can't %s", + "read the XdndDirectSave0 (type text/plain) property.\n"); Do we need this warning? The message is a bit weird too ("I")... @@ +554,3 @@ + + if (!success) { + g_warning ("%s", error->message); No need to warn here either. @@ +646,3 @@ + signals[SIGNAL_SAVE_ATTACHMENT] = + g_signal_new ("direct_save_attachment", Use - instead of _ in signal names (canonical name). @@ +719,3 @@ GTK_ICON_VIEW (ev_attachbar->priv->icon_view), GDK_BUTTON1_MASK, + targets, 2, Use G_N_ELEMENTS (targets) instead of hardcoding the current number of elements (2).
Created attachment 244082 [details] [review] Patch to add support for DnD using XDS Hi all, This is the updated patch, based on Christian's comments above.
Review of attachment 244082 [details] [review]: Thanks for the patch, it looks good, I have just some comments. ::: shell/ev-sidebar-attachments.c @@ +400,3 @@ + +static gchar * +read_xds_property (GdkDragContext *context, gboolean delete) Is this used with delete = TRUE? otherwise I would just remove the parameter. @@ +405,3 @@ + gint length; + gchar *retval = NULL; + g_return_val_if_fail (context != NULL, NULL); Do not use g_return macros in private functions, use g_assert instead or nothing. @@ +421,3 @@ + +static void +write_xds_property (GdkDragContext *context, const guchar *value) I prefer if this receives a const gchar * and you only cast the value to guchar when passed to gdk_property_change. @@ +446,3 @@ + + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), + _("A file with the same name already exists. This action will overwrite it. Continue?")); I would use the same messages and buttons than the GTK+ dialog for consistency. See the implementation here: https://git.gnome.org/browse/gtk+/tree/gtk/gtkfilechooserdefault.c#n5449 @@ +524,3 @@ + gtk_tree_model_get (GTK_TREE_MODEL (ev_attachbar->priv->model), &iter, + COLUMN_ATTACHMENT, &attachment, + -1); Instead of duplicating this code here and in drag_begin, move it to a helper function that returns the selected attachment or NULL, something like: EvAttachment *ev_sidebar_attachments_get_selected_attachment (EvSidebarAttachments *ev_attachbar); @@ +531,3 @@ + char *template; + + uris = g_ptr_array_new (); We are only handling one attachment now, so we can use a stack allocated array instead of a GPtrArray, since we know it always be of size 2. @@ +559,2 @@ + guchar to_send = 'E'; + gchar *uri_recd; what does recd stand for? received? Please don't use abbreviations, I think in this case since the variable is limited to the else block you can simply use uri. @@ +571,3 @@ + filename = g_filename_from_uri (uri_recd, NULL, NULL); + if ( !g_file_test (filename, G_FILE_TEST_EXISTS) || + ev_file_overwrite_dialog (NULL, uri_recd)) { You should pass the parent window instead of NULL. You can get the toplevel window of the sidebar widget. This could be simplified if the overwrite dialog function does everything, something like: gboolean ev_sidebar_attachments_confirm_overwrite (EvSidebarAttachments *ev_attachbar, const gchar *uri); Checking if the file exists and returning early if it doesn't exist. @@ +587,3 @@ + g_free (filename); + g_free (uri_recd); + gtk_selection_data_set (data, STRING_ATOM, 8, &to_send, 1); I think it would be clearer to use the success variable instead of to_send, something like: gtk_selection_data_set (data, STRING_ATOM, 8, success ? 'S' : 'E', 1); Maybe adding also macros for 'S' and 'E' making it more obvious what they are. #define XDS_ERROR 'E' #define XDS_SUCCESS 'S' Assuming S -> success and E -> error are right. @@ +668,3 @@ + signals[SIGNAL_SAVE_ATTACHMENT] = + g_signal_new ("direct-save-attachment", Use the same name for the signal than the vmthod, save-attachment. @@ +675,3 @@ + g_cclosure_marshal_generic, + G_TYPE_BOOLEAN, 2, + G_TYPE_POINTER, Why not G_TYPE_OBJECT? instead of pointer? @@ +689,3 @@ + GtkTargetEntry targets[] = { {"text/uri-list", 0, EV_DND_TARGET_TEXT_URI_LIST}, + {"XdndDirectSave0", 0, EV_DND_TARGET_XDS}}; Can this be static const? ::: shell/ev-sidebar-attachments.h @@ +55,3 @@ + void (*save_attachment) (EvSidebarAttachments *ev_attachbar, + EvAttachment *attachment, + const char *direct_save_uri); I would name this just uri or destination_uri ::: shell/ev-window.c @@ +5219,3 @@ + + g_object_unref (dest_file); + } All this code is duplicated in attachment_save_dialog_response_cb. Move the common code to a helper function.
Created attachment 244589 [details] [review] Patch to add support for DnD using XDS Hi Carlos, thanks for the really detailed review! Hi all, I have updated the patch based on the review given by Carlos. To answer his queries - >::: shell/ev-sidebar-attachments.c >@@ +400,3 @@ >+ >+static gchar * >+read_xds_property (GdkDragContext *context, gboolean delete) > >Is this used with delete = TRUE? otherwise I would just remove the parameter. No, you're right, it's not used with delete = TRUE. So, I have removed the parameter. >@@ +405,3 @@ >+ gint length; >+ gchar *retval = NULL; >+ g_return_val_if_fail (context != NULL, NULL); > >Do not use g_return macros in private functions, use g_assert instead or >nothing. Fixed it, used g_assert >@@ +421,3 @@ >+ >+static void >+write_xds_property (GdkDragContext *context, const guchar *value) > >I prefer if this receives a const gchar * and you only cast the value to guchar >when passed to gdk_property_change. Yes, that looks better. Fixed it. >@@ +446,3 @@ >+ >+ gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), >+ _("A file with the same name >already exists. This action will overwrite it. Continue?")); > >I would use the same messages and buttons than the GTK+ dialog for consistency. >See the implementation here: > >https://git.gnome.org/browse/gtk+/tree/gtk/gtkfilechooserdefault.c#n5449 Thanks for the link! Jose too had suggested the same thing. So, I have used that in the updated patch and it looks good now. >@@ +524,3 @@ >+ gtk_tree_model_get (GTK_TREE_MODEL (ev_attachbar->priv->model), &iter, >+ COLUMN_ATTACHMENT, &attachment, >+ -1); > >Instead of duplicating this code here and in drag_begin, move it to a helper >function that returns the selected attachment or NULL, something like: > >EvAttachment *ev_sidebar_attachments_get_selected_attachment >(EvSidebarAttachments *ev_attachbar); Made a new function as suggested. >@@ +531,3 @@ >+ char *template; >+ >+ uris = g_ptr_array_new (); > >We are only handling one attachment now, so we can use a stack allocated array >instead of a GPtrArray, since we know it always be of size 2. Yes, thanks for the suggestion! Fixed that now. >@@ +559,2 @@ >+ guchar to_send = 'E'; >+ gchar *uri_recd; > >what does recd stand for? received? Please don't use abbreviations, I think in >this case since the variable is limited to the else block you can simply use >uri. Sorry about that, I have used simply uri now. >@@ +571,3 @@ >+ filename = g_filename_from_uri (uri_recd, NULL, NULL); >+ if ( !g_file_test (filename, G_FILE_TEST_EXISTS) || >+ ev_file_overwrite_dialog (NULL, uri_recd)) { > >You should pass the parent window instead of NULL. You can get the toplevel >window of the sidebar widget. This could be simplified if the overwrite dialog >function does everything, something like: > >gboolean ev_sidebar_attachments_confirm_overwrite (EvSidebarAttachments >*ev_attachbar, const gchar *uri); > >Checking if the file exists and returning early if it doesn't exist. Yes, that looks cleaner. I have now implemented the above function as suggested. >@@ +587,3 @@ >+ g_free (filename); >+ g_free (uri_recd); >+ gtk_selection_data_set (data, STRING_ATOM, 8, &to_send, 1); > >I think it would be clearer to use the success variable instead of to_send, >something like: > >gtk_selection_data_set (data, STRING_ATOM, 8, success ? 'S' : 'E', 1); > >Maybe adding also macros for 'S' and 'E' making it more obvious what they are. > >#define XDS_ERROR 'E' >#define XDS_SUCCESS 'S' > >Assuming S -> success and E -> error are right. I have now used the macros for 'S' and 'E' so that its clear what they are. Yes, to_send doesn't look very clean. I am still using it because we need to pass a pointer as the 4th parameter to gtk_selection_data_set (). to_send comes handy then. >@@ +668,3 @@ > >+ signals[SIGNAL_SAVE_ATTACHMENT] = >+ g_signal_new ("direct-save-attachment", > >Use the same name for the signal than the vmthod, save-attachment. Used "save-attachment" now. >@@ +675,3 @@ >+ g_cclosure_marshal_generic, >+ G_TYPE_BOOLEAN, 2, >+ G_TYPE_POINTER, > >Why not G_TYPE_OBJECT? instead of pointer? Fixed that. >@@ +689,3 @@ > >+ GtkTargetEntry targets[] = { {"text/uri-list", 0, >EV_DND_TARGET_TEXT_URI_LIST}, >+ {"XdndDirectSave0", 0, EV_DND_TARGET_XDS}}; > >Can this be static const? Yes, I think it can. I have changed it to static const in the new patch. >::: shell/ev-sidebar-attachments.h >@@ +55,3 @@ >+ void (*save_attachment) (EvSidebarAttachments *ev_attachbar, >+ EvAttachment *attachment, >+ const char *direct_save_uri); > >I would name this just uri or destination_uri Renamed direct_save_uri to uri >::: shell/ev-window.c >@@ +5219,3 @@ >+ >+ g_object_unref (dest_file); >+ } > >All this code is duplicated in attachment_save_dialog_response_cb. Move the >common code to a helper function. Implemented a helper function save_attachment_to_target_file () and moved the common code to it. Thanks once again for the reviews! The patch does look much cleaner now. Also, please do tell me if there is anything that I have left out.
Review of attachment 244589 [details] [review]: Yeah, it looks much better, thank you. I still have a few nits though. ::: shell/ev-sidebar-attachments.c @@ +407,3 @@ + gint length; + gchar *retval = NULL; + g_assert (context != NULL); Leave an empty line between variable declaration block and the function body. @@ +416,3 @@ + /* g_strndup will null terminate the string */ + retval = g_strndup ((const gchar *) prop_text, length); + g_free(prop_text); Add space between function name and parentheses g_free ( @@ +440,3 @@ + */ +static GtkWindow * +ev_get_toplevel (GtkWidget *widget) Even though this is in gtk, I think we can just get the toplevel and then use a inline if. @@ +506,3 @@ + G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, + NULL, + NULL); hmm, this is synchronous, I think we can avoid this, even if the message is not exactly the same, see below. @@ +517,3 @@ + toplevel = ev_get_toplevel (GTK_WIDGET (attachbar)); + + dialog = gtk_message_dialog_new (toplevel, This could be easily replaced by: toplevel = gtk_widget_get_toplevel (GTK_WIDGET (attachbar)); dialog = gtk_message_dialog_new (gtk_widget_is_toplevel (toplevel) ? GTK_WINDOW (toplevel) : NULL, @@ +522,3 @@ + GTK_BUTTONS_NONE, + _("A file named \"%s\" already exists. Do you want to " + "replace it?"), We could use the attachment basename in this case @@ +526,3 @@ + gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog), + _("The file already exists in \"%s\". Replacing" + " it will overwrite its contents."), And the full attachment uri here, so we don't need the folder name. The file \"%s\" already exists. Replacing ... @@ +593,3 @@ + gchar *filename; + + attachment = ev_sidebar_attachments_get_selected_attachment (ev_attachbar); Note that this can return NULL, you should check it here and return early. @@ +617,1 @@ + attachment = ev_sidebar_attachments_get_selected_attachment (ev_attachbar); Ditto. @@ +619,3 @@ + if (info != EV_DND_TARGET_XDS) { + char *uri_list[2]; + char *template; Remove the extra spaces here: char *uri_list[2]; char *template; @@ +629,1 @@ Remove this extra lines. @@ +630,2 @@ + uri_list[0] = g_file_get_uri (file); + uri_list[1] = NULL; /* NULL-terminate */ I think the comment is pretty obvious in this case, I would just remove it. @@ +639,3 @@ + gtk_selection_data_set_uris (data, uri_list); + g_free(uri_list[0]); g_free ( @@ +640,3 @@ + gtk_selection_data_set_uris (data, uri_list); + g_free(uri_list[0]); + } else { Extra line here too. @@ +644,2 @@ + guchar to_send = XDS_ERROR; + gchar *uri; Extra spaces here too. @@ +652,3 @@ + + if (ev_sidebar_attachments_confirm_overwrite (ev_attachbar, uri)) { + Extra line here. @@ +659,3 @@ + 0, + attachment, + (const char *) uri, Why do you need this cast? ::: shell/ev-window.c @@ +5187,3 @@ +{ + GFile *save_to = NULL; + GError *error = NULL; More extra spaces here. @@ +6849,3 @@ attachment = (EvAttachment *) l->data; + save_attachment_to_target_file(attachment, Add space between function name and parentheses.
OH... I just saw that this bug is also valid if you drag an image! possibly we need to refactor this patch so we can taclke both bugs at the same time. Aakash? could you take a look and say how easy/dificult is to adapt this patch to also handle the image case?
Created attachment 360560 [details] [review] shell: Fix DnD support for attachments Follows XDS if available. Otherwise, uses temporary files as fallback. Fixes
I rebased the patch, fixes the conflicts, and addresses all Carlos' comments. In doing so, I noticed a major regression of this patch: when using DnD, it only saves only one attachment regardless of how many files are selected in the sidebar. You can see the removal of the loop that iterates through every file. Other than that, any review would be appreciated.
Review of attachment 360560 [details] [review]: Thanks! ::: shell/ev-window.c @@ +5187,3 @@ + is_native = g_file_is_native (target_file); + + success = save_attachment_to_target_file (attachment, extra space here after the =
Created attachment 360718 [details] [review] shell: Fix DnD support for attachments I made a patch a little more readable, set the variables close to where they are used, and dealt with a magic number. Even though the patch is accepted to be committed, I am not quite happy with the patch because on multiple selection, only drops the last attachment selected instead of the full list.
(In reply to José Aliste from comment #18) > OH... I just saw that this bug is also valid if you drag an image! possibly > we need to refactor this patch so we can taclke both bugs at the same time. > Aakash? could you take a look and say how easy/dificult is to adapt this > patch to also handle the image case? I think dragging an image is a different thing. 1. When there are documents attached, there is metadata (e.g. file name and description). The images in a PDF do not have such metadata. 2. The documents attached without the patch, save the documents as name.ext.XXXXXX where as the images are saves as image-XXXXXX.png What name should be provided for the images? At the most, image-1.png, image-2.png, and so on. However, that pattern might work if the user select and save all the images together. I have not been able to so, I can only save one image at a time, therefore, the schema of consecutive numbers do not apply either. So, the only approach is to minimize the odds of repeating a file name by setting a random image there. I maybe me missing something. José, what do you think?
(In reply to Germán Poo-Caamaño from comment #22) > Created attachment 360718 [details] [review] [review] > shell: Fix DnD support for attachments > > Even though the patch is accepted to be committed, I am not quite > happy with the patch because on multiple selection, only drops > the last attachment selected instead of the full list. Hi Germán, that is because the Direct Save Protocol (XDS)[1] only supports passing a *single* filename. I stumbled on this problem also when I implemented XDS for Gedit, see last paragraph of my comment here: https://bugzilla.gnome.org/show_bug.cgi?id=710546#c4 I think the potential usefulness of XDS, which might be big, it's very reduced by this limitation. Now that Wayland is still getting shape it may be a good moment to propose an extension to the Wayland specific implementation/version of XDS to support passing a list of files. [1] https://www.freedesktop.org/wiki/Specifications/XDS/
Comment on attachment 360718 [details] [review] shell: Fix DnD support for attachments Thank you Nelson for the your comments. Do you feel like exploring a draft for XDS? (I could help :-) I applied the patch in master, but I am inclined to leave the bug opened.
(In reply to Germán Poo-Caamaño from comment #25) > Comment on attachment 360718 [details] [review] [review] > shell: Fix DnD support for attachments > > Thank you Nelson for the your comments. > > Do you feel like exploring a draft for XDS? (I could help :-) > > I applied the patch in master, but I am inclined to leave the bug opened. I don't have too much time right now to lead that effort so I encourage you to do it :-) and please CC me as I could help with testing and/or some code.
-- 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/evince/issues/296.