After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710004 - URLs should be displayed as unescaped unicode
URLs should be displayed as unescaped unicode
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
3.11.x
Other Linux
: Normal enhancement
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
: 596399 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-10-12 18:40 UTC by Eddy Castillo
Modified: 2015-07-28 15:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Searching "main page" in japanese in Firefox and Epiphany. (433.10 KB, image/jpeg)
2013-10-12 18:40 UTC, Eddy Castillo
  Details
ephy-title-box: Show correctly UTF-8 URL (1.24 KB, patch)
2014-02-28 11:01 UTC, Yosef Or Boczko
none Details | Review
nautilus-floating-bar: Show correctly UTF-8 URL (961 bytes, patch)
2014-02-28 13:22 UTC, Yosef Or Boczko
none Details | Review
ephy-title-box: Show correctly UTF-8 URL (1.46 KB, patch)
2014-08-26 22:05 UTC, Yosef Or Boczko
none Details | Review
ephy-title-box: Show correctly UTF-8 URL (1.46 KB, patch)
2014-08-26 22:08 UTC, Yosef Or Boczko
none Details | Review
ephy-title-box: Show correctly UTF-8 URL (1.53 KB, patch)
2014-09-24 09:05 UTC, Yosef Or Boczko
none Details | Review
ephy-title-box: Show correctly UTF-8 URL (1.53 KB, application/octet-stream)
2014-09-24 09:06 UTC, Yosef Or Boczko
  Details
ephy-title-box: Show correctly UTF-8 URL (1.53 KB, patch)
2014-09-24 09:09 UTC, Yosef Or Boczko
reviewed Details | Review
ephy-title-box: Show correctly UTF-8 URL (1.54 KB, patch)
2014-12-10 15:16 UTC, Michael Catanzaro
none Details | Review
proof (172.12 KB, image/png)
2014-12-10 18:50 UTC, Michael Catanzaro
  Details
more proof -- it works (89.76 KB, image/png)
2014-12-10 18:50 UTC, Michael Catanzaro
  Details
Properly display UTF-8 URLs in the location entry (1.83 KB, patch)
2014-12-10 19:39 UTC, Michael Catanzaro
none Details | Review
Display unescaped URIs in the location entry completion (1.29 KB, patch)
2014-12-11 10:50 UTC, Michael Catanzaro
none Details | Review
Display unescaped URIs in the bookmark properties dialog (1.86 KB, patch)
2014-12-11 11:17 UTC, Michael Catanzaro
none Details | Review
Add ephy_tree_model_node_add_column_full (4.23 KB, patch)
2014-12-11 17:55 UTC, Michael Catanzaro
none Details | Review
Add ephy_node_view_add_column_full (4.16 KB, patch)
2014-12-11 17:55 UTC, Michael Catanzaro
none Details | Review
Display unescaped URLs in the bookmarks editor (1.77 KB, patch)
2014-12-11 17:55 UTC, Michael Catanzaro
none Details | Review
Display unescaped URIs in the history dialog (3.22 KB, patch)
2014-12-11 18:11 UTC, Michael Catanzaro
none Details | Review
EphyWebView: decode the URI before storing it in address (2.82 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
none Details | Review
EphyWebView: Decode URI before setting the loading or status message (3.44 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
Display decoded URIs in the location entry completion (1.29 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
Display decoded URIs in the bookmark properties dialog (1.86 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
Add ephy_tree_model_node_add_column_full (4.23 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
Add ephy_node_view_add_column_full (4.16 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
Display decoded URIs in the bookmarks editor (1.77 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
Display decoded URIs in the history dialog (3.22 KB, patch)
2015-07-25 21:12 UTC, Michael Catanzaro
committed Details | Review
EphyWebView: add display-address property (5.77 KB, patch)
2015-07-25 22:20 UTC, Michael Catanzaro
reviewed Details | Review
Show decoded URI in the new web application dialog (1008 bytes, patch)
2015-07-25 22:20 UTC, Michael Catanzaro
none Details | Review
EphyWindow: store a decoded address, not a percent-encoded address (2.12 KB, patch)
2015-07-25 22:20 UTC, Michael Catanzaro
none Details | Review
Fixup for 'display decoded URIs in bookmarks editor' (1009 bytes, patch)
2015-07-28 00:49 UTC, Michael Catanzaro
committed Details | Review
Fixup for 'display decoded URIs in the history dialog' (1016 bytes, patch)
2015-07-28 00:50 UTC, Michael Catanzaro
committed Details | Review
EphyWebView: add get_display_address() (4.07 KB, patch)
2015-07-28 01:00 UTC, Michael Catanzaro
none Details | Review
Show decoded URI in the new web application dialog (1008 bytes, patch)
2015-07-28 01:00 UTC, Michael Catanzaro
none Details | Review
EphyWindow: store a decoded address, not a percent-encoded address (2.12 KB, patch)
2015-07-28 01:01 UTC, Michael Catanzaro
none Details | Review
EphyWebView: add get_display_address() (3.85 KB, patch)
2015-07-28 01:06 UTC, Michael Catanzaro
committed Details | Review
Show decoded URI in the new web application dialog (1008 bytes, patch)
2015-07-28 01:06 UTC, Michael Catanzaro
committed Details | Review
EphyWindow: store a decoded address, not a percent-encoded address (2.12 KB, patch)
2015-07-28 01:06 UTC, Michael Catanzaro
committed Details | Review

Description Eddy Castillo 2013-10-12 18:40:13 UTC
Created attachment 257115 [details]
Searching "main page" in japanese in Firefox and Epiphany.

Every time a link has a character different than english is shown as hexadecimal code in epiphany inside the page and the address bar, which makes them unreadable to humans. And sometimes (when page title is different of link) the only way to search a page is by those characters, making it impossible to find them.
Comment 1 Yosef Or Boczko 2014-02-28 11:01:20 UTC
Created attachment 270550 [details] [review]
ephy-title-box: Show correctly UTF-8 URL

This fix only the url in the subtitle when it shown.

If I do the same in the location entry, when the user
edit an UTF-8 url it get a non-valid url.
Comment 2 Yosef Or Boczko 2014-02-28 13:22:00 UTC
Created attachment 270560 [details] [review]
nautilus-floating-bar: Show correctly UTF-8 URL
Comment 3 Yosef Or Boczko 2014-04-13 11:50:20 UTC

*** This bug has been marked as a duplicate of bug 596399 ***
Comment 4 Yosef Or Boczko 2014-08-26 22:05:38 UTC
Created attachment 284557 [details] [review]
ephy-title-box: Show correctly UTF-8 URL

Rebase for master.
Comment 5 Yosef Or Boczko 2014-08-26 22:08:02 UTC
Created attachment 284561 [details] [review]
ephy-title-box: Show correctly UTF-8 URL
Comment 6 Yosef Or Boczko 2014-09-24 09:05:27 UTC
Created attachment 286957 [details] [review]
ephy-title-box: Show correctly UTF-8 URL
Comment 7 Yosef Or Boczko 2014-09-24 09:06:02 UTC
Created attachment 286958 [details]
ephy-title-box: Show correctly UTF-8 URL
Comment 8 Yosef Or Boczko 2014-09-24 09:09:12 UTC
Created attachment 286959 [details] [review]
ephy-title-box: Show correctly UTF-8 URL
Comment 9 Michael Catanzaro 2014-09-24 13:13:17 UTC
Review of attachment 286959 [details] [review]:

::: src/ephy-title-box.c
@@ +683,3 @@
   EphyTitleBoxPrivate *priv;
+  EphyEmbedShellMode   mode;
+  gchar               *decode_uri;

How about "decoded_uri" instead of "decode_uri" (decode is a verb!).
Comment 10 Michael Catanzaro 2014-12-10 15:16:28 UTC
Created attachment 292448 [details] [review]
ephy-title-box: Show correctly UTF-8 URL

Both patches are simple and correct; I'm uploading a new version of the second patch because I want to rename the decode_uri variable in the second patch to unescaped_uri, which seems more sensible to me.
Comment 11 Yosef Or Boczko 2014-12-10 18:15:29 UTC
You are wrong.
Try this url, for example, and see what do you see.
https://he.wikipedia.org/wiki/%D7%99%D7%A9%D7%A8%D7%90%D7%9C#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94

With the patch, I see in the title this label:
https://he.wikipedia.org/wiki/ישראל#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94
But I had to see this title:
https://he.wikipedia.org/wiki/ישראל#כלכה

(in English: https://en.wikipedia.org/wiki/Israel#Economy)

Also, when I click on the title and try to edit the url in the location entry, I see the
uncidoe url:
https://he.wikipedia.org/wiki/%D7%99%D7%A9%D7%A8%D7%90%D7%9C#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94

But I had to see this url: https://he.wikipedia.org/wiki/ישראל#כלכה

(The same for the url in the statusbar)
Comment 12 Michael Catanzaro 2014-12-10 18:30:56 UTC
So it looks like g_uri_unescape_string() can't handle the # character....
Comment 13 Yosef Or Boczko 2014-12-10 18:41:27 UTC
It look like we needs something else to handle this,  but I don't know what or how.
Firefox handle it correctly, so maybe we can to try looking in Firefox's code?
Comment 14 Michael Catanzaro 2014-12-10 18:47:34 UTC
(In reply to comment #12)
> So it looks like g_uri_unescape_string() can't handle the # character....

Er, just kidding. I went digging in that function, couldn't find anything wrong, was about to start looking into GtkLabel... would have been a huge waste of time, those URLs both display fine for me (with the attached patches). The first URL looks like it's displayed unescaped, but that's because the portion after the # was not escaped in the first place. So the patches are fine.

The location entry still needs to be fixed.
Comment 15 Michael Catanzaro 2014-12-10 18:50:24 UTC
Created attachment 292478 [details]
proof
Comment 16 Michael Catanzaro 2014-12-10 18:50:51 UTC
Created attachment 292479 [details]
more proof -- it works
Comment 17 Michael Catanzaro 2014-12-10 19:38:35 UTC
*** Bug 596399 has been marked as a duplicate of this bug. ***
Comment 18 Michael Catanzaro 2014-12-10 19:39:20 UTC
Created attachment 292482 [details] [review]
Properly display UTF-8 URLs in the location entry
Comment 19 Yosef Or Boczko 2014-12-10 19:54:55 UTC
Michael, for a moment I thought you are right, but infact it still dosn't work.

Take this link: https://he.wikipedia.org/wiki/ישראל#כלכלה
It will move you to the link  https://he.wikipedia.org/wiki/ישראל, but not
the the section #כלכלה, and in the title you see ישראל#כלכלה.

Take the escaped unicode link: https://he.wikipedia.org/wiki/%D7%99%D7%A9%D7%A8%D7%90%D7%9C#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94
It will move you to the link https://he.wikipedia.org/wiki/ישראל#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94
and to the section #כלכלה, but the title will be ישראל#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94,
not ישראל#כלכלה.

The same with the float bar (put the cursor on the links in the comment #11 and see.
Comment 20 Michael Catanzaro 2014-12-11 09:36:12 UTC
(In reply to comment #19)
> Michael, for a moment I thought you are right, but infact it still dosn't work.
> 
> Take this link: https://he.wikipedia.org/wiki/ישראל#כלכלה
> It will move you to the link  https://he.wikipedia.org/wiki/ישראל, but not
> the the section #כלכלה, and in the title you see ישראל#כלכלה.

This is an unrelated, very annoying bug: see bug #722718

> Take the escaped unicode link:
> https://he.wikipedia.org/wiki/%D7%99%D7%A9%D7%A8%D7%90%D7%9C#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94
> It will move you to the link
> https://he.wikipedia.org/wiki/ישראל#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94
> and to the section #כלכלה, but the title will be
> ישראל#.D7.9B.D7.9C.D7.9B.D7.9C.D7.94,
> not ישראל#כלכלה.
> 
> The same with the float bar (put the cursor on the links in the comment #11 and
> see.

But isn't that expected? The dots aren't escape characters, only the %s are. The new behavior seems to be exactly the same as Firefox's.
Comment 21 Michael Catanzaro 2014-12-11 10:50:26 UTC
Created attachment 292517 [details] [review]
Display unescaped URIs in the location entry completion
Comment 22 Michael Catanzaro 2014-12-11 11:17:26 UTC
Created attachment 292522 [details] [review]
Display unescaped URIs in the bookmark properties dialog
Comment 23 Michael Catanzaro 2014-12-11 13:11:53 UTC
Still need to do the history dialog, and bookmarks editor window.
Comment 24 Michael Catanzaro 2014-12-11 17:55:34 UTC
Created attachment 292557 [details] [review]
Add ephy_tree_model_node_add_column_full

Allow creating a tree model column that derives its values from a
property, but is modified by a function for display.
Comment 25 Michael Catanzaro 2014-12-11 17:55:40 UTC
Created attachment 292558 [details] [review]
Add ephy_node_view_add_column_full

Allow creating an EphyNodeView column that derives its values from a
property, but is modified by a function for display.
Comment 26 Michael Catanzaro 2014-12-11 17:55:45 UTC
Created attachment 292559 [details] [review]
Display unescaped URLs in the bookmarks editor
Comment 27 Michael Catanzaro 2014-12-11 18:11:29 UTC
Created attachment 292560 [details] [review]
Display unescaped URIs in the history dialog
Comment 28 Michael Catanzaro 2014-12-11 18:17:15 UTC
OK, done. The patches are all very small/simple, but since there are so many, I wonder if we should convert the URL much earlier internally, unescaping the URL before it ever reaches the history or bookmarks database, rather than unescaping it just before display.
Comment 29 Yosef Or Boczko 2014-12-17 21:35:28 UTC
I think we needs the same patch for the files download's title.
I just downoladed a file and get this title: %D7%90%D7%95%D7%A8%D7%95%D7%AA%20-%20%D7%96%D7%A8%D7%A2%D7%95%D7%A0%D7%99%D7%9D%20-%20%D7%94.%20%D7%99%D7%A1%D7%95%D7%A8%D7%99%D7%9D%20%D7%9E%D7%9E%D7%A8%D7%A7%D7%99%D7%9D.pdf
Comment 30 Michael Catanzaro 2014-12-19 19:09:38 UTC
(In reply to comment #29)
> I think we needs the same patch for the files download's title.
> I just downoladed a file and get this title

I confirm. [1] contains a test for this. But I think this one is different enough to merit a new bug, since we'll probably want to fix it in WebKit.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=619847
Comment 31 Michael Catanzaro 2015-03-19 12:51:29 UTC
Review of attachment 292517 [details] [review]:

This seems to be causing trace #234872
Comment 32 Michael Catanzaro 2015-03-19 12:52:16 UTC
Well that didn't work

  • #0 append_escaped_text
    at gmarkup.c line 2163
  • #1 g_markup_escape_text
    at gmarkup.c line 2239
  • #2 g_markup_vprintf_escaped
    at gmarkup.c line 2492
  • #3 g_markup_printf_escaped
    at gmarkup.c line 2551
  • #4 textcell_data_func
    at ephy-location-entry.c line 1098
  • #5 apply_cell_attributes
    at gtkcellarea.c line 1258
  • #6 g_hash_table_foreach
    at ghash.c line 1607
  • #7 gtk_cell_area_real_apply_attributes
    at gtkcellarea.c line 1287
  • #8 gtk_cell_area_box_apply_attributes
    at gtkcellareabox.c line 1311
  • #9 _gtk_marshal_VOID__OBJECT_BOXED_BOOLEAN_BOOLEANv
    at gtkmarshalers.c line 5040
  • #10 _g_closure_invoke_va
    at gclosure.c line 831
  • #11 g_signal_emit_valist
    at gsignal.c line 3214
  • #12 g_signal_emit
    at gsignal.c line 3361
  • #13 gtk_cell_area_apply_attributes
    at gtkcellarea.c line 2376
  • #14 gtk_tree_view_column_cell_set_cell_data
    at gtktreeviewcolumn.c line 2843
  • #15 validate_row
    at gtktreeview.c line 6304
  • #16 do_validate_rows
    at gtktreeview.c line 6875
  • #17 gtk_tree_view_get_preferred_width
    at gtktreeview.c line 2622
  • #18 gtk_widget_query_size_for_orientation
    at gtksizerequest.c line 180
  • #19 gtk_widget_compute_size_for_orientation
    at gtksizerequest.c line 390
  • #20 gtk_widget_get_preferred_width_for_height
    at gtksizerequest.c line 562
  • #21 _gtk_widget_get_preferred_size_and_baseline
    at gtksizerequest.c line 705
  • #22 gtk_widget_get_preferred_size
    at gtksizerequest.c line 747
  • #23 validate_visible_area
    at gtktreeview.c line 6711
  • #24 do_presize_handler
    at gtktreeview.c line 6984
  • #25 validate_rows
    at gtktreeview.c line 7027
  • #26 gdk_threads_dispatch
    at gdk.c line 717
  • #27 g_main_dispatch
    at gmain.c line 3122
  • #28 g_main_context_dispatch
    at gmain.c line 3737
  • #29 g_main_context_iterate
    at gmain.c line 3808
  • #30 g_main_context_iteration
    at gmain.c line 3869
  • #31 g_application_run
    at gapplication.c line 2308
  • #32 main
    at ephy-main.c line 486
  • #0 append_escaped_text
    at gmarkup.c line 2163
  • #1 g_markup_escape_text
    at gmarkup.c line 2239
  • #2 g_markup_vprintf_escaped
    at gmarkup.c line 2492
  • #3 g_markup_printf_escaped
    at gmarkup.c line 2551
  • #4 textcell_data_func
    at ephy-location-entry.c line 1098
  • #5 apply_cell_attributes
    at gtkcellarea.c line 1258
  • #6 g_hash_table_foreach
    at ghash.c line 1607
  • #7 gtk_cell_area_real_apply_attributes
    at gtkcellarea.c line 1287
  • #8 gtk_cell_area_box_apply_attributes
    at gtkcellareabox.c line 1311
  • #9 _gtk_marshal_VOID__OBJECT_BOXED_BOOLEAN_BOOLEANv
    at gtkmarshalers.c line 5040
  • #10 _g_closure_invoke_va
    at gclosure.c line 831
  • #11 g_signal_emit_valist
    at gsignal.c line 3214
  • #12 g_signal_emit
    at gsignal.c line 3361
  • #13 gtk_cell_area_apply_attributes
    at gtkcellarea.c line 2376
  • #14 gtk_tree_view_column_cell_set_cell_data
    at gtktreeviewcolumn.c line 2843
  • #15 validate_row
    at gtktreeview.c line 6304
  • #16 do_validate_rows
    at gtktreeview.c line 6875
  • #17 gtk_tree_view_get_preferred_width
    at gtktreeview.c line 2622
  • #18 gtk_widget_query_size_for_orientation
    at gtksizerequest.c line 180
  • #19 gtk_widget_compute_size_for_orientation
    at gtksizerequest.c line 390
  • #20 gtk_widget_get_preferred_width_for_height
    at gtksizerequest.c line 562
  • #21 _gtk_widget_get_preferred_size_and_baseline
    at gtksizerequest.c line 705
  • #22 gtk_widget_get_preferred_size
    at gtksizerequest.c line 747
  • #23 validate_visible_area
    at gtktreeview.c line 6711
  • #24 do_presize_handler
    at gtktreeview.c line 6984
  • #25 validate_rows
    at gtktreeview.c line 7027
  • #26 gdk_threads_dispatch
    at gdk.c line 717
  • #27 g_main_dispatch
    at gmain.c line 3122
  • #28 g_main_context_dispatch
    at gmain.c line 3737
  • #29 g_main_context_iterate
    at gmain.c line 3808
  • #30 g_main_context_iteration
    at gmain.c line 3869
  • #31 g_application_run
    at gapplication.c line 2308
  • #32 main
    at ephy-main.c line 486

Comment 33 Michael Catanzaro 2015-07-22 14:20:47 UTC
Comment on attachment 292517 [details] [review]
Display unescaped URIs in the location entry completion

This seems to be a bug in g_markup. I don't think it should hold up this patchset, since nobody seems to be able to reproduce it.
Comment 34 Carlos Garcia Campos 2015-07-23 10:01:49 UTC
I think most of this patches do the escaping at the wrong place.

 * nautilus-loating-bar: this is supposed to be a generic widget that can show any message, not necessarily uris. It should be the caller then one escaping the message only when it's a URI. That message is built inside EphyWebView in ephy_web_view_set_loading_message(). That's probably where the escaping should happen.

 * Title box and location entry: Both are updated from the location controller. And the location controller location is set by the window. I think the window should give the URI escaped to the location controller. Again in this case, sometimes the window doesn't even need to escape the URI, for example when using the typed address.

 * And probably same for the others, I haven't looked at them in detail.

I would probably help to store the escaped URI in EphyWebView on demand, adding a function to get it, ephy_web_view_get_display_address() for example. That way the URL is escaped only once and used everywhere.
Comment 35 Michael Catanzaro 2015-07-25 18:58:29 UTC
(In reply to Carlos Garcia Campos from comment #34)
> I would probably help to store the escaped URI in EphyWebView on demand,
> adding a function to get it, ephy_web_view_get_display_address() for
> example. That way the URL is escaped only once and used everywhere.

Well the EphyWebView already has separate address and URI properties. The address property seems to be what is intended to be displayed to the user, so I think we should just use the address property for this.
Comment 36 Michael Catanzaro 2015-07-25 20:26:38 UTC
(In reply to Carlos Garcia Campos from comment #34)
> I think most of this patches do the escaping at the wrong place.
> 
>  * nautilus-loating-bar: this is supposed to be a generic widget that can
> show any message, not necessarily uris. It should be the caller then one
> escaping the message only when it's a URI. That message is built inside
> EphyWebView in ephy_web_view_set_loading_message(). That's probably where
> the escaping should happen.

OK, I agree, this can be handled in ephy_web_view_set_loading_message() and ephy_web_view_set_link_message().

>  * Title box and location entry: Both are updated from the location
> controller. And the location controller location is set by the window. I
> think the window should give the URI escaped to the location controller.
> Again in this case, sometimes the window doesn't even need to escape the
> URI, for example when using the typed address.

OK, I agree. These cases are both covered by decoding the URI in ephy_web_view_set_address(). With one exception: the location entry completion. This still has to be decoded in EphyLocationEntry itself, as in the patch above, since the GtkTreeModel passed to ephy_location_entry_set_completion() needs to have the percent-encoded form of the URI; only the view can have the decoded URI (since we must ensure not to attempt to load the decoded URI; the bugs would be very subtle: it would work almost always, unless the encoded character was something that would mess with the URI or the server, like a / or a ?).

>  * And probably same for the others, I haven't looked at them in detail.

The other cases are bookmarks dialog and history dialog, which have the same problem as the location entry completion: they need to store the percent-encoded URI, and only convert it for display.
Comment 37 Michael Catanzaro 2015-07-25 20:39:57 UTC
(In reply to Yosef Or Boczko from comment #29)
> I think we needs the same patch for the files download's title.
> I just downoladed a file and get this title:
> %D7%90%D7%95%D7%A8%D7%95%D7%AA%20-
> %20%D7%96%D7%A8%D7%A2%D7%95%D7%A0%D7%99%D7%9D%20-%20%D7%94.
> %20%D7%99%D7%A1%D7%95%D7%A8%D7%99%D7%9D%20%D7%9E%D7%9E%D7%A8%D7%A7%D7%99%D7%9
> D.pdf

Can you provide a link to a file like this? I think something else is wrong with my test case.
Comment 38 Michael Catanzaro 2015-07-25 21:11:16 UTC
Here's a new set of patches; it should properly handle everything except downloads. Note that I am not quite sure what to do with the Copy Location context menu items on the bookmarks and history dialogs; currently those return a percent-encoded URI reflecting the value that's actually stored rather than what is displayed, but maybe those should be decoded too. I think not decoding those is probably the right thing to do, though, so the user can copy-paste the URI into the address bar and be sure that the load will work.
Comment 39 Michael Catanzaro 2015-07-25 21:12:25 UTC
Created attachment 308150 [details] [review]
EphyWebView: decode the URI before storing it in address

This means the address property can now be used to get a user-friendly
decoded address suitable for display. The uri property should now be
used to get the percent-encoded address. It turns out that most uses of
these functions were already correct for this new scheme, but except we
must be sure to store the percent-encoded address in the bookmarks
database and to display the decoded address when creating a web app.

For more information on encoded URIs, see RFC 3986 sections 2.1 and 2.4.
Comment 40 Michael Catanzaro 2015-07-25 21:12:29 UTC
Created attachment 308151 [details] [review]
EphyWebView: Decode URI before setting the loading or status message

This ensures that the URI displayed in the floating bar is decoded.
Comment 41 Michael Catanzaro 2015-07-25 21:12:34 UTC
Created attachment 308152 [details] [review]
Display decoded URIs in the location entry completion
Comment 42 Michael Catanzaro 2015-07-25 21:12:38 UTC
Created attachment 308153 [details] [review]
Display decoded URIs in the bookmark properties dialog
Comment 43 Michael Catanzaro 2015-07-25 21:12:43 UTC
Created attachment 308154 [details] [review]
Add ephy_tree_model_node_add_column_full

Allow creating a tree model column that derives its values from a
property, but is modified by a function for display.
Comment 44 Michael Catanzaro 2015-07-25 21:12:47 UTC
Created attachment 308155 [details] [review]
Add ephy_node_view_add_column_full

Allow creating an EphyNodeView column that derives its values from a
property, but is modified by a function for display.
Comment 45 Michael Catanzaro 2015-07-25 21:12:52 UTC
Created attachment 308156 [details] [review]
Display decoded URIs in the bookmarks editor
Comment 46 Michael Catanzaro 2015-07-25 21:12:56 UTC
Created attachment 308157 [details] [review]
Display decoded URIs in the history dialog
Comment 47 Michael Catanzaro 2015-07-25 21:26:51 UTC
Comment on attachment 308150 [details] [review]
EphyWebView: decode the URI before storing it in address

This one was maybe a bad idea; I already found a few cases I missed. I will add a second property.
Comment 48 Michael Catanzaro 2015-07-25 22:20:18 UTC
Created attachment 308158 [details] [review]
EphyWebView: add display-address property

This stores a decoded URI. It is the only URI that is appropriate for
display to the user.
Comment 49 Michael Catanzaro 2015-07-25 22:20:22 UTC
Created attachment 308159 [details] [review]
Show decoded URI in the new web application dialog
Comment 50 Michael Catanzaro 2015-07-25 22:20:27 UTC
Created attachment 308160 [details] [review]
EphyWindow: store a decoded address, not a percent-encoded address

This means the location controller will get a user-friendly display URI.
Comment 51 Yosef Or Boczko 2015-07-26 09:51:26 UTC
I just test the new patches, and now it works really good, even
when I typed on the location entry I see the right character.

Thanks!
Comment 52 Carlos Garcia Campos 2015-07-27 08:52:36 UTC
Review of attachment 308151 [details] [review]:

Ok
Comment 53 Carlos Garcia Campos 2015-07-27 08:53:01 UTC
Review of attachment 308152 [details] [review]:

Ok
Comment 54 Carlos Garcia Campos 2015-07-27 08:53:25 UTC
Review of attachment 308153 [details] [review]:

Ok
Comment 55 Carlos Garcia Campos 2015-07-27 08:53:52 UTC
Review of attachment 308154 [details] [review]:

Looks good
Comment 56 Carlos Garcia Campos 2015-07-27 08:54:29 UTC
Review of attachment 308155 [details] [review]:

Ok
Comment 57 Carlos Garcia Campos 2015-07-27 08:56:02 UTC
Review of attachment 308156 [details] [review]:

Please, consider my comment before committing.

::: src/bookmarks/ephy-bookmarks-editor.c
@@ +1487,3 @@
+	char *unescaped_url = g_uri_unescape_string (url, NULL);
+	g_value_set_string (value, unescaped_url);
+	g_free (unescaped_url);

This could be simplified and you avoid a heap allocation by using g_value_take_string().

const char *url = g_value_get_string (value);
g_value_take_string (value, g_uri_unescape_string (url, NULL));
Comment 58 Carlos Garcia Campos 2015-07-27 08:57:38 UTC
Review of attachment 308157 [details] [review]:

Please, fix the memory leak before committing.

::: src/ephy-history-window.c
@@ +775,3 @@
+{
+	int col_id = GPOINTER_TO_INT (user_data);
+	const char *url;

This is not correct, gtk_tree_model_get will return a copy of the string that you should free.
Comment 59 Carlos Garcia Campos 2015-07-27 09:00:46 UTC
Review of attachment 308158 [details] [review]:

I'm not sure we want another property for this. This is actually a different view of the existing address property, that we store just for convenience to return a const char and avoid decoding multiple times. If you need to monitor it, you can connect to the notify of the address property and then call get_display_address(). So, I think this could be simplified by just decoding the current address on demand in get_display_address().
Comment 60 Carlos Garcia Campos 2015-07-27 09:01:12 UTC
Review of attachment 308159 [details] [review]:

Ok
Comment 61 Carlos Garcia Campos 2015-07-27 09:01:51 UTC
Review of attachment 308160 [details] [review]:

Perfect.
Comment 62 Michael Catanzaro 2015-07-28 00:36:52 UTC
(In reply to Carlos Garcia Campos from comment #57)
> ::: src/bookmarks/ephy-bookmarks-editor.c
> @@ +1487,3 @@
> +	char *unescaped_url = g_uri_unescape_string (url, NULL);
> +	g_value_set_string (value, unescaped_url);
> +	g_free (unescaped_url);
> 
> This could be simplified and you avoid a heap allocation by using
> g_value_take_string().
> 
> const char *url = g_value_get_string (value);
> g_value_take_string (value, g_uri_unescape_string (url, NULL));

Cool, thanks.

(In reply to Carlos Garcia Campos from comment #58)
> ::: src/ephy-history-window.c
> @@ +775,3 @@
> +{
> +	int col_id = GPOINTER_TO_INT (user_data);
> +	const char *url;
> 
> This is not correct, gtk_tree_model_get will return a copy of the string
> that you should free.

Oops, good catch.

(In reply to Carlos Garcia Campos from comment #59)
> I'm not sure we want another property for this. This is actually a different
> view of the existing address property, that we store just for convenience to
> return a const char and avoid decoding multiple times. If you need to
> monitor it, you can connect to the notify of the address property and then
> call get_display_address(). So, I think this could be simplified by just
> decoding the current address on demand in get_display_address().

OK.
Comment 63 Michael Catanzaro 2015-07-28 00:39:33 UTC
Attachment 308151 [details] pushed as b2f5f1a - EphyWebView: Decode URI before setting the loading or status message
Attachment 308152 [details] pushed as 17e4d34 - Display decoded URIs in the location entry completion
Attachment 308153 [details] pushed as 072ccac - Display decoded URIs in the bookmark properties dialog
Attachment 308154 [details] pushed as e9f2882 - Add ephy_tree_model_node_add_column_full
Attachment 308155 [details] pushed as 9652445 - Add ephy_node_view_add_column_full
Attachment 308156 [details] pushed as 7218cb4 - Display decoded URIs in the bookmarks editor
Attachment 308157 [details] pushed as 71be3ea - Display decoded URIs in the history dialog
Comment 64 Michael Catanzaro 2015-07-28 00:40:56 UTC
Naturally I pushed before fixing your comments... sigh.
Comment 65 Michael Catanzaro 2015-07-28 00:49:51 UTC
Created attachment 308269 [details] [review]
Fixup for 'display decoded URIs in bookmarks editor'

I pushed before addressing Carlos's review comments. Bad Michael!
Comment 66 Michael Catanzaro 2015-07-28 00:50:02 UTC
Created attachment 308270 [details] [review]
Fixup for 'display decoded URIs in the history dialog'
Comment 67 Michael Catanzaro 2015-07-28 00:50:14 UTC
Attachment 308269 [details] pushed as 339e290 - Fixup for 'display decoded URIs in bookmarks editor'
Attachment 308270 [details] pushed as dbee788 - Fixup for 'display decoded URIs in the history dialog'
Comment 68 Michael Catanzaro 2015-07-28 00:55:57 UTC
(In reply to Carlos Garcia Campos from comment #59)
> So, I think this could be simplified by just
> decoding the current address on demand in get_display_address().

I'd rather decode in set_address() and store it in the priv struct without any property. Otherwise, the caller of get_display_address() has to free the memory.
Comment 69 Michael Catanzaro 2015-07-28 01:00:52 UTC
Created attachment 308271 [details] [review]
EphyWebView: add get_display_address()

This returns a decoded URI. It is the only URI that is appropriate for
display to the user.
Comment 70 Michael Catanzaro 2015-07-28 01:00:57 UTC
Created attachment 308272 [details] [review]
Show decoded URI in the new web application dialog
Comment 71 Michael Catanzaro 2015-07-28 01:01:04 UTC
Created attachment 308273 [details] [review]
EphyWindow: store a decoded address, not a percent-encoded address

This means the location controller will get a user-friendly display URI.
Comment 72 Michael Catanzaro 2015-07-28 01:06:47 UTC
Created attachment 308274 [details] [review]
EphyWebView: add get_display_address()

This returns a decoded URI. It is the only URI that is appropriate for
display to the user.
Comment 73 Michael Catanzaro 2015-07-28 01:06:51 UTC
Created attachment 308275 [details] [review]
Show decoded URI in the new web application dialog
Comment 74 Michael Catanzaro 2015-07-28 01:06:56 UTC
Created attachment 308276 [details] [review]
EphyWindow: store a decoded address, not a percent-encoded address

This means the location controller will get a user-friendly display URI.
Comment 75 Michael Catanzaro 2015-07-28 01:20:54 UTC
Blaarg Bugzilla... OK Carlos, can you look over attachment #308274 [details]? I'm not sure if that's what you want.
Comment 76 Carlos Garcia Campos 2015-07-28 12:52:24 UTC
(In reply to Michael Catanzaro from comment #68)
> (In reply to Carlos Garcia Campos from comment #59)
> > So, I think this could be simplified by just
> > decoding the current address on demand in get_display_address().
> 
> I'd rather decode in set_address() and store it in the priv struct without
> any property. Otherwise, the caller of get_display_address() has to free the
> memory.

I'm not proposing to not store the decoded value, but to do it only the first time get_display_address is called instead of in set_address. I guess it doesn't matter in this case, because get_display_address will always be called, even in case of redirection for the provisional URI.
Comment 77 Carlos Garcia Campos 2015-07-28 12:53:37 UTC
Review of attachment 308274 [details] [review]:

Ok
Comment 78 Michael Catanzaro 2015-07-28 15:13:26 UTC
(In reply to Carlos Garcia Campos from comment #76)
> I'm not proposing to not store the decoded value, but to do it only the
> first time get_display_address is called instead of in set_address. I guess
> it doesn't matter in this case, because get_display_address will always be
> called, even in case of redirection for the provisional URI.

I prefer to do it in set_address(); your suggestion seems like a hyperoptimization.
Comment 79 Michael Catanzaro 2015-07-28 15:15:35 UTC
Attachment 308274 [details] pushed as a3317ec - EphyWebView: add get_display_address()
Attachment 308275 [details] pushed as 9610744 - Show decoded URI in the new web application dialog
Attachment 308276 [details] pushed as 3c4525d - EphyWindow: store a decoded address, not a percent-encoded address