Updating Fl_Sys_Menu_Bar

[Martin McDonough]

I want to be able to make items in an Fl_Sys_Menu_Bar active or inactive at various times. I've currently been using Fl_Sys_Menu_Bar::menu to do this, since just setting it on the menu item does not seem to work.

But I've also tracked a few crashes to that call. I am reusing the same menu array, just with updated information in it. How is this supposed to be done with Fl_Sys_Menu_Bar?

[Manolo Gouy]

> Le 27 avr. 2015 à 01:43, Martin McDonough wrote:
> I want to be able to make items in an Fl_Sys_Menu_Bar active or inactive at various times. I've currently been using Fl_Sys_Menu_Bar::menu to do this, since just setting it on the menu item does not seem to work.

You can’t just change the menu item because the Mac menu bar won’t see it.
You should call Fl_Sys_Menu_Bar::mode(int rank, int flag)
where rank is the item rank and flag is the item flag set as follows

To inactivate do:
Fl_Sys_Menu_Bar *mymenu = ….
int flag = Fl_Sys_Menu_Bar::mode(rank);
flag |= FL_MENU_INACTIVE;
mymenu->mode(rank, flag);

and to activate:
int flag = Fl_Sys_Menu_Bar::mode(rank);
flag &= ~FL_MENU_INACTIVE;
mymenu->mode(rank, flag);

>
> But I've also tracked a few crashes to that call. I am reusing the same menu array, just with updated information in it. How is this supposed to be done with Fl_Sys_Menu_Bar?

Please, describe those crashes if they still occur after using the method given above.

[Manolo Gouy]

> Le 27 avr. 2015 à 09:58, Manolo Gouy wrote :
>
>
>> Le 27 avr. 2015 à 01:43, Martin McDonough wrote:
>> I want to be able to make items in an Fl_Sys_Menu_Bar active or inactive at various times. I've currently been using Fl_Sys_Menu_Bar::menu to do this, since just setting it on the menu item does not seem to work.
>
> You can’t just change the menu item because the Mac menu bar won’t see it.
> You should call Fl_Sys_Menu_Bar::mode(int rank, int flag)
> where rank is the item rank and flag is the item flag set as follows
>
> To inactivate do:
> Fl_Sys_Menu_Bar *mymenu = ….
> int flag = Fl_Sys_Menu_Bar::mode(rank);

typo here, this should be
int flag = mymenu->mode(rank);

> flag |= FL_MENU_INACTIVE;
> mymenu->mode(rank, flag);
>
> and to activate:
> int flag = Fl_Sys_Menu_Bar::mode(rank);

typo here too, this should be
int flag = mymenu->mode(rank);

[Greg Ercolano]

I just got bit by the issue of Fl_Sys_Menu_Bar checkbox items not changing
when adjusted using the usual find_item() and item->set()/item->clear().

This code works fine on Windows + Linux is not working on the mac
when Fl_Sys_Menu_Bar is used.

We really need to have the Fl_Sys_Menu_Bar docs indicate this issue,
and recommend the right way to do this that works for both Fl_Menu_Bar
and Fl_Sys_Menu_Bar.

The way the docs currently read, it gives the impression one need
only replace Fl_Menu_Bar with Fl_Sys_Menu_Bar. The docs only warn
about cosmetic things like font styling of the labels. It doesn't
mention how accessing Fl_Menu_Item methods directly doesn't work.

A few questions:

> What /is/ the most compatible way to set/clear menu item checkboxes
across Fl_Menu_Bar and Fl_Sys_Menu_Bar? From what I can tell, we should
maybe use Fl_Menu_Bar::mode() instead of find_item() and item->set()/clear()
If so, can we include some examples in the docs? I'm willing to write this,
just want to make sure that's the recommended / most consistent way.

> In the Fl_Sys_Menu_Bar docs, there are references to 'rank', e.g.

void replace (int rank, const char *name)
rename an item from the system menu bar

void Fl_Sys_Menu_Bar::remove(int rank)
remove an item from the system menu bar

I'm guessing 'rank' means "index"?
If so, I suggest we change the docs (and code) to refer to 'index'
for consistency,

> Are there no tricks we can do in e.g. item->set() and item->clear()
methods to have them work, e.g. detect the parent menu is an Fl_Sys_Menu_Bar,
and do whatever extra work is necessary to ensure these changes make it to the GUI?
It just seems weird they silently don't work.

> Is there maybe a way we can have an apply() or sync() method
in Fl_Sys_Menu_Bar (or some such named method) that can apply any changes
made to the menu()[] array so that they're reflected in the GUI?
This would make it easier than having to retool apps from using the item techniques.

> It might not be a bad idea to put some warnings in the Fl_Menu_Item methods
about which calls aren't compatible with Fl_Sys_Menu_Bar, or maybe \see also
references to Fl_Sys_Menu_Bar with the above recommended doc warnings.

This created some really embarrassing bugs in my gui apps that hundreds of
customers were encountering.

[Manolo]

On Friday, 17 June 2016 01:46:05 UTC+2, Greg Ercolano wrote:

I just got bit by the issue of Fl_Sys_Menu_Bar checkbox items not changing
when adjusted using the usual find_item() and item->set()/item->clear().

This code works fine on Windows + Linux is not working on the mac
when Fl_Sys_Menu_Bar is used.

We really need to have the Fl_Sys_Menu_Bar docs indicate this issue,
and recommend the right way to do this that works for both Fl_Menu_Bar
and Fl_Sys_Menu_Bar.

The way the docs currently read, it gives the impression one need
only replace Fl_Menu_Bar with Fl_Sys_Menu_Bar. The docs only warn
about cosmetic things like font styling of the labels. It doesn't
mention how accessing Fl_Menu_Item methods directly doesn't work.

A few questions:

        > What /is/ the most compatible way to set/clear menu item checkboxes
          across Fl_Menu_Bar and Fl_Sys_Menu_Bar? From what I can tell, we should
          maybe use Fl_Menu_Bar::mode() instead of find_item() and item->set()/clear()
          If so, can we include some examples in the docs? I'm willing to write this,
          just want to make sure that's the recommended / most consistent way.

The recommended way is, starting from a menu item :

int index = smenubar->find_index(item);
// to set:
smenubar->mode(index, smenubar->mode(index) | FL_MENU_VALUE);
// to clear:
smenubar->mode(index, smenubar->mode(index) & ~FL_MENU_VALUE);


 

        > In the Fl_Sys_Menu_Bar docs, there are references to 'rank', e.g.

                void         replace (int rank, const char *name)
                         rename an item from the system menu bar

                void Fl_Sys_Menu_Bar::remove(int rank)         
                        remove an item from the system menu bar

          I'm guessing 'rank' means "index"?
          If so, I suggest we change the docs (and code) to refer to 'index'
          for consistency,

Yes, please.
 

        > Are there no tricks we can do in e.g. item->set() and item->clear()
          methods to have them work, e.g. detect the parent menu is an Fl_Sys_Menu_Bar,
          and do whatever extra work is necessary to ensure these changes make it to the GUI?
          It just seems weird they silently don't work.

        > Is there maybe a way we can have an apply() or sync() method
          in Fl_Sys_Menu_Bar (or some such named method) that can apply any changes
          made to the menu()[] array so that they're reflected in the GUI?
          This would make it easier than having to retool apps from using the item techniques.

This method exists: Fl_Sys_Menu_Bar::update()
but, unfortunately it is protected.
I think we should make it public in 1.4. It can't be done in 1.3.4 because it would break the ABI.
With this, one could write:

item->set();    (or  item->clear();)
smenubar->update();

and have the item changed in the Mac menu.

With 1.3.x it is possible (but ugly) to fool the protection:

class fool: public Fl_Sys_Menu_Bar {
public:
  void update() {Fl_Sys_Menu_Bar::update();}
};

((fool*)smenubar)->update();
 

        > It might not be a bad idea to put some warnings in the Fl_Menu_Item methods
          about which calls aren't compatible with Fl_Sys_Menu_Bar, or maybe \see also
          references to Fl_Sys_Menu_Bar with the above recommended doc warnings.

My vision would be to first set the update() method public, and then to explain in
class Fl_Sys_Menu_Bar that all operations applied to menu items should be followed by
a call to the update() menu method to be visible in the interface.
 

[Greg Ercolano]

On 06/17/16 04:21, Manolo wrote:
> > What /is/ the most compatible way to set/clear menu item checkboxes
> across Fl_Menu_Bar and Fl_Sys_Menu_Bar?
>

> The recommended way is, starting from a menu item :
>
> int index = smenubar->find_index(item);
> // to set:
> smenubar->mode(index, smenubar->mode(index) | FL_MENU_VALUE);
> // to clear:
> smenubar->mode(index, smenubar->mode(index) & ~FL_MENU_VALUE);

Great! Will add some docs to that effect.

That's the fix I ended up using.. I'd never used mode() before,
or even noticed it in the API.

> I'm guessing 'rank' means "index"?
> If so, I suggest we change the docs (and code) to refer to 'index'
> for consistency,
>
> Yes, please.

Will do..

> > Is there maybe a way we can have an apply() or sync() method
> in Fl_Sys_Menu_Bar (or some such named method) that can apply any changes
> made to the menu()[] array so that they're reflected in the GUI?
> This would make it easier than having to retool apps from using the item techniques.
>
> This method exists: Fl_Sys_Menu_Bar::update()
> but, unfortunately it is protected.
> I think we should make it public in 1.4. It can't be done in 1.3.4 because it would break the ABI.

If a few others sign off on this idea of making it public,
maybe I can add some ABI guards to 1.3.4 so it can be accessed optionally.

And maybe you can add it as public to the 1.4 branch?
(I haven't been keeping an eye on the 1.4 branch, but if you don't have time,
I can check it out and make the mod)

> With this, one could write:
>
> item->set(); (or item->clear();)
> smenubar->update();

Right, that sounds useful, as I'm thinking that could be really useful
for old code that directly manipulates the items..

> With 1.3.x it is possible (but ugly) to fool the protection:
>
> class fool: public Fl_Sys_Menu_Bar {
> public:
> void update() {Fl_Sys_Menu_Bar::update();}
> };
>
> ((fool*)smenubar)->update();

Ah, interesting technique to force a method from protected -> public.

I kind of had the reverse problem during debugging; for me the mode()
technique wasn't working, until I realized I was passing the sys menubar
around as an Fl_Menu_Bar pointer instead, and was getting the wrong mode() method..!

> > It might not be a bad idea to put some warnings in the Fl_Menu_Item methods
> about which calls aren't compatible with Fl_Sys_Menu_Bar, or maybe \see also
> references to Fl_Sys_Menu_Bar with the above recommended doc warnings.
>
> My vision would be to first set the update() method public, and then to explain in
> class Fl_Sys_Menu_Bar that all operations applied to menu items should be followed by
> a call to the update() menu method to be visible in the interface.

Right, sounds reasonable.
I can add mention of this to the existing caveats for Fl_Sys_Menu_Bar.

Going to wait for one of the other devs to weigh in on all the above
before I make any changes; often Ian/Albrecht/Matt/etc come up with some
interesting insights..

[Albrecht Schlosser]

On 17.06.2016 19:20 Greg Ercolano wrote:

>> This method exists: Fl_Sys_Menu_Bar::update()
>> but, unfortunately it is protected.
>> I think we should make it public in 1.4. It can't be done in 1.3.4 because it would break the ABI.
>
> If a few others sign off on this idea of making it public,
> maybe I can add some ABI guards to 1.3.4 so it can be accessed optionally.

+1

> And maybe you can add it as public to the 1.4 branch?
> (I haven't been keeping an eye on the 1.4 branch, but if you don't have time,
> I can check it out and make the mod)

svn r11785 (by Manolo):

"Make Fl_Sys_Menu_Bar::update() public and cross-platform.

It was before protected and Mac OS-specific.
This allows to call it after direct modification of menu items.
"

I'm not sure what this means, i.e. if there are simple ways to do the
same for 1.3.4 with ABI guards. But if Manolo and you agree that it's
possible, I'm all for it.

Note: I don't know/use Fl_Sys_Menu_Bar myself, so I can't contribure
more than that to the discussion.

>> With this, one could write:
>>
>> item->set(); (or item->clear();)
>> smenubar->update();
>
> Right, that sounds useful, as I'm thinking that could be really useful
> for old code that directly manipulates the items..

Would need user code changes, but sounds simple enough.

>> My vision would be to first set the update() method public, and then to explain in
>> class Fl_Sys_Menu_Bar that all operations applied to menu items should be followed by
>> a call to the update() menu method to be visible in the interface.
>
> Right, sounds reasonable.
> I can add mention of this to the existing caveats for Fl_Sys_Menu_Bar.

+1

[Greg Ercolano]

On 06/17/16 11:45, Albrecht Schlosser wrote:
> On 17.06.2016 19:20 Greg Ercolano wrote:
>
>>> This method exists: Fl_Sys_Menu_Bar::update()
>>> but, unfortunately it is protected.
>>> I think we should make it public in 1.4. It can't be done in 1.3.4 because it would break the ABI.
>>
>> If a few others sign off on this idea of making it public,
>> maybe I can add some ABI guards to 1.3.4 so it can be accessed optionally.
>
> +1

Great -- I'll make an STR out of all of this
so the commits can refer to it.

[Manolo]

The update() method has been made public in the porting branch (a.k.a. 1.4)
and committed (r.11785). Besides making this method public rather than
protected, it also defines it for all platforms (it is Mac-specific in 1.3).

====================
1.3 code structure

#if __APPLE__
class Fl_Sys_Menu_Bar : public Fl_Menu_Bar {
protected:
  void update();
public:
  ...a bunch of methods also defined in Fl_Menu_Bar...
};
#else
typedef  Fl_Menu_Bar Fl_Sys_Menu_Bar;
#endif

==================
1.4 code structure:

#if __APPLE__
class Fl_Sys_Menu_Bar : public Fl_Menu_Bar {
public:
  void update();
  ...a bunch of methods also defined in Fl_Menu_Bar...
};
#else
class Fl_Sys_Menu_Bar : public Fl_Menu_Bar {
public:
  void update() {}
};
#endif
===========================

I believe the new structure is necessary so an Fl_Sys_Menu_Bar object
can be handled in source code in a cross-platform way
(and it involves calling object->update() sometimes).

If Greg agrees, he could commit that with ABI guards to the 1.3 branch.

[Greg Ercolano]

STR# 3317

[Greg Ercolano]

    Yes, I'll implement that with ABI guards on 1.3.

    One question though, and I imagine you may have considered it:

    Do you think there'd be any API benefit to implementing update()
    in Fl_Menu_Bar base class as an empty virtual? (Much like
    the draw() method in Fl_Widget)

    Then the typedef (in green above) wouldn't have to be replaced
    with the small class with the empty update() method; users calling
    update() on non-mac platforms would essentially get a stub the same way.

    Also, users with small libraries to manage menu bars
    could work with both Fl_Menu_Bar and Fl_Sys_Menu_Bar interchangeably.
    For instance, subroutines that clear a dynamic submenu and recreate it
    could do so for both classes without modification.
   

   

[Manolo]

On Wednesday, 22 June 2016 08:32:24 UTC+2, Greg Ercolano wrote:

    One question though, and I imagine you may have considered it:

    Do you think there'd be any API benefit to implementing update()
    in Fl_Menu_Bar base class as an empty virtual? (Much like
    the draw() method in Fl_Widget)

    Then the typedef (in green above) wouldn't have to be replaced
    with the small class with the empty update() method; users calling
    update() on non-mac platforms would essentially get a stub the same way.

    Also, users with small libraries to manage menu bars
    could work with both Fl_Menu_Bar and Fl_Sys_Menu_Bar interchangeably.
    For instance, subroutines that clear a dynamic submenu and recreate it
    could do so for both classes without modification.

This is an interesting proposition that goes towards allowing to work with an Fl_Menu_Bar pointer
that is an Fl_Sys_Menu_Bar on the Mac Platform. But for this to really work,
all Fl_Sys_Menu_Bar member functions should be made virtual, not update()
only, or the risk of using the bad implementation of a function is too high.
These other member functions are all from Fl_Menu_, so they shoud be made virtual there.
That would be possibly too many changes to be protected by an ABI guard.

My suggestion would be:
- for 1.3.4, the correct use is to work with an Fl_Sys_Menu_Bar object on all platforms,
when there's a chance such object is used on the Mac platform.

- for 1.4, define a bunch of virtual functions in Fl_Menu_ to implement a
variably located menu bar, possibly in other platforms.

[Greg Ercolano]

On 06/22/16 05:46, Manolo wrote:
> My suggestion would be:
> - for 1.3.4, the correct use is to work with an Fl_Sys_Menu_Bar object on all platforms,
> when there's a chance such object is used on the Mac platform.
>
> - for 1.4, define a bunch of virtual functions in Fl_Menu_ to implement a
> variably located menu bar, possibly in other platforms.

Yes, that sounds wise.

I did commit your mods to 1.3, and I'll probably be making some documentation
additions that includes example code for doing the common techniques, such as
setting/getting the value of menu checkboxes and radio buttons.