419324 - Places needs to unescape URLs in the UI (library tree/edit, bookmark tooltip/status, etc)

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, defect)

Description

[Ed Lee :Mardak]

When bug 415460 lands, the url listed in the treeview will correctly be unescaped, but clicking a bookmark to show more information about it has the %escaped% url because it uses uri.spec.

We can keep the two consistent by showing what the tree row shows.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Comment 2

[Asaf Romano (gone)]

hrm, but what happens once you change the field contents?

Comment 3

[Ed Lee :Mardak]

Oh interesting. The tree view shows %bl%ah%ju%nk but if you close the window and reopen it, it shows the value from the database.

I'm assuming there was an optimization to avoid hitting the DB again when the location changed?

Comment 4

[Ed Lee :Mardak]

Comment on attachment 305374 [details] [diff] [review]
v1

(In reply to comment #2)
> hrm, but what happens once you change the field contents?
Updated the patch in bug 415460 to support incremental/live bookmark update, so the url in the tree view is also unescaped. So this patch is good as is.

Comment 5

[Ed Lee :Mardak]

I suppose technically this doesn't need to wait for bug 415460 to land. Except the tree will be showing escaped URLs while the edit panel has unescaped.

Comment 6

[Ed Lee :Mardak]

Attachment: v2

This doesn't require the backend changes from bug 415460 that probably won't happen. But probably won't land until the JSON patch as that will be moving PlaceUtils methods around.

Comment 7

[Ed Lee :Mardak]

Attachment: v2.1

This fixes the URI for UI in the bookmark toolbar tooltip, edit bookmarks in Library, statusbar for sidebar, and Library tree view.

The statusbar still shows escaped URIs when hovering over the bookmark item though...

Comment 8

[Marco Bonardo [:mak]]

Edward: what's the status here? who's the reviewer?

Comment 9

[Ed Lee :Mardak]

I believe there was a comment from mano about potentially running into performance issues because something (tree view?) would keep asking for the URLs, so if we keep unescaping them on the fly, there could be issues.

Comment 11

[Masahiro YAMADA]

Comment on attachment 305705 [details] [diff] [review]
v2.1

Foreced encode of bidi is missing.
See also browser.js

Comment 12

[Masahiro YAMADA]

This bug may be related to (or depends on ) bug 320807.

Comment 13

[Marco Bonardo [:mak]]

can we take a patch with bidi fixed (we can probably inherit the bidi code from browser.js losslessdecodeURI function), skipping the treeView change?
Editing is probably the more real issue users find (bug 449994, that i'm going to mark as dependant on this)

Comment 14

[Marco Bonardo [:mak]]

and probably the helper should be in PlacesUIUtils

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 305705 [details] [diff] [review]
v2.1

please request review from an actual peer

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 17

[al_9x]

Why are spaces (%20) unescaped in urls in the urlbar?  Is there an option to show them escaped?

You want spaces unescaped in places to facilitated editing, but the urlbar, at least optionally, should show the actual url.

Comment 18

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.