Created attachment 2474[details]
Submenus with default choice improved
Proposed patch improve the new behavior of the Openbox menus.
One of the items of a submenu may be marked as default item by placing the "*" character as a first character of a item label.
If a marked item present in a submenu, then corresponding line of the parent menu is marked in the special manner - the triangle inside of the square, as in the OS/2. Left button click or the "Enter" key pressing executes the default item of the submenu immediately, without the additional keyboard or mouse actions with the submenu. The right button and the "Right" key hold their old behavior (expand and select the submenu). Left button click inside the square expand a submenu also.
Image of the mark of a submenu whith default item is theme-configurable, filename is "bullet_def.xbm" (this is a new theme file).
This patch based on the Dana's work branch, http://git.icculus.org/?p=dana/openbox.git;a=commit;h=51f4ec2c1d7e19ab4a9a701a2aa35b1c5b96cf27.
A new version of this patch contains the following changes:
1. The default item is marked in a expanded submenu by the same bullet as a menu item mark, but placed at the left of the bar. If one or more items have icons, the icons and bullet are located in separate columns (bullet first, icons second).
2. The default item should be marked in the menu.xml by new item property "default", f.e. <item default="true" label="my-label">. If the menu has item with this property, then its row in the parent menu will be marked with the symbol of a submenu with default.
3. New patch based on Dana's Master branch 3.5.0 (http://git.icculus.org/?p=dana/openbox.git;a=snapshot;h=98b02c6b60bbde2a5db026b3ee3e6e6dc44d1a92)
(In reply to comment #3)
DJ> Lots of comments, mostly formatting.
Thank You for Your comments. I followed most of your advice, but I also have some problems, see below lines marked by (!).
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/obrender/theme.h_sec1
DJ> - menu_bullet_def_mask needs to be freed.
Fixed
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/event.c_sec1
DJ> - some indent problem with the last }
Fixed
DJ> - I'd prefer consistent behaviour for the entire menu entry. You can
DJ> right-click to open the submenu without executing it, or hover. Just
DJ> make a left click always execute please. - can do this without the
DJ> local variable def_entry in 2 lines instead of 4.
Fixed.
But I'm not sure it is good. 1) The previous behavior was inherited from the OS/2, so I borrowed the idea of a menu whith default. 2) Code with local variable seems to me more readable, as it clearly shows that the tested and assigned values are the same.
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/event.c_sec3
DJ> - what if you have a submenu that has no default?
DJ> menu_frame_select_next(frame->child) should still be executed.
DJ> - frame->press_doexec = TRUE; was only set true if (frame->selected)
DJ> was true
I did not understand what was going on. The Enter key will open the submenu, and choose his first item. Exactly as without my patch.
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/event.c_sec5
DJ> - ending } in the wrong spot
Fixed
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menu.c_sec3
DJ> - if multiple defaults are there i'd like it to pick the first. so
DJ> check if default_entry is set also.
Fixed. Perhaps there ought to show a warning?
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menu.c_sec7
DJ> - whitespace change intentional?
Fixed
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec2
DJ> - why?
Fixed
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec3
DJ> - no spaces between ((s
DJ> - don't use () around a single check (a == b)
Fixed. However, in my opinion readability has deteriorated.
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec5
DJ> - don't define a new variable in the middle of a block, has to be at
DJ> the top.
Fixed.
DJ> - comment on an empty line
(!) Not fixed.
This is not an empty line, this is the opening bracket of the block and comment applies to the entire block.
DJ> - 552: don't line break here. why move the && ?
(!) Not fixed.
I agree with the formatting rules adopted in openbox. But I do not know them. They are described somewhere?
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec9
DJ> - indenting
Fixed.
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec11
DJ> - can fit on one line
(!) Not fixed.
I did not understand what you mean
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec5
DJ> - The bullet in the submenu denoted the default entry is not
DJ> vertically centered.
Fixed.
DJ> - it should have a width and height here both = ITEM_HEIGHT-2*PADDING.
DJ> we want it based on the size of the menu not the xbm.
Why? Why spend too much space, if the bullet is small? But if You want...
Fixed.
DJ> - it's y position should be PADDING. like the bullet for submenus.
Fixed.
DJ> - the x position should PADDING.
Fixed.
DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
DJ> nbox.git/openbox/menuframe.c_sec13
DJ> - use ITEM_HEIGHT-2*PADDING instead of def_w here.
Fixed.
In addition, I performed a small optimization: the field "has_default_entry" deleted. In fact, it always has the same value as (default_entry != NULL).
I build two patches
Thanks, replies below.
(In reply to comment #5)
> (In reply to comment #3)
> DJ> Lots of comments, mostly formatting.
> Thank You for Your comments. I followed most of your advice, but I also
> have some problems, see below lines marked by (!).
>
> DJ> - if multiple defaults are there i'd like it to pick the first. so
> DJ> check if default_entry is set also.
> Fixed. Perhaps there ought to show a warning?
I'm okay with it not.
> DJ> - don't define a new variable in the middle of a block, has to be at
> DJ> the top.
> Fixed.
> DJ> - comment on an empty line
> (!) Not fixed.
> This is not an empty line, this is the opening bracket of the block and
> comment applies to the entire block.
I meant to move the comment onto an empty line below the brace. Putting a comment after a brace is something we've never done in openbox code.
> DJ> - 552: don't line break here. why move the && ?
> (!) Not fixed.
> I agree with the formatting rules adopted in openbox. But I do not know
> them. They are described somewhere?
There used to be a bit of a style guide in git somewhere but this was long ago and it has since disappeared.. You changed the if statement's formatting but not its content which is bad in general, but when splitting an if across lines, we put some logical set of tests on the first line, never just "(\n". Also we put "&&" and "||" on the end of lines not the beginning. You should see this from other code. You can just leave the if statement on 527 (left side) untouched and add your new code above it.
> DJ> https://bugzilla.icculus.org/attachment.cgi?id=2971&action=diff#../ope
> DJ> nbox.git/openbox/menuframe.c_sec11
>
> DJ> - can fit on one line
> (!) Not fixed.
> I did not understand what you mean
You split line 795 (right side) across multiple lines but its less than 80 characters of stuff.
(In reply to comment #4)
> if you can stick the patch in a github fork and link to it here rather than
> attaching it, that would make reviewing it sooo much easier.
I try to do it, but it will take time. I've never worked with git, only with diff-patch as such.
Created attachment 3000[details]
Submenus with default choice - version 4
(In reply to comment #7)
All done.
About the condition in menuframe.c:555 - in fact now require only test "self->entry->icon"
Created attachment 2971 [details] Submenus with default choice - version 2 This patch must bi aplied instead of it's first versionCreated attachment 3407 [details] compatibility with current sources restoredCreated attachment 3523 [details] menu_w_def8.patch adapted to the current state of the "master" branch