Code Review Request - Fix for #1917 - Allow non-selectable menuitems (seperators)

[John LaBanca]

Emily -

Please do a code review on this patch which adds a non-selectable MenuItemSeparators to the MenuBar.

Description:
It would be useful if you could add seperators to MenuBar's, a feature in most modern GUI's. You are able to add in items to a menubar that trigger a dummy command, but they are still selected.

Fix:
I created a new method in MenuBar called addSeparator, which adds an unselectable MenuItemSeparator to the MenuBar.  MenuItemSeparators can also be removed, just like normal MenuItems.

The following CSS is recommended:

.gwt-MenuBar-horizontal .gwt-MenuItem-separator {
width: 1px;
padding: 0px;
margin: 0px;
border: none;
border-left: 1px solid #888888;
background: white;
}

.gwt-MenuBar-horizontal .gwt-MenuItem-separator div {
width: 1px;
background: white;
}

.gwt-MenuBar-vertical .gwt-MenuItem-separator {
padding: 0px;
border: 0px;
}

.gwt-MenuBar-vertical .gwt-MenuItem-separator div {
height: 1px;
padding: 0px;
border: 0px;
border-top: 1px solid #777777;
background: #ddddee;
}

Testing:
I tested this manually on all browsers to very the separator works and can be made to look the same across all browsers.  I also wrote a unit test to verify that separators can be added and removed without interfering with the MenuItems.


Found by: charlesjbarrett
Fixed by: jlabanca

--
Thanks,
John LaBanca
jlab...@google.com

[Emily Crutcher]

Nice new feature! A few comments below:

     
1) MenuItemSeparator is a gwt class, so the style name should be gwt-MenuItemSeparator

2) To allow uses to subclass MenuItemSeparator, rather then having the method addSeparator() which returns a separator, I'd rather have addSeparator(MenuItemSeparator separator);

 3) It seems slightly cleaner to have  only one list for items and separators rather than two. We'd have to code a getFirstItem() method if we combined the two lists. Would there be any other bad side effects?

4)  Why do MenuItemSeparators need an internal div?  Can we name the inner div something descriptive? 

5) Can you abstract the default style names into  private STYLENAME_DEFAULT fields?

--
"There are only 10 types of people in the world: Those who understand binary, and those who don't"

[John LaBanca]

1) MenuItemSeparator is a gwt class, so the style name should be gwt-MenuItemSeparator

You're right, I'll change this.

2) To allow uses to subclass MenuItemSeparator, rather then having the method addSeparator() which returns a separator, I'd rather have addSeparator(MenuItemSeparator separator);

Agreed.  Should we also have a no-arg addMenuItemSeparator() that uses the default separator?  This is inline with the addMenuItem() methods that can take a MenuItem or construct one from a String.

 3) It seems slightly cleaner to have  only one list for items and separators rather than two. We'd have to code a getFirstItem() method if we combined the two lists. Would there be any other bad side effects?

The biggest problem is that their is a protected method called "getItems()" that returns a List of MenuItems.  If anyone overrides MenuBar, we would need to create a new list that does not include the MenuItemSeparators.  The other problem is that when the user hovers an item, we would need a runtime class type check to verify that the hovered element belongs to a MenuItem, not a MenuItemSeparator.  We would also need a lot of type casting, since the combined array is of the type "UIObject", not "MenuItem".  The parallel arrays take a little extra space, but I think they save a lot of runtime checking.

4)  Why do MenuItemSeparators need an internal div?  Can we name the inner div something descriptive? 

You need to put something in the MenuItemSeparator or the td will not show up on some browsers.  If you put a "<br>" or "&nbsp;", the cell will expand to fit its contents and cannot be set to 1px through CSS.  Even using the "empty-cells" style attribute, setting the td height or width to 1px has different meanings in some browsers.  It looks great in Firefox and Safari, but IE includes the 1px in the border, which I also set to create an illusion of a beveled edge.  Using the div works consistantly across all browsers.

5) Can you abstract the default style names into  private STYLENAME_DEFAULT fields?

Yes, I'll abstract it to a private field.


Let me know if you agree with my responses and I'll send you another patch.

[Emily Crutcher]

 Cool! Answers in lined below.

2) To allow uses to subclass MenuItemSeparator, rather then having the method addSeparator() which returns a separator, I'd rather have addSeparator(MenuItemSeparator separator);

Agreed.  Should we also have a no-arg addMenuItemSeparator() that uses the default separator?  This is inline with the addMenuItem() methods that can take a MenuItem or construct one from a String.

Seems reasonable to me.
 

 3) It seems slightly cleaner to have  only one list for items and separators rather than two. We'd have to code a getFirstItem() method if we combined the two lists. Would there be any other bad side effects?

The biggest problem is that their is a protected method called "getItems()" that returns a List of MenuItems.  If anyone overrides MenuBar, we would need to create a new list that does not include the MenuItemSeparators.  The other problem is that when the user hovers an item, we would need a runtime class type check to verify that the hovered element belongs to a MenuItem, not a MenuItemSeparator.  We would also need a lot of type casting, since the combined array is of the type "UIObject", not "MenuItem".  The parallel arrays take a little extra space, but I think they save a lot of runtime checking.

Sounds like a good argument.
 

4)  Why do MenuItemSeparators need an internal div?  Can we name the inner div something descriptive? 

You need to put something in the MenuItemSeparator or the td will not show up on some browsers.  If you put a "<br>" or "&nbsp;", the cell will expand to fit its contents and cannot be set to 1px through CSS.  Even using the "empty-cells" style attribute, setting the td height or width to 1px has different meanings in some browsers.  It looks great in Firefox and Safari, but IE includes the 1px in the border, which I also set to create an illusion of a beveled edge.  Using the div works consistantly across all browsers.

OK, why don't we name it something like "inner-space" or "content"  to make it slightly clearer what's going on in the style sheet.  (It;s a "special" element, not just a random div)

[John LaBanca]

Attached is an updated patch with all of the changes.

[Emily Crutcher]

Nice job!

There is one more preexisting bug that you might want to fix while you are already in this code base: clearItems does not set the items parents to null.

Also, just to follow protocol, can you send around an API  enhancement email? I don't anticipate any problems, but it's usually best to be thorough.

        Cheers,

                Emily

[John LaBanca]

Here is the patch with the clearItems fix included.  The funny thing is that I made the fix for the removeItems method, and completely missed it in the clearItems method.  Thanks for pointing it out!

[John LaBanca]

Oops, sorry to be a pain!  This patch also includes a little unit test to verify that the clearItems method actually removes the parent of the MenuItems and MenuItemSeparators.

[John LaBanca]

Emily -

Here is the updated patch for the MenuItemSeparators.  This patch also includes the following minor fixes/enhancements per our dicussions:
- clearItems clears the parent MenuBar of MenuItems
- addItem(MenuItem) returns the MenuItem
- addSeparator(MenuItemSeparator) returns the MenuItemSeparator


For those reading this thread, here is the default styling to go with the separators.  These styles are required, or the separator will not show up correctly:

.gwt-MenuBar-horizontal .gwt-MenuItemSeparator {

  width: 1px;
  padding: 0px;
  margin: 0px;

  border: 0px;

  border-left: 1px solid #888888;
  background: white;
}

.gwt-MenuBar-horizontal .gwt-MenuItemSeparator .content {
  width: 1px;
  background: white;
}

.gwt-MenuBar-vertical .gwt-MenuItemSeparator {
  padding: 0px;
  border: 0px;
}

.gwt-MenuBar-vertical .gwt-MenuItemSeparator .content {

  height: 1px;
  padding: 0px;
  border: 0px;
  border-top: 1px solid #777777;
  background: #ddddee;

  overflow: hidden;
}

On Jan 15, 2008 12:18 PM, Emily Crutcher <e...@google.com> wrote:

Don't forget to send out the API enhancement method...