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

VERIFIED FIXED in Firefox 3 alpha7

Status

()

Firefox
Bookmarks & History
P1
normal
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: mano, Assigned: rflint)

Tracking

Trunk
Firefox 3 alpha7
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx2-parity])

Attachments

(1 attachment, 2 obsolete attachments)

Neither the tree view nor the menu view support microsummaries.
Summary: Generated titles (microsummaries) are note used in tree and menu places views → Generated titles (microsummaries) are not used in tree and menu places views
Whiteboard: [Fx2-parity]
Blocks: 317881
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?
Not that important, certainly a beta blocker though.
Flags: blocking-firefox3?
Target Milestone: Firefox 3 → Firefox 3 alpha6

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Assignee: nobody → ryan
Target Milestone: Firefox 3 alpha6 → Firefox 3 alpha5
Blocks: 355737
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...
(..and for that reason, this should remain a a5 blocker so we're not forced to write more migration code for the bleeding edge).

Updated

11 years ago
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6
Ryan, are you working on this for A6?
Blocks: 358972
Blocks: 337969
FWIW, I chatted with Ryan yesterday, and he mentioned that he's working on a fix for this bug.
retargeting bugs that don't meet the alpha release-blocker criteria at http://wiki.mozilla.org/Firefox3/Schedule.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Duplicate of this bug: 385498
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: Firefox 3 M7 → Firefox 3 M8
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.
Attachment #273795 - Flags: review?(mano) → review-
Created attachment 273819 [details] [diff] [review]
Patch v2

Adds a _setTitle call to removeMicrosummary
Attachment #273795 - Attachment is obsolete: true
Attachment #273819 - Flags: review?(mano)
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.
Attachment #273819 - Flags: review?(mano) → review+
Created attachment 273830 [details] [diff] [review]
As checked in

Followups filed as bugs 389578 and 389579.
Attachment #273819 - Attachment is obsolete: true
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 
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 M8 → Firefox 3 M7
No longer blocks: 337969
Duplicate of this bug: 337969
verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.