425851 - "Most Visited", "Recently Bookmarked", and "Recent Tags" should be treated as folders

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5 ID:2008032619

Currently selecting any of the three default saved searches under "Smart Bookmarks", let you change the Location or set a keyword. It shouldn't be possible.

Steps to reproduce:
1. Open Library and choose "Smart Bookmarks" within the left pane
2. Select "Recent Tags"

Now the details pane shows the query string for that saved search, which can be modified. You can even set a keyword. Using this keyword later results in an alert message:

Firefox doesn't know how to open this address, because the protocol (place) isn't associated with any program.

Comment 1

[bomfog]

Keywords I could care less about (though not by much), but I hope the query in the location box can stay exposed, un-modifiable if need be. Else-wise discoverability, customization and the tinkering experience would seem to be severely degraded.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets ask for blocking-firefox3 to get a clear statement from drivers.

Comment 3

[Mike Connor [:mconnor]]

This makes a lot of sense.

My take is that exposing place: URIs isn't useful, if you want to use place, read the docs or build an extension.

Comment 4

[Marco Bonardo [:mak]]

what if a user wants to change the max number of records returned by those queries?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Uh, was this fixed by something else? I'm not seeing locations for smart bookmarks anymore.

Marco: I think we need to rely on add-ons and future work to help people build queries, not poorly supported UI which can easily be messed up in ways that are damaging.

Connor actually meant to minus this, despite his comment and action to the contrary. Doesn't block, might actually be fixed, very confusing!

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> Uh, was this fixed by something else? I'm not seeing locations for smart
> bookmarks anymore.

Mike, i see them...

Comment 7

[Reed Loden [:reed]]

As of the landing of bug 421483, the context menu for "Properties" is missing for the "Most Visited" folder, making it very difficult for me to rename the folder. MaK77 showed me how I could get to it in the Library to rename it, but it would be nice to have the Properties option back, as other folders offer this option.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch

enable properties context menu for queries (but not for generated queries without an id, like date containers).
allow only name edit for queries (both in Library and in Properties dialog), hide other fields.
disable properties for folder shortcuts (we are not supporting them actually and show Edit properties for "(null)" :( plus we should not allow editing name of roots folder shortcuts)

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch

better, consider them bookmarks in both panels

Comment 10

[Asaf Romano (gone)]

Oh, hrm, can you update this patch to only fix the bookmark properties dialog once i land bug 407541?

Comment 11

[Marco Bonardo [:mak]]

will see what will be left after that check-in

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Look good. Selecting any of the mentioned folders only shows the name and description. The user doesn't have the opportunity to change the location anymore.

Which patch fixed this issue?

Comment 13

[Asaf Romano (gone)]

Bug 407541, we still need to fix the old dialog though.

Comment 14

[Marco Bonardo [:mak]]

this should be at least wanted+ to be consistent with the Library details pane

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch

what this patch contains:

- fixed title field for tag containers (we need the concreteItemId)
- changed "load in sidebar" to "loadInSidebar" to be consistent with editOverlay
- unified _folderId, _bookmarkId to _itemId (unique identifier)
- changed _bookmarkURI to _uri to be consistent with editOverlay
- changed _forceHideRows to work more like showHideRows of editOverlay, now bookmarkProperties can show/hide wisely based on content (now renamed to _showHideRows)
- removed duped show/hide code from _populateProperties
- _showHideRows is called directly from _populateProperties
- re-enabled Properties context menuitem for queries on toolbar/menus
- removed no more used code from places.js
- unified showFolderProperties and showBookmarkProperties to showItemProperties

this work should make bookmarkProperties more similar to editOverlay, making it easier to maintain

Comment 16

[Marco Bonardo [:mak]]

reasking blocking (wanted, really), see comment #14

Comment 17

[Marco Bonardo [:mak]]

Attachment: patch

fixed minor glitch in showItemProperties method

Comment 18

[Asaf Romano (gone)]

Comment on attachment 317698 [details] [diff] [review]
patch

r=mano

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 317698 [details] [diff] [review]
patch

asking approval for consistency between edit bookmarks panes and dialogs

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 317698 [details] [diff] [review]
patch

a=mconnor on behalf of 1.9 drivers

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: unbitrot

Fixes minor conflict with the patch from bug 430213.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/places/content/bookmarkProperties.js 	1.86
mozilla/browser/components/places/content/controller.js 	1.233
mozilla/browser/components/places/content/places.js 	1.162
mozilla/browser/components/places/content/placesOverlay.xul 	1.36
mozilla/browser/components/places/content/utils.js 	1.140 

Comment 23

[Marco Bonardo [:mak]]

thank you Gavin for the unbitrot

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042907 Minefield/3.0pre

Comment 25

[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