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.

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)

> at least, it matches the title in history (moz_places table) note, this is by design (see bug #387996 comment #35)

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

> 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.

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.

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 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

> nit: please indent 2 spaces here indented by 2 spaces locally, thanks for the prompt review dietrich.

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

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.)

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.

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.

> 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.

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 on attachment 288032 [details] [diff] [review] patch clearing review/approval, per mano's comments.

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

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.

(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.

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.

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

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

Marco: fixed by bug 420520?

(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.

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

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 #26 sounds good to me. Either way, despite this being ugly, I don't think it's gonna block.

(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.

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.

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