400590 - children of tag queries have empty titles in edit bookmark dialog / preview pane

Status: VERIFIED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[Florian]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102004 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102004 Minefield/3.0a9pre

I will describe my case and a case which i reproduced with a new firefox profile. View "Steps to reproduce" for the case with the new profile.

My case:

If I view Places/Most Used Tags/anyTag/, then none of the bookmarks has a title. However I can right click the entry and give it a title. My bookmark has then a new title in this view, but it has it's old title in Places/Recently Starred Pages view.



Reproducible: Always

Steps to Reproduce:
1. create a new profile
2. bookmark a page using the star, and give it a tag.
3. go to Places/Recently Used Tags/yourTag and right click the new bookmark in order to edit it.
4. You will see that the bookmark doesn't has the title you gave it at creation. The title of the page tell you that the bookmark has the title "null", and the title field is empty.
5. Give the bookmark a title.
6. The title ofthe bookmark will change in Places/Recently Used Tags/yourTag, but Places/Recently Starred Pages will show the old original title.

Comment 1

[(not reading, please use [e-mail] instead)]

I'm able to reproduce this on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102005 Minefield/3.0a9pre.

to clarify, I'm right clicking on the menu item (which is not possible on mac)

a couple of comments:

1)  you can see a possibly related "null" title issue if you select an item under Places/Recently Used Tags/yourTag in the places organizer.  the "name" column is correct [at least, it matches the title in history (moz_places table)] but the name in the preview pane is blank.

2)  I'm not sure if we should allow all the context menu commands for items under tag queries.  (what does delete mean?).  note, context menu click of the tag itself will result in an assertion (see bug #400109)

Comment 2

[(not reading, please use [e-mail] instead)]

> at least, it matches the title in history (moz_places table)

note, this is by design (see bug #387996 comment #35)

Comment 3

[Dietrich Ayala (:dietrich)]

Seth, is this fixed with the resolveNullBookmarkTitles change? Or does that need to be applied in this case?

Comment 4

[(not reading, please use [e-mail] instead)]

> Seth, is this fixed with the resolveNullBookmarkTitles change? Or does that
> need to be applied in this case?

this is not fixed yet.  In the preview pane, we use editBookmarkOverlay.js and _getItemStaticTitle() to get the title.

that ends up calling nsNavBookmarks::GetItemTitle(), which does not attempt to resolve null titles against history.

(Note the resolveNullBookmarkTitles option is going away, see bug #387746 for more details)

In nsNavBookmarks::GetItemTitle(), we need to do something similar to what we do in nsNavHistory::ConstructQueryString() [or, what we will be doing after #387746], and if we have a null (note, not empty) title, find it places. 

Comment 5

[(not reading, please use [e-mail] instead)]

note, our query (mDBGetItemProperties) for determining the title (and other properties) is:

SELECT b.id, (SELECT url from moz_places WHERE id = b.fk), b.title, b.position, b.fk, b.parent, b.type, b.folder_type, b.dateAdded, b.lastModified FROM moz_bookmarks b WHERE b.id = ?1

we can join against moz_places and do COALESCE(b.title, h.title) here, and get url from moz_places, too.

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: patch

note, we need to do LEFT OUTER JOIN against moz_places (and not a JOIN) because there are bookmark items (like the personal toolbar folder, etc) that are not in moz_places.

Comment 7

[Dietrich Ayala (:dietrich)]

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

>Index: nsNavBookmarks.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v
>retrieving revision 1.125
>diff -u -8 -p -r1.125 nsNavBookmarks.cpp
>--- nsNavBookmarks.cpp	25 Oct 2007 20:18:41 -0000	1.125
>+++ nsNavBookmarks.cpp	9 Nov 2007 19:53:32 -0000
>@@ -180,19 +180,20 @@ nsNavBookmarks::Init()
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING("SELECT id, fk, type FROM moz_bookmarks WHERE parent = ?1 AND position = ?2"),
>                                getter_AddRefs(mDBGetChildAt));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // get bookmark/folder/separator properties 
>   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING(
>-      "SELECT b.id, (SELECT url from moz_places WHERE id = b.fk), b.title, b.position, b.fk, b.parent, b.type, b.folder_type, b.dateAdded, b.lastModified "
>+      "SELECT b.id, h.url, COALESCE(b.title, h.title), "
>+       "b.position, b.fk, b.parent, b.type, b.folder_type, b.dateAdded, b.lastModified "

nit: please indent 2 spaces here

Comment 8

[(not reading, please use [e-mail] instead)]

> nit: please indent 2 spaces here

indented by 2 spaces locally, thanks for the prompt review dietrich.

Comment 9

[Asaf Romano (gone)]

So, _why_ we're fixing this in nsNavBookmarks rather than in the front-end code?

Comment 10

[(not reading, please use [e-mail] instead)]

mano, you are proposing that we fix the callers of GetItemTitle() instead?  

http://lxr.mozilla.org/seamonkey/search?string=getitemtitle

Can you elaborate on how that's better?  (it seems less ideal than my fix.)

Comment 11

[Asaf Romano (gone)]

I don't want anything in nsINavBookmarkService to return anything non-static, with your patch setItemTilte(id, getItemTitle(id) changes the title of a bookmark and there's no way to sort out it used to be live.  Of course, it also breaks var foo = null; setItemTitle(id, foo); getItemTitle(id) == "foo".

One could make the return-value complex enough to distinguish live-titles from static titles, but it's not worth the API complexity IMO.

Comment 12

[Asaf Romano (gone)]

Btw, what's the purpose of changing the title of an item under a tag container (except the current UI allows that),? we explicitly don't search against it. 

Comment 13

[(not reading, please use [e-mail] instead)]

> Btw, what's the purpose of changing the title of an item under a tag container
> (except the current UI allows that),? we explicitly don't search against it. 

that's an interesting point.  Perhaps for items under a tag container (and possibly other items, like history queries, if those ever show up in organizer) we should not allow the user to edit the properties?

Perhaps this bug should morph into that.

Should we make all the text fields be disabled?  When opening the properties dialog for one of these items (because the preview / detail pane is minimized, should we do the same?

On a related note, see bug #400109, which is also about handling tags and tag items differently.

Comment 14

[Asaf Romano (gone)]

One option, proposed by Mike, is to make the edit-item-panel for a tag/history-item edit the properties for the most-recent-bookmark of the item's url.

Comment 15

[(not reading, please use [e-mail] instead)]

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

clearing review/approval, per mano's comments.

Comment 16

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

I recognized a similar behavior within the Organizer. It happens for bookmarks under 'Tags' and is filed as bug 408345. Are they somewhat related?

Comment 17

[David Duong]

Another option is to add a method to nsINavBookmarksService that returns an array of bookmark ids given a URI (it is possible to bookmark the same URI multiple times) like my (Feature Request Bug 411250), and then use the Bookmarks Service to get the title of the bookmark.

Comment 18

[bomfog]

(In reply to comment #14)
> One option, proposed by Mike, is to make the edit-item-panel for a
> tag/history-item edit the properties for the most-recent-bookmark of the item's
> url.
> 

Seems to me this would be better than dis-allowing editing there. I'd like to be able to edit a bookmark where it's most conveniently/easily found, which might well be under a Smart Bookmarks query.

Unless I misunderstood, in which case, nevermind.

Comment 19

[Marco Bonardo [:mak]]

imho this should be duped to Bug 420520, the problem is the same, tag children are copy of the original bookmark, you should not be able to see properties for the tag children, at least you should see properties of the original bookmark. So Bug 420520 has to implement a way of showing the correct data, or disable editing.

Comment 21

[Marco Bonardo [:mak]]

mh no, since this is a bookmark edit dialog (and not panel) i'm reverting the dupe :(

Comment 22

[Florian]

Does this bug block Firefox 3?
I added a flag request.

Comment 23

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

Marco: fixed by bug 420520?

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #23)
> Marco: fixed by bug 420520?
> 

no, this bug is about right click on a bookmark contained into a tag, the property dialog shows an empty title (they are duped bookmarks with empty title).
that should probably point to the last modified bookmark with the same url.

Comment 25

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

So why wouldn't we just use the same value that we use in the inspector panel, below?

Comment 26

[Asaf Romano (gone)]

I would rather just disallow opening the panel for tagged items (and maybe also don't show the inspector panel either, as we do for history items).

Comment 27

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

Comment #26 sounds good to me. Either way, despite this being ugly, I don't think it's gonna block.

Comment 28

[Dietrich Ayala (:dietrich)]

(In reply to comment #24)
> (In reply to comment #23)
> > Marco: fixed by bug 420520?
> > 
> 
> no, this bug is about right click on a bookmark contained into a tag, the
> property dialog shows an empty title (they are duped bookmarks with empty
> title).
> that should probably point to the last modified bookmark with the same url.
> 

Where are you right-clicking the bookmark to do this? In the organizer, tag container children have no "properties" entry in the context menu. The properties panel seems to behave exactly as you describe - the last modified bookmark is shown.

Comment 29

[Florian]

Originally this bug was about right clicking a bookmark at:
Bookmarks Toolbar/Places/Recently Used Tags/your tag/your bookmark. Since the places menu got removed I can no longer reproduce it with a fresh profile. 

The bookmarks in "Recent Tags/your tag" have now a title and renaming them there over a right click works as well.

Comment 30

[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