Closed
Bug 479348
Opened 16 years ago
Closed 16 years ago
Choosing Properties on a Special Folder changes its title to (no title)
Categories
(Firefox :: Bookmarks & History, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: alice0775, Assigned: mak)
References
Details
(Keywords: regression, verified1.9.1)
Attachments
(1 file, 2 obsolete files)
9.27 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.*.
Reporter | ||
Updated•16 years ago
|
Assignee | ||
Comment 1•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Priority: -- → P2
Comment 2•16 years ago
|
||
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-
Assignee | ||
Updated•16 years ago
|
Blocks: 411261
Keywords: regressionwindow-wanted
Assignee | ||
Comment 3•16 years ago
|
||
this unifies behaviour with the Library, fixes bug 473442 too
Attachment #363864 -
Flags: review?(dietrich)
Comment 4•16 years ago
|
||
There is bug 471096 floating around which could be the same issue.
Assignee | ||
Comment 5•16 years ago
|
||
(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
Updated•16 years ago
|
Keywords: regression
Comment 6•16 years ago
|
||
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-
Assignee | ||
Comment 7•16 years ago
|
||
(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)
Assignee | ||
Updated•16 years ago
|
Summary: The title of Special Folder becomes (no title) → Choosing Properties on a Special Folder changes its title to (no title)
Comment 8•16 years ago
|
||
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+
Assignee | ||
Comment 9•16 years ago
|
||
(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.
Assignee | ||
Comment 10•16 years ago
|
||
addressed comments
Attachment #367755 -
Attachment is obsolete: true
Assignee | ||
Comment 11•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Assignee | ||
Updated•16 years ago
|
Attachment #368018 -
Flags: approval1.9.1?
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 368018 [details] [diff] [review]
patch v1.2
wanted, yay, broken root names beware!
risk is medium
Comment 14•16 years ago
|
||
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?
Assignee | ||
Comment 15•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
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+
Comment 18•16 years ago
|
||
(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?
Assignee | ||
Comment 19•16 years ago
|
||
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.
Assignee | ||
Comment 20•16 years ago
|
||
Keywords: fixed1.9.1
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 21•16 years ago
|
||
(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
Keywords: fixed1.9.1 → verified1.9.1
OS: Windows XP → All
Hardware: x86 → All
Comment 22•15 years ago
|
||
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.
Description
•