Closed Bug 479348 Opened 13 years ago Closed 13 years ago

Choosing Properties on a Special Folder changes its title to (no title)

Categories

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

3.5 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: alice0775, Assigned: mak)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090219 Firefox/3.1b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090219 Gecko/20080102 Minefield/3.2a1pre ID:20090219032045

When I select properties of the Special Folder (Bookmarks Toolbar, Bookmarks  Menu and Unsorted Mookmarks)in the Sidebar, and I click "Cancel" button in the Properties window,
The title of Special Folder becomes (no title) in the Left and Right pane of  the Library


Reproducible: Always

Steps to Reproduce:
1. Start Minefield with clean profile.
2. View > Sidebar > Bookmarks
3. Right Click on 'Unsorted Bookmarks' and Select Properties
4. Click Cancel on Properties Window
5. Bookmarks > Organize Bookmarks
Actual Results:  
The name of 'Unsorted Bookmarks' is changed as followings;
(no title) in the left pane
(no title) in the right upper pane
'Unsorted Bookmarks' in the Bookmarks Sidebar

Expected Results:  
Title should not be changed.

Furthermore ,
6. select (no title) which should be 'Unsorted Bookmarks' in the left pane

The name is stll 'Unsorted Bookmarks' in the detail pane.

[Note]
This problem arises also in brunch built
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090219 Shiretoko/3.1b3pre ID:20090219033707.

This problem does not arise in 3.0.*.
Flags: blocking-firefox3.1?
Version: unspecified → 3.1 Branch
i would say this comes from the change of bookmarks properties dialog to use the editItemOverlay, bug 411261. should be confirmed though
Assignee: nobody → mak77
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
We'd take a fix, but I don't think this blocks. As a regression fix it's something I think the Places team should try to get done, though.
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Blocks: 473442
Attached patch patch v1.0 (obsolete) — Splinter Review
this unifies behaviour with the Library, fixes bug 473442 too
Attachment #363864 - Flags: review?(dietrich)
There is bug 471096 floating around which could be the same issue.
(In reply to comment #4)
> There is bug 471096 floating around which could be the same issue.

no, that's unrelated, this is caused by the properties dialog
Comment on attachment 363864 [details] [diff] [review]
patch v1.0


>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -159,24 +159,19 @@ PlacesController.prototype = {
>       return this._canInsert();
>     case "placesCmd_new:separator":
>       return this._canInsert() &&
>              !asQuery(this._view.getResult().root).queryOptions.excludeItems &&
>              this._view.getResult().sortingMode ==
>                  Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>     case "placesCmd_show:info":
>       var selectedNode = this._view.selectedNode;
>-      if (selectedNode) {
>-        if (PlacesUtils.nodeIsFolder(selectedNode) ||
>-            (PlacesUtils.nodeIsQuery(selectedNode) &&
>-             selectedNode.itemId != -1) ||
>-            (PlacesUtils.nodeIsBookmark(selectedNode) &&
>-            !PlacesUtils.nodeIsLivemarkItem(selectedNode)))
>-          return true;
>-      }
>+      if (selectedNode &&
>+          PlacesUtils.getConcreteItemId(selectedNode) != -1)
>+        return true;

what about the livemark child case?
`
>@@ -657,21 +652,25 @@ PlacesController.prototype = {
>    * Opens the bookmark properties for the selected URI Node.
>    */
>   showBookmarkPropertiesForSelection: 
>   function PC_showBookmarkPropertiesForSelection() {
>     var node = this._view.selectedNode;
>     if (!node)
>       return;
> 
>-    if (PlacesUtils.nodeIsFolder(node))
>-      PlacesUIUtils.showItemProperties(node.itemId, "folder");
>-    else if (PlacesUtils.nodeIsBookmark(node) ||
>-             PlacesUtils.nodeIsQuery(node))
>-      PlacesUIUtils.showItemProperties(node.itemId, "bookmark");
>+    var itemType = PlacesUtils.nodeIsFolder(node) ||
>+                   PlacesUtils.nodeIsTagQuery(node) ? "folder" : "bookmark";
>+    var concreteId = PlacesUtils.getConcreteItemId(node);
>+    var isRootItem = PlacesUtils.isRootItem(concreteId);
>+    var useConcreteId = isRootItem ||
>+                        PlacesUtils.nodeIsTagQuery(node);
>+
>+    PlacesUIUtils.showItemProperties(useConcreteId ? concreteId : node.itemId,
>+                                     itemType, isRootItem);
>   },

using inline ternary expressions hurts code readability. please move the determination out of the function call, and add a comment to describe what's going on there.

>+  /**
>+   * Checks if aItemId is a root.
>+   *
>+   *   @param aItemId
>+   *          item id to look for.
>+   *   @returns true if aItemId is a root, false otherwise.
>+   */
>+  isRootItem: function PU_isRootItem(aItemId) {
>+    return aItemId == PlacesUtils.bookmarksMenuFolderId ||
>+           aItemId == PlacesUtils.toolbarFolderId ||
>+           aItemId == PlacesUtils.unfiledBookmarksFolderId;
>   },

why not tag and places root?
Attachment #363864 - Flags: review?(dietrich) → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
(In reply to comment #6)
> (From update of attachment 363864 [details] [diff] [review])
> >diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
> >     case "placesCmd_show:info":
> >       var selectedNode = this._view.selectedNode;
> >-      if (selectedNode) {
> >-        if (PlacesUtils.nodeIsFolder(selectedNode) ||
> >-            (PlacesUtils.nodeIsQuery(selectedNode) &&
> >-             selectedNode.itemId != -1) ||
> >-            (PlacesUtils.nodeIsBookmark(selectedNode) &&
> >-            !PlacesUtils.nodeIsLivemarkItem(selectedNode)))
> >-          return true;
> >-      }
> >+      if (selectedNode &&
> >+          PlacesUtils.getConcreteItemId(selectedNode) != -1)
> >+        return true;
> 
> what about the livemark child case?

For livemarks children Properties isn't even visible, so there's no need to change the enabled state. Btw i've added it for completeness in case we want to reenable it in future.

> >@@ -657,21 +652,25 @@ PlacesController.prototype = {
> >+    PlacesUIUtils.showItemProperties(useConcreteId ? concreteId : node.itemId,
> >+                                     itemType, isRootItem);
> >   },
> 
> using inline ternary expressions hurts code readability. please move the
> determination out of the function call, and add a comment to describe what's
> going on there.

done, even if in this case i think ternary was helping readability instead, added comments too.

> >+  /**
> >+   * Checks if aItemId is a root.
> >+   *
> >+   *   @param aItemId
> >+   *          item id to look for.
> >+   *   @returns true if aItemId is a root, false otherwise.
> >+   */
> >+  isRootItem: function PU_isRootItem(aItemId) {
> >+    return aItemId == PlacesUtils.bookmarksMenuFolderId ||
> >+           aItemId == PlacesUtils.toolbarFolderId ||
> >+           aItemId == PlacesUtils.unfiledBookmarksFolderId;
> >   },
> 
> why not tag and places root?

mostly because they are not exposed anywhere in the UI, but since this is toolkit utils i've added them.
Attachment #363864 - Attachment is obsolete: true
Attachment #367755 - Flags: review?(dietrich)
Summary: The title of Special Folder becomes (no title) → Choosing Properties on a Special Folder changes its title to (no title)
Comment on attachment 367755 [details] [diff] [review]
patch v1.1

>+      // the concrete item properties for shortcuts to root nodes.
>+      var concreteId = PlacesUtils.getConcreteItemId(aSelectedNode);
>+      var isRootItem = concreteId != -1 && PlacesUtils.isRootItem(concreteId);
>+      var readOnly = isRootItem ||
>+                     aSelectedNode.parent.itemId == PlacesUIUtils.leftPaneFolderId;
>+      var useConcreteId = isRootItem ||
>+                          PlacesUtils.nodeIsTagQuery(aSelectedNode);
> 

nit: whitespace, since you're there

>-      }
>-      else {
>-        var itemId = PlacesUtils.getConcreteItemId(aSelectedNode);
>-        gEditItemOverlay.initPanel(itemId != -1 ? itemId :
>-                                   PlacesUtils._uri(aSelectedNode.uri),
>-                                   { hiddenRows: ["folderPicker"] });
>-      }
>+      var itemId = concreteId != -1 ?
>+        useConcreteId ? concreteId : aSelectedNode.itemId :
>+        PlacesUtils._uri(aSelectedNode.uri);

curse your nested conditionals! temporary local vars are really not *that* expensive. however, every extra minute of lost time trying to figure out code *is*. and i'm not even going to try to quantify all the potential contributors that look at our code and run away screaming.

r=me otherwise.
Attachment #367755 - Flags: review?(dietrich) → review+
(In reply to comment #8)
> (From update of attachment 367755 [details] [diff] [review])
> curse your nested conditionals! temporary local vars are really not *that*
> expensive.

uh sorry, i really didn't look at that, it is effectively confusing, more then the other one.
Attached patch patch v1.2Splinter Review
addressed comments
Attachment #367755 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/05bdacee4e7a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Attachment #368018 - Flags: approval1.9.1?
Comment on attachment 368018 [details] [diff] [review]
patch v1.2

wanted, yay, broken root names beware!

risk is medium
Depends on: 485458
a test is being added in bug 485458
Flags: in-testsuite?
So what should happen with such folders which names were changed before this patch went into the tree? The properties window doesn't allow to change the name anymore and shows e.g. "Bookmarks Toolbar" while the real folder is named "Test". Why there is a difference?
So we should not allow users to rename roots, i know branch allows that, but is wrong.
We can fix that with preventive maintenance, i already have a task there but is currently disabled being a risky change. But if this is going to nag a lot of users i can come with a complete test and enable the task.

Henrik could you please file a followup to fix users roots edited names?
Depends on: 484019
Marco, why is the textbox under the properties dialog not set as read only on OS X? It should be greyed out but shows the normal focus. That's a bit misleading.

(In reply to comment #15)
> Henrik could you please file a followup to fix users roots edited names?

Will be covered by bug 484019.
Comment on attachment 368018 [details] [diff] [review]
patch v1.2

a=191, please make sure to land with the tests included in bug 485458 (which don't need approval, as they're tests!)
Attachment #368018 - Flags: approval1.9.1? → approval1.9.1+
(In reply to comment #16)
> Marco, why is the textbox under the properties dialog not set as read only on
> OS X? It should be greyed out but shows the normal focus. That's a bit
> misleading.

Marco, same applies to Windows. The textbox has a white background which implies that it is active. Shall I file new bug on making it look disabled?
if you're talking about Vista, could be same as bug 451771?
On Vista no disabled input field is ever gray for me, this could also be an ux theme bug if other apps do gray their disabled input fields.
Flags: in-testsuite? → in-testsuite+
(In reply to comment #19)
> if you're talking about Vista, could be same as bug 451771?
> On Vista no disabled input field is ever gray for me, this could also be an ux
> theme bug if other apps do gray their disabled input fields.

Looks like. Let's move this issue over to this bug.

Verified fixed on trunk and 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090408 Minefield/3.6a1pre ID:20090408030741

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre ID:20090408030745
Status: RESOLVED → VERIFIED
OS: Windows XP → All
Hardware: x86 → All
Blocks: 434441
Blocks: 415114
Blocks: 421530
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.