Fl_Menu_ and Fl_Menu_Item::pulldown incremental overhaul?

[melcher....@googlemail.com]

Hi Devs,

tl;dr : I would like to overhaul the menubar code, and I would like to do it incrementally, so we can find regressions. I want to use the master branch for that, but I would like to hear you first, since that may add work.

Ok, so if you ever looked into the menu bar code (src/Fl_Menu.cxx), you know that it is really hard to maintain. It also has some shortcomings. For example, all menu items in a single menu must have the same height, which is why the divider always looks like it was squeezed in as an afterthought.

I implemented code that fixes a lot of these issues when trying to add better dividers. The code is tested and works, but if I alone test things, I may miss major regressions. I would like to push my code to master when I reach a milestone, so that I am not the only one testing. Of course, every milestone would be thoroughly tested in my branch before using it.

A bit of history:

The menubar code was probably written before the actual FLTK core became capable enough to do what the pulldown code implements in a different, undocumented, and badly named way. I would like to fix that while keeping the old API intact and adding a new way to add menus that fits the FLTK hierarchical system.

Milestone 1, flexible menu item sizes:

To implement this, I create Proxy widgets for every item that may appear in a pulldown menu. When a menu window is opened, a true FLTK widget hierarchy is created on the fly using the proxies. With proxies, every menu item now has coordinates and sizes, and we can finally have dividers that don't just draw a line, but also visually divide groups. We can no easily have the horizontal stacking, as FL_MENU_HORIZONTAL always promised, but never implemented.

Milestone 2, reducing special event handling and drawing:

Now, instead of having a three-page long loop, the menu window and proxy widgets handle events as FLTK does anyway. We can make the drawing code a lot simpler with the same results.

Milestone 3, alternative to Fl_Menu_Item arrays:

And by deriving Fl_Menu_ from Fl_Group instead of Fl_Widget, we can completely do away with the entire FL_Menu_Item array special handling. The user just adds buttons and groups to the Fl_Menu_ driver widget, and the menu window code will take care of generating the proxies and laying them out. This solves also all i18n woes, questions of ownership, and inserting menu items into arrays.

 - Matthias

[Greg Ercolano]

On 11/20/22 01:34, 'melcher....@googlemail.com' via fltk.coredev wrote:

Hi Devs,

tl;dr : I would like to overhaul the menubar code, and I would like to do it incrementally, so we can find regressions. I want to use the master branch for that, but I would like to hear you first, since that may add work.

    Cool! Sounds a little scary though, lol. But maybe that's needed.

    We seem to keep holding back 1.4.0 release, so I'm thinking we'd need
    to keep the old code the default, but could maybe make your code a
    build option that can be turned on to test, and later it could become
    the default?

    I know that old application code might be using perhaps "unsavory"
    ways of accessing the internals of Fl_Menu's data to work around
    API limitations, which a code cleanup might break.

    Or, should the rewrite have a new API, e.g. Fl_Menubar_V2, Fl_Menu_V2
    so you can go bananas on the API, without having to worry about breaking
    old code (since the old Fl_Menubar / Fl_Menu API would remain "forever")

[Albrecht Schlosser]

On 11/20/22 12:23 Greg Ercolano wrote:
>
> On 11/20/22 01:34, 'melcher....@googlemail.com' via fltk.coredev wrote:
>
>> Hi Devs,
>>
>> tl;dr : I would like to overhaul the menubar code, and I would like
>> to do it incrementally, so we can find regressions. I want to use the
>> master branch for that, but I would like to hear you first, since
>> that may add work.
>
>     Cool! Sounds a little scary though, lol. But maybe that's needed.

Yes, I agree to all three points: cool, scary, and needed.

>     We seem to keep holding back 1.4.0 release, so I'm thinking we'd need
>     to keep the old code the default, but could maybe make your code a
>     build option that can be turned on to test, and later it could become
>     the default?

I'm really afraid that such massive changes would have unforeseeable
effects and might break user code. Once we push them to master it would
be very hard to turn this back.

Hence I would suggest to use a branch, probably in a (i.e. Matthias')
fork for first testing. I would volunteer to test and once we (me and
other volunteers) see the code and find it "good enough" (i.e. unlikely
to break existing code) we can still decide whether to merge it in
master or not.

>     I know that old application code might be using perhaps "unsavory"
>     ways of accessing the internals of Fl_Menu's data to work around
>     API limitations, which a code cleanup might break.

Yes, I'm also afraid of such effects. Maybe manipulations in
Fl_Menu_Item arrays etc. that we don't expect.

> Or, should the rewrite have a new API, e.g. Fl_Menubar_V2, Fl_Menu_V2
>     so you can go bananas on the API, without having to worry about
> breaking
>     old code (since the old Fl_Menubar / Fl_Menu API would remain
> "forever")

That's indeed an option but in the end we'd need to *replace* the old
menu code with the new code for one of the next releases. Keeping that
old code "forever" would be a problem I'd like to avoid.

I think in a later phase a build option that would *replace* the old
menu code with the new code so users can test their own applications w/o
rewriting anything would be desirable.

I think we can't decide what to do (incrementally integrate in master or
not) before we see the code. What I definitely don't want is "trying"
something (in master) that doesn't work and needs to be reverted or is
perhaps released at some time and breaks user code.

That said, this is my statement for now, w/o seeing the code, just from
a "FLTK management" point of view. I have some more questions to the
technical details mentioned by Matthias which I will ask in a separate
message.

[Albrecht Schlosser]

On 11/20/22 10:34 'melcher...' via fltk.coredev wrote:
> tl;dr : I would like to overhaul the menubar code, and I would like to
> do it incrementally, so we can find regressions. I want to use the
> master branch for that, but I would like to hear you first, since that
> may add work.

I replied regarding "master branch or not" in another message. Here I'm
going to ask for some details.

> Ok, so if you ever looked into the menu bar code (src/Fl_Menu.cxx),
> you know that it is really hard to maintain. It also has some
> shortcomings. For example, all menu items in a single menu must have
> the same height, which is why the divider always looks like it was
> squeezed in as an afterthought.
>
> I implemented code that fixes a lot of these issues when trying to add
> better dividers. The code is tested and works, but if I alone test
> things, I may miss major regressions.

Can we see this code somewhere? Do (did) you push it to your fork?

> I would like to push my code to master when I reach a milestone, so
> that I am not the only one testing. Of course, every milestone would
> be thoroughly tested in my branch before using it.

See my other reply.

> A bit of history:
>
> The menubar code was probably written before the actual FLTK core
> became capable enough to do what the pulldown code implements in a
> different, undocumented, and badly named way. I would like to fix that
> while keeping the old API intact and adding a new way to add menus
> that fits the FLTK hierarchical system.

Great idea. I would appreciate this very much if it could be done w/o
changing the API. However, this looks like a big task, and I wonder if
it wouldn't be easier and cleaner to rewrite a new menu system from
scratch. This way only users opting in to the new menu system would be
affected.

> Milestone 1, flexible menu item sizes:
>
> To implement this, I create Proxy widgets for every item that may
> appear in a pulldown menu. When a menu window is opened, a true FLTK
> widget hierarchy is created on the fly using the proxies.

Sounds interesting but also complicated. Maybe rewriting this from
scratch ... ;-)

> With proxies, every menu item now has coordinates and sizes, and we
> can finally have dividers that don't just draw a line, but also
> visually divide groups. We can no easily have the horizontal stacking,
> as FL_MENU_HORIZONTAL always promised, but never implemented.

Sounds good, although I don't think that horizontal menus are really
worth considering. Is there anybody that *wants* such menus, or would it
just be "because it can be done"?

> Milestone 2, reducing special event handling and drawing:
>
> Now, instead of having a three-page long loop, the menu window and
> proxy widgets handle events as FLTK does anyway. We can make the
> drawing code a lot simpler with the same results.

That would certainly be the most valuable part and we should try to do
this. Again, rewriting from scratch ... Well, doing this with "original"
menu widgets rather than Fl_Menu_Item arrays and proxies sound much less
complicated and in the end much easier to maintain.

> Milestone 3, alternative to Fl_Menu_Item arrays:
>
> And by deriving Fl_Menu_ from Fl_Group instead of Fl_Widget, we can
> completely do away with the entire FL_Menu_Item array special
> handling. The user just adds buttons and groups to the Fl_Menu_ driver
> widget,

These "buttons and groups" would be Fl_Button and Fl_Group widgets, correct?

> and the menu window code will take care of generating the proxies and
> laying them out.

If the answer to my question above is true, why would we still need
proxies? Can't we just use the new menu widgets?

> This solves also all i18n woes, questions of ownership, and inserting
> menu items into arrays.

That would be very much appreciated, and I assume, fluid support would
also be done, right?


After all I believe that a full rewrite from scratch [1] would be the
way to go. It would be as easy as it could be: much easier than
integrating the new code into the old, complicated code which would
likely make it even more complicated. Knowing what the goals are we
(you) could use today's FLTK techniques. It would also be fully optional
for users, it would not affect existing (menu) code, and it could be
integrated in git 'master' as soon as "it works", even if we need to
improve it later and fix bugs. As long as it is optional for users and
documented to be in "beta stadium" it doesn't do any harm.

Looking forward to further discussions and other opinions.


[1] IMHO it's similar to the decision whether the new Fl_Flex widget
should be integrated in Fl_Pack or not. For me the decision was obvious:
write a new - hopefully better - widget and forget the old one.

[Albrecht Schlosser]

On 11/20/22 16:07 Albrecht Schlosser wrote:
> After all I believe that a full rewrite from scratch ... would be as
> easy as it could be ...

One more thought: the new menu system would give us the chance to
redesign the menu display strategy, similar to what current browsers
seem to do, maybe related to necessary Wayland changes since Wayland has
limited user control about popup (menu) windows.

It looks like recent Firefox opens one "main" menu window and displays
submenus by "shifting" the main menu out of the menu window, making room
for the submenu - rather than opening a second (and third and fourth
...) window. Example: click the "hamburger menu", then on "Help" or
"More tools". This is true even for the X11 version of Firefox [1].

I'm not sure if this is the *best* way to deal with (user) menus but
there's at least a chance for a menu redesign that is more Wayland
compatible than our current menu system. Particularly very long menus
are problematic and could be improved by a new menu system.


[1] FYI: You can start a new instance of firefox as a Wayland process if
you set an environment variable and use another profile than already
running firefox processes, for instance:

$ MOZ_ENABLE_WAYLAND=1 firefox -P

Commandline switch -P runs the firefox profile manager so you can either
use an existing profile or create a new one if necessary.

[Lauri Kasanen]

On Sun, 20 Nov 2022 01:34:00 -0800 (PST)
"'melcher....@googlemail.com' via fltk.coredev"

<fltkc...@googlegroups.com> wrote:

> Milestone 2, reducing special event handling and drawing:
>
> Now, instead of having a three-page long loop, the menu window and proxy
> widgets handle events as FLTK does anyway. We can make the drawing code a
> lot simpler with the same results.

What would this do to efficiency? Say a menu with 30 items. If each
item now makes its own drawcalls, that'd be 30 calls to draw small
pieces of backgrounds vs current 1 for the whole menu, no? (the text
drawcalls were already per-item, I think).

- Lauri

[Manolo]

I agree with the objectives of this proposal, and the recommendation to develop it as new code

in addition to the present Fl_Menu classes.

Two remarks :

1) Release of FLTK 1.4 is long due. I would hope this proposal would not delay it further.

2) Class Fl _Sys_Menu_Bar is to be taken in consideration too.

[melcher....@googlemail.com]

Oh wow, thanks for the numerous answers!

> Cool! Sounds a little scary though, lol. But maybe that's needed.

LOL, it's actually OK once you get the hang of it.

> We seem to keep holding back 1.4.0 release

This probably worth a new thread. I'll be happy to do this for 1.5.0, or 1.4.1 in a minor ABI safe version.

> maybe make your code a build option

If users don't choose the option, a branch is probably better. What I propose leaves the old system intact, but it should not make the library much bigger, because the old and new implementation reuse more core code than the current implementation.

> I know that old application code might be using perhaps "unsavory" ways of accessing the internals

That's always possible. I assume that the Menu Item array does not change when a menu is up, but so does the current code. And when the menu pops up the next time, the proxy stuff is rebuilt, reflecting all potential changes.

> Or, should the rewrite have a new API, e.g. Fl_Menubar_V2, Fl_Menu_V2

Not a fan. Both APIs can coexist without causing confusion. 

> I'm really afraid that such massive changes would have unforeseeable effects and might break user code. Once we push them to master it would  be very hard to turn this back. 

That's the main reason for me asking. If it does break user code, at least we will know quickly. If we have a branch and merge in the final 1.4.0, we may mess up a major release which absolutely do not want.

> What I definitely don't want is "trying"  something (in master) that doesn't work and needs to be reverted

Yes, me neither. I was planning on keeping a branch, but once that is tested, I was thinking that milestones are better than one huge change of the core.

> Can we see this code somewhere? Do (did) you push it to your fork? 

Sure: https://github.com/fltk/fltk/compare/master...MatthiasWM:fltk:menu_divider . This removes the limitation that all menu items must be the same height and adds support for a divider item, a menu item without label, but with the divider flag set. All dividers have more space around them. I will add screenshots later.

> I wonder if it wouldn't be easier and cleaner to rewrite a new menu system from scratch

We would have two code blocks that do essentially the same. So improvements to one system would not benefit the other. Just having better dividers in existing apps is worth the effort to me.

> I don't think that horizontal menus are really worth considering.

Yeah, it's at best a "nice to have".

> doing this with "original" menu widgets rather than proxies sound much less complicated and in the end much easier to maintain.

I was back and forth about that. By using proxies, users can just add the already familiar  Fl_Group, Fl_Button, Fl_Radio_Button, and Fl_Check_Button, and none of the internals spills into  the core code (no new types, no overriding ::draw() and ::handle() if we are inside a menubar). I would add Fl_Divider though.

> fluid support would also be done, right? 

Certainly.

> After all I believe that a full rewrite from scratch [1] would be the way to go. 

Hmm, thanks for all the input everyone. This is a really great help. Maybe it is enough for now if we derive Fl_Menu_ from Fl_Group instead of Fl_Widget. This would give me enough hooks to add things after 1.4.0 without changing the ABI (I think). 

Screenshots in the next mail.

[melcher....@googlemail.com]

> What would this do to efficiency? If each item now makes its own drawcalls, that'd be 30 calls to draw small pieces of backgrounds vs current 1 for the whole menu, no? 

Fl_Menu.cxx is duplicating functionality that is already in the core. fl_damage and ::redraw() do the same stuff that the huge loop does. 

> Class Fl _Sys_Menu_Bar is to be taken in consideration too.

Yes, I am developing on macOS, so I will not forget ;-)

> Release of FLTK 1.4 is long due. I would hope this proposal would not delay it further.

Yes, agreed. I will limit my request to making Fl_Menu_ derive from Fl_Group. Does anyone see and issue with that?

[melcher....@googlemail.com]

Example 1: in the left version, menu items can have arbitrary height. The dividers have more top and bottom margin than on the right, where the divider is just squeezed between two items.

Example 2: on the left, the font size of every item determines the height of the menu item. On the right, all items must be the same height.

[Albrecht Schlosser]

On 11/20/22 19:12 'melcher....@googlemail.com' via fltk.coredev wrote:
> ... I will limit my request to making Fl_Menu_ derive from Fl_Group.

> Does anyone see and issue with that?

IMHO it's always a problem if you change the base class of "something"
(in this case Fl_Menu_). Users deriving their own classes from any menu
subclasses could (at least) be surprised.

I'm not sure if it would do any harm though, it's just my gut feeling
that it *could* be an issue. Here's a theory: let's assume a user
derived a subclass from Fl_Menu_ and now this is no longer a subclass of
Fl_Widget but of Fl_Group. In this case the user class inherits a bunch
of *new* methods which Fl_Group brings with it. Hence there are at least
potential method name conflicts etc. etc..

[Lauri Kasanen]

On Sun, 20 Nov 2022 10:26:28 -0800 (PST)

"'melcher....@googlemail.com' via fltk.coredev"
<fltkc...@googlegroups.com> wrote:

>

I actually prefer the old picture in both cases. In particular with the
six font sizes, it's much better for usability to make it easier to hit
the tiny one. So please keep it possible with a setting?

- Lauri

[imacarthur]

I'm late to this one, but here goes anyway...

I'm "uncomfortable" (as others have been) about dropping this in as a replacement for the existing menu system - I'm pretty sure that would cause more pain, at least in the short term. In particular, changing the "base" widget to Fl_Group rather than Fl_Widget seems like it might cause "surprises" out in the wild... and there might be ABI issues with that? (Though that's maybe less of a worry for 1.4, which has never been "released"...)

That being said, a new "widget style" menu system might be worth a shot and should bring advantages - this is the way fltk2 went, I think? 

So: If we're picking names, I like the sound of Fl_Widget_Menu_* for this...

Beyond that, Lauri flags a few things: 

- I'm not sure that drawing performance will be that much of a setback - IIRC this sort of approach worked fine on fltk2, and that would generally have been on less capable hardware than folks have nowadays. Nonetheless, something we might need to keep an eye on.

- Menu sizes; it may be useful to have the menu lines scalable, but the usability aspect might well come into play if the menu item is "too small" to be clickable, so we might need some min/max bounds for this and/or (as Lauri suggests) a means to inhibit the scaling. From a usability perspective, it is "normal" to make all the items the "same size" as that makes usage more consistent - "least surprise" and all that...