GNOME Bugzilla – Bug 320521
Make "Open URL" dialog more intelligent
Last modified: 2015-11-14 16:48:01 UTC
If you enter "mirror.fluendo.com:8800" instead of "http://mirror.fluendo.com:8800", we're probably trying to open a file when we should be adding http before.
Also, for extra intelligence, you could do a cool thing that Azureus does - check the clipboard for something that resembles an URL. If it's an URL, automatically paste it, saving the user one paste in most cases. Just a minor wish, if you're already going to be adding logic there, you might as well do this.
Created attachment 79309 [details] [review] Patch to solve the initial bug report Attached you can find a patch to solve the initial bug report, not the suggestions made in the second comment. I haven't had time yet to find a solution for that, but I am going to look into that in the next few days.
Created attachment 79383 [details] [review] Complete patch I have updated the patch and the code now does what it should do according to both the inital bug report and the first comment. Perhaps the logic I used to find out if there is something useful on the clipboard can be made intelligent, but at the moment I have no idea how. Someone else?
Comment on attachment 79383 [details] [review] Complete patch >--- totem.c 2006-11-24 14:39:08.000000000 +0100 >+++ totem-fix.c 2007-01-04 16:08:49.000000000 +0100 >@@ -1695,6 +1695,16 @@ on_open_location1_activate (GtkButton *b > if (glade == NULL) > return; > >+ /* Initialize the clipboard and get its content*/ >+ GtkClipboard *clipboard = gtk_clipboard_get_for_display (gdk_display_get_default (), >+ GDK_SELECTION_CLIPBOARD); Don't use gdk_display_get_default(), but gtk_widget_get_display(). >+ gchar* clipboardcontent = gtk_clipboard_wait_for_text (clipboard); >+ >+ /* Check clipboard content for "://", if it does, show that content in the dialog */ >+ if (g_strrstr (clipboardcontent, "://") != NULL) >+ gtk_entry_set_text (GTK_ENTRY (glade_xml_get_widget (glade, "uri")), clipboardcontent); >+ g_free (clipboardcontent); You need to add a NULL check for the clipboard content. Also, variables should be defined at the beginning of the function. You could split those bits to something like totem_open_location_set_from_clipboard(). > dialog = glade_xml_get_widget (glade, "open_uri_dialog"); > entry = glade_xml_get_widget (glade, "uri"); > gentry = glade_xml_get_widget (glade, "uri_list"); >@@ -1708,6 +1718,9 @@ on_open_location1_activate (GtkButton *b > uri = gtk_entry_get_text (GTK_ENTRY (entry)); > if (uri != NULL && strcmp (uri, "") != 0) > { >+ if (g_strrstr (uri, "://") == NULL) >+ uri = g_strconcat ("http://", uri, NULL); You're leaking the original URI here. > filenames[0] = uri; > filenames[1] = NULL; > totem_action_open_files (totem, (char **) filenames);
> You're leaking the original URI here. What exactly do you mean by that? Your other comments on my patch have been fixed. Thanks a lot!
That's you're leaking memory, in this case "uri" is leaked. That should be: if (g_strrstr (uri, "://") == NULL) { char *tmp; tmp = g_strconcat ("http://", uri, NULL); g_free (uri); uri = tmp; }
Created attachment 79442 [details] [review] Complete patch with updates I think this is the final patch :) . I have fixed all the comments you made on the previous patch, so I think everything is OK now. There is one problem however: if I use the code you have given in http://bugzilla.gnome.org/show_bug.cgi?id=320521#c4 I get this warning from the compiler: totem.c: In function ‘totem_action_open_location’: totem.c:719: warning: passing argument 1 of ‘g_free’ discards qualifiers from pointer target type I tried to find out how to solve this, but I can't find anything.
And as a follow up: previously I was building this patch based on the code totem-2.16.4. Now I am doing it based on the code of totem-2.17.3. That's the most recent development version as far as I can see.
(In reply to comment #8) > And as a follow up: previously I was building this patch based on the code > totem-2.16.4. Now I am doing it based on the code of totem-2.17.3. That's the > most recent development version as far as I can see. It would be better to use the current svn trunk instead of a released version (and it would make diff'ing a lot easier too...). (In reply to comment #7) > Created an attachment (id=79442) [edit] > Complete patch with updates > > I think this is the final patch :) . I have fixed all the comments you made on > the previous patch, so I think everything is OK now. > > There is one problem however: if I use the code you have given in > http://bugzilla.gnome.org/show_bug.cgi?id=320521#c4 I get this warning from the > compiler: > > totem.c: In function ‘totem_action_open_location’: > totem.c:719: warning: passing argument 1 of ‘g_free’ discards qualifiers > from pointer target type > > I tried to find out how to solve this, but I can't find anything. That'd be because "uri" is a const char*, so can't be modified. You'll need to add a new variable instead.
Created attachment 79635 [details] [review] Even more complete patch Wow, I have learned a lot during the creation of this patch. :) Thanks for that! This patch fixes the things you mentioned in #c9 and builds against the latest svn of totem. (It took some time to find out that Ubuntu doesn't install automake1.9 byde default, but after fixing that, it was quite easy to build totem from svn.)
Created attachment 79675 [details] [review] Complete patch with NULL check While testing my patch a bit more, I found that gtk_entry_get_text gave an error when I feeded it a string with NULL as content. So, I added a NULL check.
Comment on attachment 79675 [details] [review] Complete patch with NULL check >Index: /home/harm/totem/src/totem-private.h >=================================================================== >--- /home/harm/totem/src/totem-private.h (revision 3829) >+++ /home/harm/totem/src/totem-private.h (working copy) >@@ -188,6 +188,7 @@ > #define ZOOM_OUT_OFFSET -1 > > void totem_action_open (Totem *totem); >+char * totem_open_location_set_from_clipboard (Totem *totem); Why is this public? It should be static, as it's only used in totem.c itself. > void totem_action_open_location (Totem *totem); > void totem_action_eject (Totem *totem); > void totem_action_take_screenshot (Totem *totem); >Index: /home/harm/totem/src/totem.c >=================================================================== >--- /home/harm/totem/src/totem.c (revision 3829) >+++ /home/harm/totem/src/totem.c (working copy) >@@ -682,12 +682,34 @@ > totem_action_open_dialog (totem, NULL, TRUE); > } > >+char * so should be "static char *" <snip> >- uri = gtk_entry_get_text (GTK_ENTRY (entry)); >+ uri_from_entry = gtk_entry_get_text (GTK_ENTRY (entry)); >+ uri = g_malloc (strlen (uri_from_entry)); >+ g_stpcpy (uri, uri_from_entry); those last 2 lines could be simply replaced by "uri_from_entry = g_strdup (gtk_entry_get_text (GTK_ENTRY (entry)));" Finally, after recently merging the code from bug 338020, your patch might need updating, although it should be fairly straight forward.
Created attachment 79736 [details] [review] Complete patch I updated the patch and I altered the thing you mentioned in your comments: totem_open_location_set_from_clipboard is now static and it no longer resides in totem-private.h, but in totem.c. I also changed the three lines you mentioned at the end of your comment to use g_strdup. As far as I can see my code still works and compiles after applying the patch in bug 338020 (I updaded via svn). So everything is complete I guess :) .
Comment on attachment 79736 [details] [review] Complete patch >Index: src/totem.c >=================================================================== >--- src/totem.c (revision 3832) >+++ src/totem.c (working copy) >@@ -110,6 +110,7 @@ > static void popup_timeout_remove (Totem *totem); > static gint totem_compare_recent_stream_items (GtkRecentInfo *a, GtkRecentInfo *b); > static void totem_action_add_recent_stream (Totem *totem, const char *uri); >+static char * totem_open_location_set_from_clipboard (Totem *totem); Why is this needed? It's only called once, and the function calling it is below. Just remove it. <snip> >@@ -781,11 +810,20 @@ > > if (response == GTK_RESPONSE_OK) > { >- const char *uri; >+ gchar *uri; > >- uri = gtk_entry_get_text (GTK_ENTRY (entry)); >+ uri = g_strdup (gtk_entry_get_text (GTK_ENTRY (entry))); >+ > if (uri != NULL && strcmp (uri, "") != 0) uri can't be NULL here, it's a copy of the string from the entry, which can't be NULL. > { >+ if (g_strrstr (uri, "://") == NULL) >+ { >+ char *tmp; >+ tmp = g_strconcat ("http://", uri, NULL); >+ g_free (uri); >+ uri = tmp; >+ } >+ > filenames[0] = uri; > filenames[1] = NULL; > totem_action_open_files (totem, (char **) filenames); >@@ -796,6 +834,7 @@ > mrl = totem_playlist_get_current_mrl (totem->playlist); > totem_action_set_mrl_and_play (totem, mrl); > g_free (mrl); >+ g_free (uri); And in the case where uri is != NULL, but is empty (ie. strcmp returns 0 above), you're leaking uri.
Created attachment 79871 [details] [review] Patch which includes comments from #c14 I altered the patch based on your comments and once again I learned a lot :) . I didn't change `if (uri != NULL && strcmp (uri, "") != 0)` to something without the NULL check, but instead made sure that uri is freed at all times. So that code should be fine now, I hope.
2007-01-09 Bastien Nocera <hadess@hadess.net> * src/totem.c: (totem_open_location_set_from_clipboard), (totem_action_open_location): Patch from Harm Hilvers <harm@tweakers.net> to copy the URL obtained from the clipboard into the "Open URL" dialogue, and automatically prepend "http://" when necessary (Closes: #320521)