GNOME Bugzilla – Bug 761660
location-entry: Encode URI before copy/paste
Last modified: 2016-02-09 15:05:03 UTC
The location entry is currently the only place in our UI where it is possible to copy a decoded URI. This is basically never what the user wants. I am getting tired of accidentally pasting broken URIs like https://build.webkit.org/results/GTK Linux 64-bit Release (Tests)/r196216 (13627)/results.html. URIs should be encoded before stored in the clipboard. Now I can copy https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r196216%20(13627)/results.html instead as nature intended. Of course, if only part of the entry is selected, then it's not a full URI and should not be messed with; in this case, just copy it raw.
Created attachment 320565 [details] [review] location-entry: Encode URI before copy/paste
Review of attachment 320565 [details] [review]: ::: lib/widgets/ephy-location-entry.c @@ +212,3 @@ + gint end; + + if (gtk_editable_get_selection_bounds (GTK_EDITABLE (entry), &start, &end)) I would return early. @@ +218,3 @@ + text = gtk_editable_get_chars (GTK_EDITABLE (entry), start, end); + + if (start == 0) So, this is only chking if the selection started at 0, right? but it can stop in the middle of the text, and you still have a partial selections. I think the simpler way here is comparing the elected text with gtk_entry_get_text(). @@ +238,3 @@ +{ + ephy_location_entry_copy_clipboard (entry); + gtk_editable_delete_selection (GTK_EDITABLE (entry)); We need to check here if the entry is editable, in app mode and in case of popups, the entry si not editable, you can copy but not cut. In that case we should probably error_bell.
(In reply to Carlos Garcia Campos from comment #2) > Review of attachment 320565 [details] [review] [review]: > > ::: lib/widgets/ephy-location-entry.c > @@ +212,3 @@ > + gint end; > + > + if (gtk_editable_get_selection_bounds (GTK_EDITABLE (entry), &start, &end)) > > I would return early. Yeah, not sure why I decided not to. > @@ +218,3 @@ > + text = gtk_editable_get_chars (GTK_EDITABLE (entry), start, end); > + > + if (start == 0) > > So, this is only chking if the selection started at 0, right? but it can > stop in the middle of the text, and you still have a partial selections. I > think the simpler way here is comparing the elected text with > gtk_entry_get_text(). If you start copying from the beginning of the URI, you have a usable URI even if you don't copy all the way to the end (e.g. if you decide not to copy query parameters, or only the host portion), but if you start copying from somewhere else, it's not a usable URI, so there's no reason to encode it. For instance, with the URI https://ja.wikipedia.org/wiki/ヨハネの手紙三#.E6.A7.8B.E6.88.90, if you copy only the URI before the fragment, https://ja.wikipedia.org/wiki/ヨハネの手紙三, you'll want the encoded form https://ja.wikipedia.org/wiki/%E3%83%A8%E3%83%8F%E3%83%8D%E3%81%AE%E6%89%8B%E7%B4%99%E4%B8%89 in your clipboard, even though that's not the result of gtk_entry_get_text(). Or, in the URI https://ja.wikipedia.org/wiki/メインページ, if you copy only the text メインページ from the end of the URI, you probably want to get メインページ in your clipboard and not %E3%83%A1%E3%82%A4%E3%83%B3%E3%83%9A%E3%83%BC%E3%82%B8. > @@ +238,3 @@ > +{ > + ephy_location_entry_copy_clipboard (entry); > + gtk_editable_delete_selection (GTK_EDITABLE (entry)); > > We need to check here if the entry is editable, in app mode and in case of > popups, the entry si not editable, you can copy but not cut. In that case we > should probably error_bell. Good catch.
Created attachment 320574 [details] [review] location-entry: Encode URI before copy/paste The location entry is currently the only place in our UI where it is possible to copy a decoded URI. This is basically never what the user wants. I am getting tired of accidentally pasting broken URIs like https://build.webkit.org/results/GTK Linux 64-bit Release (Tests)/r196216 (13627)/results.html. URIs should be encoded before stored in the clipboard. Now I can copy https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r196216%20(13627)/results.html instead as nature intended. Of course, if only part of the entry is selected, then it's not a full URI and should not be messed with; in this case, just copy it raw.
Created attachment 320575 [details] [review] location-entry: Encode URI before copy/paste The location entry is currently the only place in our UI where it is possible to copy a decoded URI. This is basically never what the user wants. I am getting tired of accidentally pasting broken URIs like https://build.webkit.org/results/GTK Linux 64-bit Release (Tests)/r196216 (13627)/results.html. URIs should be encoded before stored in the clipboard. Now I can copy https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r196216%20(13627)/results.html instead as nature intended. Of course, if only part of the entry is selected, then it's not a full URI and should not be messed with; in this case, just copy it raw.
(In reply to Michael Catanzaro from comment #3) > > @@ +218,3 @@ > > + text = gtk_editable_get_chars (GTK_EDITABLE (entry), start, end); > > + > > + if (start == 0) > > > > So, this is only chking if the selection started at 0, right? but it can > > stop in the middle of the text, and you still have a partial selections. I > > think the simpler way here is comparing the elected text with > > gtk_entry_get_text(). > > If you start copying from the beginning of the URI, you have a usable URI > even if you don't copy all the way to the end (e.g. if you decide not to > copy query parameters, or only the host portion), but if you start copying > from somewhere else, it's not a usable URI, so there's no reason to encode > it. Good point, you are right.
Attachment 320575 [details] pushed as 761d2fa - location-entry: Encode URI before copy/paste
It's encoding also the ? for query parameters and the # for fragments, breaking more than it fixed
Created attachment 320678 [details] [review] Standardize on "decoded" terminology for URIs, not "unescaped" A bit more precise
Created attachment 320679 [details] [review] location-entry: Properly normalize URI for cut/copy 761d2fa %-encodes too much, including the ? in query parameters, and the # before the fragment. I wonder if I will ever again attempt to put a # in the first column of a git commit message. Can't say I've ever wanted to do that before.
Review of attachment 320679 [details] [review]: ::: lib/ephy-uri-helpers.h @@ +26,3 @@ char *ephy_remove_tracking_from_uri (const char *uri); char *ephy_uri_decode (const char *uri); +char *ephy_uri_normalize (const char *uri); Since we renamed the other as decode, why not using encode here instead of normalize?
(In reply to Carlos Garcia Campos from comment #11) > Review of attachment 320679 [details] [review] [review]: > > ::: lib/ephy-uri-helpers.h > @@ +26,3 @@ > char *ephy_remove_tracking_from_uri (const char *uri); > char *ephy_uri_decode (const char *uri); > +char *ephy_uri_normalize (const char *uri); > > Since we renamed the other as decode, why not using encode here instead of > normalize? I originally planned to do so, but normalize is more precise. I actually couldn't find any way to normalize the URI using any of the URI encode functions, they all encode too much.
Attachment 320678 [details] pushed as 0dd2a62 - Standardize on "decoded" terminology for URIs, not "unescaped" Attachment 320679 [details] pushed as 8c36f9c - location-entry: Properly normalize URI for cut/copy