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

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

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

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i would say this comes from the change of bookmarks properties dialog to use the editItemOverlay, bug 411261. should be confirmed though

Comment 2

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

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.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

this unifies behaviour with the Library, fixes bug 473442 too

Comment 4

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

There is bug 471096 floating around which could be the same issue.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

[Dietrich Ayala (:dietrich)]

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?

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

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

Comment 8

[Dietrich Ayala (:dietrich)]

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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

addressed comments

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/05bdacee4e7a

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 368018 [details] [diff] [review]
patch v1.2

wanted, yay, broken root names beware!

risk is medium

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

a test is being added in bug 485458

Comment 14

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

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?

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 16

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

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 17

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

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

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+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?

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fc3184431e34

Comment 21

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

(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

Comment 22

[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