find_shortcut(): fix testing of multi-label alt shortcuts

[dannye]

This PR contains two bug fixes.

First: The original line of code (Fl_Widget::test_shortcut(m->text, require_alt)) unconditionally treated m->text as a char *.

This is invalid for "special" label types (_FL_MULTI_LABEL, _FL_ICON_LABEL, and _FL_IMAGE_LABEL) and would result in parsing a Fl_Multi_Label, Fl_File_Icon, or Fl_Image (etc) object as a char array, which has unpredictable results.

This was fixed by checking that the label type is not a special label type before calling test_shortcut(m->text, require_alt).

Second: If the label type is _FL_MULTI_LABEL, the text in labela and labelb were not being processed for alt shortcuts despite being drawn with the appropriate alt underlines. (This was the bug that I noticed that led me here.)

This was fixed by also calling test_shortcut() with labela and/or labelb (if the label is not a special label type).

For now, this is not done recursively (ie, if labela or labelb is itself a Fl_Multi_Label, we don't process the nested labels any further). I wanted to get opinions on the general fix first. But if a recursive solution would be best, I can do that.

Also feel free to let me know of any docs or tests you would like to see updated.

P.S. This raises an interesting question IMO about FL_FREE_LABELTYPE. Should user-defined label types be assumed to be normal labels and not special labels?

[MatthiasWM]

@dannye your patch looks good to me and I think it can be applied. I do not think it is required to recurse into a multilabel of a multilabel, as it is not the intended use and would conflict with other routines including Fl_Menu_::copy(). FL_FREE_LABELTYPE, if ever used, is completely unpredictable. It could indicate a visual cue for text like FL_EMBOSSED_LABEL, but for all I know that char* could point to the secret location of the Smurf village. Any code that encounters a type >= FL_FREE_LABELTYPE should probably ignore the label pointer, but I don't think that this is consistently followed.

[dannye]

@MatthiasWM I would definitely assume that nested multilabels are exceedingly rare and therefore it might not be "worth it" to implement handling them recursively. But I think their innermost labels would still be drawn with the underline if they include '&' in their label, and it would be astonishing to the user for their alt shortcut to not work in that case, which is why I at least bring it up. I'm not strongly suggesting one way or the other. Although, If nested multilabels specifically shouldn't respond to alt shortcuts, then perhaps their underline should be elided too..

Similarly, that's why I bring up FL_FREE_LABELTYPE. Its text might be some user-defined type, but it also might be a char *, in which case it could possibly be drawn with an underline and would be a "bummer" if it didn't respond to the alt shortcut.

These are tricky problems though, and I'm not proposing any answers. Regardless, I think this PR is a clear improvement, as it is very ordinary to have a single-level (not nested) multilabel with an icon + text, and it is very desirable for the text portion to respond to its alt shortcut.

P.S. I don't know if FLTK has a naming convention for "one off" macros. I used #define IS_SPECIAL_LABELTYPE here (and #undefd it when I was done), but if there is a preferred naming convention, I'm happy to change it.

The macro could also be entirely replaced with this:

static bool is_special_labeltype(Fl_Labeltype t) { return t == _FL_MULTI_LABEL || t == _FL_ICON_LABEL || t == _FL_IMAGE_LABEL; }

I just didn't want to keep repeating what "t" is in the code, so macro or static function both work fine.

[MatthiasWM]

If you want to implement recursing into multilevel multilabel, please absolutely feel free to do so.

As for the FL_FREE_LABELTYPE question, a possible "solution" could be to introduce FL_FREE_COMPLEX_LABELTYPE = 128, and document that everything between FREE and COMPLEX is treated assuming label_ is a char*, and for everything COMPLEX and above, label_ is not touched an must be managed by the user code.

There is no naming convention for macros that are no publicly visible. I prefer static code wherever possible over macros.

[dannye]

As for the FL_FREE_LABELTYPE question, a possible "solution" could be to introduce FL_FREE_COMPLEX_LABELTYPE = 128, and document that everything between FREE and COMPLEX is treated assuming label_ is a char*, and for everything COMPLEX and above, label_ is not touched an must be managed by the user code.

I think that's a good suggestion. I would understand wanting to wait until after the release of 1.4.0 before ironing out those details though. Similarly, recursively handling multi-labels could also wait until then, in order to keep this change for 1.4.0 more on the modest side. Also because, selfishly speaking, I don't need recursive handling to fix the bugs in my application; I only need the changes already in this PR.

If you agree, then I think this is good to go, unless you would like me to add any tests/examples/documentation.

[MatthiasWM]

Tested with FLUID where I could now add Alt shortcuts for generating new widgets (complex menus with widgets).