GNOME Bugzilla – Bug 689351
[regression] Drag'n'drop doesn't work anymore
Last modified: 2014-11-11 14:17:53 UTC
We use to support dnd'ing pictures on the icon views, we should do that again.
If we go for easter eggs, we can also allow dropping colors into the color tab.
Created attachment 230619 [details] [review] implement dnd
Review of attachment 230619 [details] [review]: The old code copied the URIs to the local disk before setting it (which meant dnd'ing from a web browser worked). Probably just need to revive the old code. Good idea about the colors though.
We should make sure it works exactly like the code used to add wallpapers in Nautilus.
*** Bug 707001 has been marked as a duplicate of this bug. ***
(In reply to comment #4) > We should make sure it works exactly like the code used to add wallpapers in > Nautilus. I have a similar issue: I want to define additional plain colors in gnome3/Fedora20 for usage as desktop background, but I do not find a way to do that (only 15 colors available). No GUI for that :-(
Created attachment 274280 [details] [review] background: Re-add drag'n'drop support Inside the chooser dialogue itself.
Created attachment 274281 [details] [review] background: Allow dropping colours as well
Created attachment 274282 [details] [review] background: Remember added colours
Created attachment 274283 [details] [review] background: Copy all the manually added backgrounds Except the ones already in the Pictures directory.
Still need to add support for dnd in the main panel, rather than in the chooser dialog alone.
Review of attachment 274280 [details] [review]: ::: panels/background/cc-background-chooser-dialog.c @@ +279,3 @@ + store = bg_source_get_liststore (BG_SOURCE (chooser->priv->pictures_source)); + g_signal_connect (G_OBJECT (store), "row-inserted", + G_CALLBACK (row_inserted), chooser); Can we use a different name for the callback instead of 'row_inserted'? There is a priv->row_inserted_id which is used to track a callback that listens for the same signal on the same object, but for a different purpose. It can get confusing. Can we tag the callback in some way so that we know that our callback was invoked as a result of a drag-n-dropped image being added and not something else? See the other signal connection mentioned above, which tracks if the model is empty or not. We don't want this to be invoked if someone just dropped a few file into a directory or something similarly unrelated. @@ +281,3 @@ + G_CALLBACK (row_inserted), chooser); + + if (bg_pictures_source_add (chooser->priv->pictures_source, uri) == FALSE) I wonder how useful the return value of bg_pictures_source_add is. Currently it is atleast better than nothing, but if you consider that it does a synchronous g_file_query_info call, then things become a messy if you want to enforce 'async-correctness'. Maybe we could just treat it as a fire and forget operation in future?
Review of attachment 274281 [details] [review]: ::: panels/background/cc-background-chooser-dialog.c @@ +302,3 @@ + /* And select the newly added item */ + gtk_icon_view_select_path (GTK_ICON_VIEW (chooser->priv->icon_view), path); +} color_row_inserted is identical to row_inserted. Maybe we could somehow parametrize this? What about a function that does the main work and it takes the button, source and callback as arguments? @@ +355,3 @@ + if (bg_colors_source_add (chooser->priv->colors_source, &rgba) == FALSE) + { + g_signal_handlers_disconnect_by_func (G_OBJECT (store), G_CALLBACK (row_inserted), chooser); Typo alert: it should be color_row_inserted, not row_inserted.
Created attachment 281062 [details] [review] background: Prevent monitor_pictures_model() running multiple times
Created attachment 281063 [details] [review] background: Re-add drag'n'drop support Inside the chooser dialogue itself.
Created attachment 281064 [details] [review] background: Allow dropping colours as well
Created attachment 281065 [details] [review] background: Remember added colours
Created attachment 281066 [details] [review] background: Copy all the manually added backgrounds Except the ones already in the Pictures directory.
Comment on attachment 281066 [details] [review] background: Copy all the manually added backgrounds That's broken...
Created attachment 281079 [details] [review] background: Copy all the manually added backgrounds Except the ones already in the Pictures directory.
Created attachment 281080 [details] [review] background: Split up creation of loading icon
Created attachment 281081 [details] [review] background: Make placeholder the same size as thumbnails Otherwise they look tiny and compacted.
Created attachment 281087 [details] [review] background: Copy all the manually added backgrounds Except the ones already in the Pictures directory.
Created attachment 281106 [details] [review] background: Copy all the manually added backgrounds Except the ones already in the Pictures directory
Created attachment 281107 [details] [review] background: Create a placeholder for manually added items When adding things that might be screenshots, always create a placeholder if the image was added manually.
Attachment 281062 [details] pushed as e1fab88 - background: Prevent monitor_pictures_model() running multiple times Attachment 281063 [details] pushed as ccf7dc7 - background: Re-add drag'n'drop support Attachment 281064 [details] pushed as 8115ba4 - background: Allow dropping colours as well Attachment 281065 [details] pushed as ebbda7a - background: Remember added colours Attachment 281080 [details] pushed as c3fc128 - background: Split up creation of loading icon Attachment 281081 [details] pushed as cf26e6c - background: Make placeholder the same size as thumbnails Attachment 281106 [details] pushed as 41be6ee - background: Copy all the manually added backgrounds Attachment 281107 [details] pushed as 65d4ba1 - background: Create a placeholder for manually added items
Review of attachment 281065 [details] [review]: ::: panels/background/bg-colors-source.c @@ +209,3 @@ + colors = g_key_file_get_string_list (keyfile, "Colors", "custom-colors", &len, NULL); + + if (len == 0 && colors != NULL) Is this needed? Won't colors be NULL if len is 0 or are there are some cases in which g_key_file_get_string_list can cause this? @@ +223,3 @@ + else + { + colors[len] = c; Due to this colors will not be a NULL terminated string array anymore, which will lead to a crash in the g_strfreev later in the file.
Created attachment 289983 [details] [review] background: Fix crash when adding the second color
Created attachment 289984 [details] [review] background: Fix crash when adding the second color Fix an off-by-one error in the previous version of the patch.
Review of attachment 289984 [details] [review]: Looks fine
Comment on attachment 289984 [details] [review] background: Fix crash when adding the second color Thanks for the review!