207779 - Bookmark Manager View->Show Columns menu does nothing.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Rick Alther]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529

Open the Bookmark Manager.  Use the View->Show Columns menu to show/hide columns
to view.  Checking/unchecking columns from the menu has no affect at all.  The
menu is initialized correctly (i.e. the visible columns are correctly checked),
but it doesn't look like the code is hooked up when checking/unchecking columns.

Also, you can not uncheck the Name column from the menu either even though it is
enabled.  Since you can not hide the Name column, this menu option should be
shown in a disabled state.  The container control button for selecting columns
correctly disables the Name column.  

If you show/hide columns using the button on the container control, it works fine.

Reproducible: Always

Steps to Reproduce:
1. Open the Bookmark Manager
2. View->Show Columns menu.  Notice this submenu is correctly initialized.
3. check/uncheck various columns.  Notice this action has no effect on the
columns visible in the container.  Also note that you can never uncheck the Name
column even though the menu option is enabled.
4. Use the container control column select button.  Checking/unchecking columns
correctly changes the column visibility.  Also note that the Name column is
disabled properly giving the user feedback that they can not change the
visibility of the Name column.  

Actual Results:  
The columns never show/hide as needed.

Expected Results:  
That the columns show/hide as appropriate.  I also expect the Name menu option
be disabled since you can never hide that column.

Comment 1

[Chris Petersen]

Confirming issue in the 2003-06-02-08 Macho and Win32 trunk builds.

Comment 2

[Jessica Blanco]

This also happens on OS/2 - mozilla 1.4 release build

Comment 3

[Jessica Blanco]

Attachment: patch that disables Name and puts "sort" attribute back in

bookmarksManager.js is using the resource attribute from the columns to
show/hide them.  This attribute was set in bookmarksTree.xml and each column
had a "sort" attribute, but this was removed when the patch for bug 205378 got
checked in (big bookmarks change).  In this patch I set the Name option in the
menu to be disabled and added the sort attribute for each column back into
bookmarksTree.xml because it's still used to show/hide columns. Why was the
sort attribute removed?

Comment 4

[Jessica Blanco]

Attachment: previous patch - wrong approach

OK, adding the sort attribute back in does affect the sorting of bookmarks. 
Instead of adding the "resource" attribute back into bookmarksTree.xml, I
removed it from bookmarksManager.js.  I'm now using the label attribute to
show/hide columns, added the "id" attribute to use instead of resource/"sort"
because that was removed and no longer used, and actually checking for Name and
disabling it in the Show Columns menu.

Comment 5

[Jan Varga [:janv]]

The last patch looks good, although it can be improved a bit

1.
I'm worried that now you end up with 2 elements sharing the same id.
So it would be probably better to use other attribute on the menuitems, e.g. "ref"

2.
I think, toggleColumnVisibility() can now take the id attribute instead of
resource/label

Let me know if there are any issues with that.

Comment 6

[Jan Varga [:janv]]

I found out that these tree column ids are actually anonymous, anyway it would
be better to use "ref" attribute on menuitems.

Comment 7

[Jessica Blanco]

Jan, did you mean change the id to ref here -
-      menuitem.setAttribute("id", "columnMenuItem:" + columns[i].resource);
+      menuitem.setAttribute("id", "columnMenuItem:" + columns[i].id);
where the "id" would now be "ref" because menuitems already have a given id?

or did you mean change it here -
+                id: child.getAttribute("id"),
and use a "ref" for the id instead?

I'll update toggleColumnVisibility() to take the id attribute.

Comment 8

[Jan Varga [:janv]]

I meant:
-      menuitem.setAttribute("id", "columnMenuItem:" + columns[i].resource);
+      menuitem.setAttribute("ref", "columnMenuItem:" + columns[i].id);

...

-  var resource = aEvent.target.getAttribute("resource");
-  if (resource != "") {
+  var ref = aEvent.target.getAttribute("ref");
+  if (ref != "") {
     var bookmarksView = document.getElementById("bookmarks-view");
-    bookmarksView.toggleColumnVisibility(resource);
+    bookmarksView.toggleColumnVisibility(ref);

Comment 9

[Jessica Blanco]

Attachment: updated patch

Thanks Jan.  I used "ref" instead of "id" for the menuitem attribute, but I
kept the use of label instead of ref in toggleColumnVisibility() because
"columnMenuItem:" gets in the way.  When I remove the columnMenuItem string
from the id, the checkmarks do not show in the right places.

Comment 10

[Jessica Blanco]

Attachment: more up to date patch

Comment 11

[Jan Varga [:janv]]

Comment on attachment 128180 [details] [diff] [review]
more up to date patch

r=varga

Comment 12

[Mike Kaply [:mkaply]]

Comment on attachment 128180 [details] [diff] [review]
more up to date patch

a=mkaply

Comment 13

[Mike Kaply [:mkaply]]

Fix checked in to branch and trunk