373500 - Generated titles (microsummaries) are not used in tree and menu places views

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Asaf Romano (gone)]

Neither the tree view nor the menu view support microsummaries.

Comment 1

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

ran into this while working on bug #378921.

is it bad enough (or are microsummaries used enough) to consider this a places-on-bookmarks blocker?

Comment 2

[Asaf Romano (gone)]

Not that important, certainly a beta blocker though.

Comment 3

[Asaf Romano (gone)]

To get this working right, especially with regard to title-based sorting, we ought to change the places-version of the microsummaries service _not_ to use an annotation for the generated title, but rather set the generated title directly on the bookmark item and cache the user-title somewhere else...

Comment 4

[Asaf Romano (gone)]

(..and for that reason, this should remain a a5 blocker so we're not forced to write more migration code for the bleeding edge).

Comment 5

[Dietrich Ayala (:dietrich)]

Ryan, are you working on this for A6?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

FWIW, I chatted with Ryan yesterday, and he mentioned that he's working on a fix for this bug.

Comment 7

[Dietrich Ayala (:dietrich)]

retargeting bugs that don't meet the alpha release-blocker criteria at http://wiki.mozilla.org/Firefox3/Schedule.

Comment 9

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

Comment 10

[Asaf Romano (gone)]

Comment on attachment 273795 [details] [diff] [review]
Patch

So, as discussed, this doesn't restore the static title when removing a microsummary from a bookmark.

Comment 11

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2

Adds a _setTitle call to removeMicrosummary

Comment 12

[Asaf Romano (gone)]

Comment on attachment 273819 [details] [diff] [review]
Patch v2

>Index: components/microsummaries/src/nsMicrosummaryService.js
>===================================================================

>+    var title = this._getTitle(bookmarkID);
>+
>+    // Ensure the user-given title is cached
>+    if (!this._hasField(bookmarkID, FIELD_STATIC_TITLE))
>+      this._setField(bookmarkID, FIELD_STATIC_TITLE, title);
> 

please file a bug for removing these wrappers, they're no longer valuable.

>     // A string identifying the bookmark to use when logging the update.
>     var bookmarkIdentity = bookmarkID;
> 
>-    if (oldValue == null || oldValue != microsummary.content) {
>-      this._setField(bookmarkID, FIELD_GENERATED_TITLE, microsummary.content);
>+    // Update if the microsummary differs from the current title.
>+    if (title == null || title != microsummary.content) {

prefer if !title or title == "" here, it's a string.

>    */
>   setMicrosummary: function MSS_setMicrosummary(bookmarkID, microsummary) {
>     this._setField(bookmarkID, FIELD_MICSUM_GEN_URI, microsummary.generator.uri.spec);
> 
>     if (microsummary.content) {
>       this._updateMicrosummary(bookmarkID, microsummary);
>     }
>     else {
>-      // Display a static title initially (unless there's already one set)
>-      if (!this._hasField(bookmarkID, FIELD_GENERATED_TITLE))
>-        this._setField(bookmarkID, FIELD_GENERATED_TITLE,
>-                       microsummary.generator.name || microsummary.generator.uri.spec);
>-
>+      // Use the bookmark's title for now and attempt an update
>       this.refreshMicrosummary(bookmarkID);
>     }
>   },
> 
>   /**
>    * Remove the current microsummary for the given bookmark.
>    *
>    * @param   bookmarkID
>    *          the bookmark for which to remove the current microsummary
>    *
>    */
>   removeMicrosummary: function MSS_removeMicrosummary(bookmarkID) {
>     var fields = [FIELD_MICSUM_GEN_URI,
>                   FIELD_MICSUM_EXPIRATION,
>-                  FIELD_GENERATED_TITLE,
>+                  FIELD_STATIC_TITLE,
>                   FIELD_CONTENT_TYPE];
> 
>-    for ( var i = 0; i < fields.length; i++ ) {
>+    for (let i = 0; i < fields.length; i++) {
>       var field = fields[i];
>-      if (this._hasField(bookmarkID, field))
>+      if (this._hasField(bookmarkID, field)) {
>+        // Restore the user's title
>+        if (field == FIELD_STATIC_TITLE)
>+          this._setTitle(bookmarkID, this._getField(bookmarkID, field));
>+

For readability, I would handle FIELD_STATIC_TITLE separately.

>Index: components/places/content/bookmarkProperties.js
>===================================================================
>@@ -547,17 +548,23 @@ var BookmarkPropertiesPanel = {
>     }
> 
>     return menuItem;
>   },
> 
>   _initNamePicker: function BPP_initNamePicker() {
>     var userEnteredNameField = this._element("userEnteredName");
>     var namePicker = this._element("namePicker");
>-    userEnteredNameField.label = this._itemTitle;
>+    const annos = PlacesUtils.annotations;
>+
>+    if (annos.itemHasAnnotation(this._bookmarkId, STATIC_TITLE_ANNO))
>+      userEnteredNameField.label = annos.getItemAnnotation(this._bookmarkId,
>+                                                           STATIC_TITLE_ANNO);

braces please.


>Index: components/places/src/nsPlacesImportExportService.cpp
>===================================================================

Please file a follow up on restoring this functionality.

r=mano otherwise.

Comment 13

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: As checked in

Followups filed as bugs 389578 and 389579.

Comment 14

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js 	1.68 	mozilla/browser/components/places/content/bookmarkProperties.js 	1.59 	mozilla/browser/components/places/content/toolbar.xml 	1.100 	mozilla/browser/components/places/tests/unit/test_placesTxn.js 	1.5 	mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 	1.27 

Comment 16

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre

Comment 17

[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