Discrepant behaviour between Fl_Choice and Fl_Input_Choice

[pvrose]

The appearance of Fl_Choice and Fl_Input_Choice differs in the width of the menu button and the size of the arrow displayed in the button.

See https://groups.google.com/d/msgid/fltkgeneral/ecb54cd1-2c8c-4af6-9324-42dd0442397fn%40googlegroups.com for further information.

[erco77]

Right, so first the low hanging fruit: attaching a v1 patch to solve the default fltk scheme case, which seems straight forward enough.

To support the other schemes, hmm, there's a few things I need to ask the other devs about, as there's some squirrely stuff that should be sussed out first, namely:

1. The size of the menu button in Fl_Input_Choice is determined in the constructor, which implies the scheme is set BEFORE the widget is created (the 'protected' virtual menu_xywh() values determine the constructor for the underlying InputMenuButton class), which is /not/ the case if e.g. win->show(argc,argv) is used by the main app where the "-scheme" flag can change the scheme after the widget is created. Not sure exactly how to fix this while still supporting the protected (e.g. public) virtual inp_xywh() and menu_xywh() methods.

2. A redesign might be in order to allow both widgets to share the same code for drawing the arrow box/button instead of having mostly duplicated code that has to be carefully kept separately in sync.

[erco77]

Attaching a simple choicetest_v1.cxx program for testing.

[Albrecht-S]

@erco77: I took a look but it's too late here for details, so just a few thoughts.

1. I modified the test program so we can test different schemes interactively: see choicetest_v2.cxx. I also added a const value for initial widget heights (ch), but resizing the window can help testing width/height ascpects as well.
2. Scheme specific initialization code must be avoided as much as possible, especially since one can change the scheme interactively (see 1. above) and because of the point mentioned by you.
3. In the long term I strive to remove all scheme specific code that changes sizes of widgets or parts of widgets, if possible.
4. Therefore I believe we should remove the scheme specific widths of the arrow boxes in both widgets.
5. Below is a much simpler patch that fixes the box size at 20 px for all schemes as a proof of concept (maybe incomplete). The "patch" is under #if (0) but the old code should be removed if we decided to use it.
6. Widget heights less than 20 are rarely used, and otherwise the width is 20 anyway, hence no need for scheme specific calculation.
7. The only exception would be if the total width was too small to use 20, but this is not taken into account in current code as well (we can ignore this for now).

My potential patch:

$ git diff -- src/
diff --git a/src/Fl_Choice.cxx b/src/Fl_Choice.cxx
index 66bd57ab6..dbde71c9a 100644
--- a/src/Fl_Choice.cxx
+++ b/src/Fl_Choice.cxx
@@ -36,9 +36,13 @@ void Fl_Choice::draw() {
 
   // Arrow area
   int H = h() - 2 * dy;
+#if (0)
   int W = Fl::is_scheme("gtk+")    ? 20 :                       // gtk+  -- fixed size
           Fl::is_scheme("gleam")   ? 20                         // gleam -- fixed size
                                    : ((H > 20) ? 20 : H);       // else: shrink if H<20
+#else
+  int W = 20;
+#endif
   int X = x() + w() - W - dx;
   int Y = y() + dy;
 

@erco77 Thanks for taking care of this.

[pvrose]

@erco77: > Right, so first the low hanging fruit: attaching a v1 patch to solve the default fltk scheme case, which seems straight forward enough.

Applied this patch and it does what I want.

@Albrecht-S: If I understand your patch correctly with the widget height set to 20, the menu button height will be 18 and the width 20.

I was thinking earlier, could the Fl_Choice be considered a variant of the Fl_Input_Choice with the Fl_Input widget configured as an Fl_Output (i.e. setting its type to FL_NORMAL_OUTPUT.

[Albrecht-S]

@Albrecht-S: If I understand your patch correctly with the widget height set to 20, the menu button height will be 18 and the width 20.

@pvrose Yes, maybe (or maybe 16), and what exactly is your question?

I don't consider scheme specific sizes of (parts of) widgets a good solution. In Fl_Choice this is something done in draw() which makes changing schemes at runtime possible w/o issues. Fl_Input_Choice however is a combo widget derived from Fl_Group with separate input and (choice) button widgets which makes it harder to do the right thing because changing the scheme at runtime would not necessarily change the group layout, as Greg mentioned: the constructor decides about the sizes with the scheme that's active at construction time, AFAICT. That's another bad fact because the user's scheme may not yet be chosen at construction time. Therefore I vote for making the button width constant (it's currently constant 20 for some schemes and variying <= 20 for other schemes. Note that resizing the widget (window) would (again) consult the current scheme and maybe change the width if the scheme was changed in the meantime. Not a good GUI behavior, IMHO.

ADDED later: a constant width, independent of the scheme and widget height would IMHO look good and could be easily maintained for both widgets in question.

My patch doesn't yet consider the different inset mentioned elsewhere though which is one reason why I wrote it may be incomplete. This should obviously be added to harmonize the view of both widgets.

I was thinking earlier, could the Fl_Choice be considered a variant of the Fl_Input_Choice with the Fl_Input widget configured as an Fl_Output ...

Fl_Input_Choice was added later and is derived from Fl_Group whereas Fl_Choice inherits Fl_Menu_, thus we can't change the base class of any one of them because user-derived classes may rely on these facts.

@erco77 I didn't want to "take over" this issue, just try to help with my thoughts and a possible patch. Looking forward to your comment(s) and suggestions.

[pvrose]

@Albrecht-S: If I understand your patch correctly with the widget height set to 20, the menu button height will be 18 and the width 20.

@pvrose Yes, maybe (or maybe 16), and what exactly is your question?

@Albrecht-S : No question just a comment.

Fl_Input_Choice was added later and is derived from Fl_Group whereas Fl_Choice inherits Fl_Menu_, thus we can't change the base class of any one of them because user-derived classes may rely on these facts.

Fair enough, It was just an idea.

I'm happy the way I have it patched at the moment, so I'll shut up await the final version.

[erco77]

I'll take another pass at this; it would help a lot if the button didn't resize based on the scheme, as there's just no mechanism (AFAIK) for it to know the scheme changed unless there was a callback or some such. And in this case, because the button is necessarily "a widget", and not just drawn geometry, it would have to change its xywh size to draw a different size.
Using clipping I was able to simulate the Fl_Choice drawing style for the other schemes, which looks like this:

..so I think I can proceed with a final patch.

[Albrecht-S]

@erco77 The problem with your screenshot is that the InpChoice button looks too big by 1 px at the top and the bottom, but this is obviously caused by the Fl_Input using a DOWN box whereas the button uses an UP box. I don't think this can be avoided (easily) and I don't want to complain ;-)

[erco77]

Right, it's certainly the first thing I noticed too, but if the goal is to keep the same look as Fl_Choice, it's tricky.

I could investigate changing the default box type when the non-default schemes are active after I get the button part ironed out. I'm not crazy about the current behavior, where the button appears inside the box. This creates a lot of visually distracting tangential lines (borders touching each other), because the button really is "inside", which is why this is hard. For reference, here's how things look for the three non-default schemes:

[Albrecht-S]

I believe that looks "good enough", even with the small border issues in the previous comment (screenshot).

Are you ready to post a patch?

[erco77]

No, not yet, and I'm unfortunately also busy as #!@$ on paying work, lol.. thought this would be easy but it's not.

To get that "consistent look" I had to cheat and draw the button larger than its xywh is, which is a bad/illegal hack.

To do it right I either need to (a) make the button larger so it can either draw over the border box, or (b) change the border box for the widget to an FL_UP_BOX (in non-default schemes, which makes the gradient) and have the input area draw over that, and have the "button" draw() method just draw the arrows and vert divider. That might look nice, haven't tried it yet.

I'm thinking the latter (b) approach is the better way, and more like what Fl_Choice does.. just not sure how the input field will look drawn over that; might have to fl_push_clip() off its border to not clip the input field's character area. It's squirrely drawing business to get it right.

[Albrecht-S]

Thanks for the update. Looking forward to whatever you find when real work permits.

[erco77]

Finally had some time to come back to this.

Here's a input_choice_v2.patch.txt which ends up looking like this in the various schemes:

The goal being to keep it looking as much like Fl_Choice as possible, but with an input field.

In that patch I applied the scheme chooser Albrecht added in his example to the test/input_choice program so that it can be used for testing.

I'm happy enough with the look and the code. Give it a test and see what you think.

[erco77]

Update: Here's input_choice_v3.patch.txt which is a code cleanup of the above v2 patch; removal of unused vars and such.

[Albrecht-S]

@erco77 Thanks for your work on this and your latest "v3" patch. The patch is IMHO good and works well but I still found a few border cases where the look of Fl_Choice and Fl_Input_Choice differ. However, this is not a fault of your patch, it's some bad assumptions in Fl_Choice. My patch below fixes these issues.

1. The width of the (simulated) menu button in Fl_Choice is shrunk if the height of the widget is lower than 20 for some schemes. This (a) is inconsistent, (b) doesn't look good, and (c) doesn't match the better layout of the Fl_Input_Choice widget after your patch.
2. The constant height (+/- 8) of the divider line in some schemes would overlap the border of the widget if the widget's height is smaller than about 19. You fixed this in your patch and I "stole" your fix for Fl_Choice.
3. The divider line and the box borders of Fl_Choice and Fl_Input_Choice didn't align properly. I fixed this in my Fl_Choice patch as well (IMHO this is the right place to fix it).

You can see all these remaining issues if you use this reworked demo program choicetest_v3.cxx if you use it with your patch but w/o my Fl_Choice patch. Change schemes and resize the window, particularly to smaller heights.

Finally, here's my additional patch: Fl_Choice.diff.txt

From my POV your patch together with mine are good to go. The layout is now pretty consistent. The only remaining issue is that some arrow sizes don't match at very low widget heights, i.e. heights lower than 20 which are - more or less - pathological and IMHO negligible.

Please let me know if you want to commit both our patches together or if you commit yours and I commit mine independently. My Fl_Choice patch is IMHO an improvement anyway, even w/o your patch. I'm leaving the decision to you.

PS: I'd appreciate if you changed the copyright year of all concerned files in your patch as well.

[Albrecht-S]

PS: my latest choicetest_v3.cxx can also be compiled against FLTK 1.3.x for comparison but then you need to select the scheme on the commandline, for instance ./choicetest -s "oxy" etc..

PPS: in the future we might want to combine Fl_Choice, Fl_Input_Choice, and Fl_Scheme_Choice in test/input_choice.cxx, similar to what we have now in choicetest_v3.cxx. We could "merge" these two programs but I didn't bother yet...

[erco77]

OK, I'll apply the two patches as separate commits:

• input_choice_v3.patch.txt
• Fl_Choice.diff.txt

Once done and everything builds, I'll close the issue.

[erco77]

OK, both patches applied - closing this.
If anyone wants to add anything, feel free to reopen.

(EDIT: applied the copyright date updates as separate commits - I was focused on the code commits working properly)

[Albrecht-S]

FTR: I wrote

... in the future we might want to combine Fl_Choice, Fl_Input_Choice, and Fl_Scheme_Choice in test/input_choice.cxx, similar to what we have now in choicetest_v3.cxx. We could "merge" these two programs ...

Done in commit 64cd50c