RFC: Is issue #568 a problem or not?

[Greg Ercolano]

I noticed text was off by +1 on x/y in my app, and could see it
in e.g. the fltk test/radio demo.

Opened issue #568:
https://github.com/fltk/fltk/issues/568

wcout mentioned it was probably the new use of Pango, and that seemed
to be the cause.

But the question is: is this a given with pango, or is there actually a small x+1/y+1 offset bug?
If it's a given, I'll close the issue.

Curious what the opinion is.

One really notices this in apps where the buttons are small, where it's easy to see
when text is off, compared to previous releases. Trying to avoid refactoring a lot of code
to get things recentered.

[imacarthur]

Hmm, I don't know, but it could well be off - I assume for pango the drawing of the glyphs will be handled by a different renderer (via Cairo?) than our usual direct-to-XFT scheme so there may be issues with origin.

That said, I hadn't actually noticed that.

I do seem to get different font faces though, and maybe at slightly different sizes, perhaps? So the pango vs. XFT font thing is not a straight mapping... It's tricky. Don't know.

 

[Albrecht Schlosser]

On 12/2/22 09:00 Greg Ercolano wrote:

I noticed text was off by +1 on x/y in my app, and could see it
in e.g. the fltk test/radio demo.

Opened issue #568:
https://github.com/fltk/fltk/issues/568

wcout mentioned it was probably the new use of Pango, and that seemed
to be the cause.

Hmm, I don't see that Pango is used in 1.3.x at all. I understand that you report that the same issue as in 1.4.x (using Pango) is already in 1.3.x (i.e. branch-1.3 after the release of 1.3.8). I doubt that this is the case. Or maybe I misunderstand your report.

Greg, can you please double check your findings? If it's related to Pango, then 1.3.x (git) should *not* be affected and you might want to edit your report.

But the question is: is this a given with pango, or is there actually a small x+1/y+1 offset bug?
If it's a given, I'll close the issue.

Curious what the opinion is.

I don't have a clear opinion. Generally I'd say that font sizes etc. can differ depending on platforms and you should never rely on a particular text fitting exactly in a button or whatever. There can always be small differences, and if you use different fonts you'll notice that the text can be much wider or smaller depending on the font (see the long text in your unittests comparison gif ("NOTE: On systems ...").

That said, if we can find out that we have - additionally to different font rendering - also a positioning (x+1, y+1) issue then we should try to fix the latter. But this could be really hard because bare X11 vs. Xft vs. Pango (Cairo) all use their different rendering and font width calculations.

Maybe Manolo can tell us more... (?)

One really notices this in apps where the buttons are small, where it's easy to see
when text is off, compared to previous releases. Trying to avoid refactoring a lot of code
to get things recentered.

In the unittests gif image  posted on GitHub it looks as if the left and right borders of fl_measure (red) vs. fl_text_extents (green) are - at least in some cases - offset by one pixel to the left or right side. This makes me believe that there could really be a positioning issue which could perhaps be fixed.

It would be interesting to see images made on Windows and macOS for comparison. ;-)

That said:

1. I did not test anything yet, my "opinions" above are based on what I see

2. IMPORTANT: if this is really an issue in 1.3.x (git branch-1.3) WRT. 1.3.8 then we should investigate and fix it in 1.3.9 which I intend to release soon because of the known compilation error in Visual Studio (which leads to unnecessary error reports).

[Greg Ercolano]

Hmm, I don't see that Pango is used in 1.3.x at all. I understand that you report that the same issue as in 1.4.x (using Pango) is already in 1.3.x (i.e. branch-1.3 after the release of 1.3.8). I doubt that this is the case. Or maybe I misunderstand your report.

Greg, can you please double check your findings? If it's related to Pango, then 1.3.x (git) should *not* be affected and you might want to edit your report.

    OK -- yeah, I was wondering about that pango being in 1.3.x.
    I figured big changes like that would only be in 1.4 development.

    FWIW 'git status' for that 1.3.x result says I'm on branch-1.3, but
    it also says something about a rebase in progress, so I think I better
    rename that dir away and reclone/checkout branch-1.3, as something's
    probably amiss with that old dir 1.3.x dir I had lying around.

    Thanks for the other input -- I read through it.

    For sure the 1.4.x info is accurate, and can probably be replicated
    when compared to a 1.3.8 source tar ball build.

[imm]

FWIW, I fired up the unittests demo under Windows, both 1.3.x. and
1.4, built from the current tip of the repo.
The text_extents test looks basically OK (on that platform)

Since this was the box that has WSL2/WSLg installed, I also tried the
Linux build and again it looked OK I think - some tendency for the
text to spill out of the right hand edge of the bounding box on the
line that ends with a capital "T" but otherwise generally fine.
(However, this isn't a "real" Xserver so rendering may be
different...? Certainly, testing the WSLg stuff has exposed some
peculiarities...)

I suspect that different fonts are being picked in each case and that
may account for the visible differences?
ISTR there was some attempt to enhance the font selection, possibly
between 1.3.4 and 1.3.8, so that may be in effect here?
Or maybe I'm talking nonsense...

[Greg Ercolano]

On 12/2/22 03:42, Albrecht Schlosser wrote:

Greg, can you please double check your findings? If it's related to Pango, then 1.3.x (git) should *not* be affected and you might want to edit your report.

    OK, edits made.

    Yeah, that 1.3.x I was working with must've been somewhere between branch-1.3 and 1.4,
    based on the mix of messages in that log I posted.

    So it's a pango issue I suppose, only in 1.4.x, which is certainly better than what I'd thought
    it leaking into 1.3.x..!

    From what I can tell, though, it's more than 'just pango' affecting things. I think there might
    be a subtle bug, perhaps which gets worse the further the text is from the top/left corner.
    Sort of a scaling thing.

    I'll see if I can whip up something later today to see if I can exercise that a bit.

[Greg Ercolano]

On 12/2/22 06:07, imm wrote:

FWIW, I fired up the unittests demo under Windows, both 1.3.x. and
1.4, built from the current tip of the repo.
The text_extents test looks basically OK (on that platform)

    Ya, that's good info, as it seems to be pango specific,
    and therefore I imagine wouldn't be affected on windows.

I suspect that different fonts are being picked in each case and that
may account for the visible differences?

    Maybe, but I think perhaps more it's the new pango effects.

    For instance, in the 2nd comparison animation:
    https://user-images.githubusercontent.com/6484779/205324110-597921a5-46ae-46fc-a615-2f0af3ce7f4c.gif

    ..one can see at the top/left where it says "Unit Tests" above the browser,
    the "T" and "e" in Tests are getting a little too close in the pango=on mode.
    That's probably something to do with kerning, which I think Manolo mentioned
    in his comment on that issue.

ISTR there was some attempt to enhance the font selection, possibly
between 1.3.4 and 1.3.8, so that may be in effect here?
Or maybe I'm talking nonsense...

    I think we can ignore anything about the problem being in 1.3.x;
    that turned out to be a bad 1.3.x git checkout on my end,
    so that was indeed all nonsense, lol.

    Seems to be purely in 1.4.x, and seems to be pango related as wcout
    brought to light.

    If the pango stuff does have a problem (that's undecided at present),
    it might perhaps be a scaling type of problem, as text near the top/left
    corner doesn't wiggle as much as further away on the x and y axis,
    which I think my radio animation comparison might be showing:
    https://user-images.githubusercontent.com/6484779/205161223-33cae018-dccd-4fd3-9162-eb59881dfa9c.gif

    ..the text for the top left button "Fl_Button A1", the top left of the first "F" doesn't move at all.
    But as you go down the window, the first "F" in the other buttons start to be offset +1 on x.
    And the radio buttons seem to all be +1 on both X and Y.
    The position of the radio text is probably pre-calculated based on the font measuring stuff
    to get things centered, so if something is wrong with that, that might explain the offset for
    only those.

    I'm not sure yet.. will try to do some tests drawing text at fixed positions in a grid
    across the window and comparing with vs without pango, etc.

    I plan to follow up later today with something that can maybe test
    that theory where text positions might be scaled a tiny bit relative
    to the top left corner.

[Albrecht Schlosser]

On 12/2/22 16:36 Greg Ercolano wrote:

On 12/2/22 06:07, imm wrote:

I suspect that different fonts are being picked in each case and that
may account for the visible differences?

    Maybe, but I think perhaps more it's the new pango effects.

I believe this is indeed a font difference...

    If the pango stuff does have a problem (that's undecided at present),
    it might perhaps be a scaling type of problem, as text near the top/left
    corner doesn't wiggle as much as further away on the x and y axis,
    which I think my radio animation comparison might be showing:
    https://user-images.githubusercontent.com/6484779/205161223-33cae018-dccd-4fd3-9162-eb59881dfa9c.gif

I'm looking at the animated gif linked above in Firefox, zoomed at 200% in Firefox ...

    ..the text for the top left button "Fl_Button A1", the top left of the first "F" doesn't move at all.

..but if you look closely at 200% zoom you see that the 'F' changes a bit: the top left stays the same but the vertical line becomes a tiny bit longer. The same is true for all 'F's in all buttons.

    But as you go down the window, the first "F" in the other buttons start to be offset +1 on x.

Not in the first three "normal" buttons. This is only true for the Light-, Check- and Round buttons on the left side. This can either be a change in the positioning algorithm of these button types or there *might* be one or more spaces (' ') involved. The latter would also explain the different positioning (different width of space in font). Too lazy to look at the sources now, sorry.

    And the radio buttons seem to all be +1 on both X and Y.

Yep, it looks so. I have no explanation for this...

    The position of the radio text is probably pre-calculated based on the font measuring stuff
    to get things centered, so if something is wrong with that, that might explain the offset for
    only those.

Maybe, something like that.

    I'm not sure yet.. will try to do some tests drawing text at fixed positions in a grid
    across the window and comparing with vs without pango, etc.

Looking forward to your results. Having fixed text positions rather than different buttons would certainly be a better test.

BTW, I would like to see separate images additionally to animated gifs to be able to view them in a browser and switch the image on a button click rather than at fixed times. TIA

[Bill Spitzak]

That animation looks pretty informative!

I think the very first thing to figure out is why the font changes size on all the buttons, but only seems to move on the radio buttons and box caption. I would expect identical changes to all the text!

--
You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/349282ed-7638-b341-2ef9-2d551d99b5e9%40online.de.

[Greg Ercolano]

On 12/2/22 08:36, Albrecht Schlosser wrote:

    ..the text for the top left button "Fl_Button A1", the top left of the first "F" doesn't move at all.

..but if you look closely at 200% zoom you see that the 'F' changes a bit: the top left stays the same but the vertical line becomes a tiny bit longer. The same is true for all 'F's in all buttons.

    See my recent two comments using a text grid.

    Really seems to show, at least regarding drawing text at an x,y position with fl_draw(s,x,y),
    that things are more stable now.

    There /may/ be more to do, perhaps not with text drawing, but with text measuring,
    as I can only guess the button and radio widgets determine how to center text based
    on, maybe the fl_measure() stuff or Ian's relatively new extents, or some such.

    Manolo did mention in his first comment in the issue listing 4 factors at work,
    specifically point #4 on kerning, where kerning is "off" for xft, but on for pango.
    He wrote: "Kerning applies in Pango. Despite much efforts, I didn't find how to turn it off.
    Any hint about how to do that is much welcome."

    So perhaps that's related to the text width being slightly affected due to inter-letter spacing.

BTW, I would like to see separate images additionally to animated gifs to be able to view them in a browser and switch the image on a button click rather than at fixed times. TIA

    If you want to do that, you can load the gif in gimp, and under layers you'll see
    the two images as separate layers, and you can simply turn the visibility of one
    layer on and off by just repeatedly clicking the "eyeball" icon for that top layer.

[imm]

On Fri, 2 Dec 2022, 23:03 Greg Ercolano wrote:

    There /may/ be more to do, perhaps not with text drawing, but with text measuring,
    as I can only guess the button and radio widgets determine how to center text based
    on, maybe the fl_measure() stuff or Ian's relatively new extents, or some such.

I think most (all) widgets that measure the text at all use fl_measure rather than the text_extents stuff. The fl_measure approach should leave more balanced spacing around the text... Should...

    Manolo did mention in his first comment in the issue listing 4 factors at work,
    specifically point #4 on kerning, where kerning is "off" for xft, but on for pango.
    He wrote: "Kerning applies in Pango. Despite much efforts, I didn't find how to turn it off.
    Any hint about how to do that is much welcome."

The odd thing about that is that kerning does often tend to make strings shorter rather than longer, since it causes some characters to close up to better utilize dead space between them. Yet we're seeing the strings getting longer than expected.

So it might be a consequence of kerning but I'm not sure it is.

--
Ian
From my Fairphone FP3

[Albrecht Schlosser]

On 12/3/22 00:03 Greg Ercolano wrote:
>
> On 12/2/22 08:36, Albrecht Schlosser wrote:
>
>>
>>>     ..the text for the top left button "Fl_Button A1", the top left
>>> of the first "F" doesn't move at all.
>>
>> ..but if you look closely at 200% zoom you see that the 'F' changes a
>> bit: the top left stays the same but the vertical line becomes a tiny
>> bit longer. The same is true for all 'F's in all buttons.
>
>
>     See my recent two comments using a text grid.
>
>     Really seems to show, at least regarding drawing text at an x,y
> position with fl_draw(s,x,y),
>     that things are more stable now.

I'm a little surprised to see that because my previous tests with the
latest commit seemed to show that Manolo's fix moved the text even a
little more down than before (x was good but y seemed to be too large,
text moved downwards). I'll have to look at your test program but it's
too late here now, so maybe tomorrow...

> There /may/ be more to do, perhaps not with text drawing, but with
> text measuring,
>     as I can only guess the button and radio widgets determine how to
> center text based
>     on, maybe the fl_measure() stuff or Ian's relatively new extents,
> or some such.

Yes, perhaps. The "radio" test with the buttons certainly used text
measurement and this could differ as well and explain my findings. This
would need more investigation.

>> BTW, I would like to see separate images additionally to animated
>> gifs to be able to view them in a browser and switch the image on a
>> button click rather than at fixed times. TIA
>
>     If you want to do that, you can load the gif in gimp, and under
> layers you'll see
>     the two images as separate layers, and you can simply turn the
> visibility of one
>     layer on and off by just repeatedly clicking the "eyeball" icon
> for that top layer.

I knew that I can switch layers on and off but I never tried it with an
animated gif. I'll try that tomorrow, thanks for the hint.

[Bill Spitzak]

In the animated gif by far my biggest concern is that *some* of the text changes size, but not all of it! I would expect any error in font size to be the same everywhere.

--
You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/376693fe-ae63-60b0-8b12-acbc14ae0190%40online.de.

[imm]

On Sat, 3 Dec 2022 at 02:57, Bill Spitzak wrote:
>
> In the animated gif by far my biggest concern is that *some* of the text changes size, but not all of it! I would expect any error in font size to be the same everywhere.

Oh, that's an interesting observation that I hadn't really seen... So
Bill, you are saying that it looks like some of the text changes size
whilst "the same" text (in the same font) rendered elsewhere, does
not?
Or have I misunderstood...

If that's correct, then I wonder if there is some sort of AA effect
coming into play here? If the PanGo/Cairo text coordinate system is
using floats and ours is integers, there may be subtle alignment
differences in where the text is placed w.r.t. the pixel grid, which
might in turn affect how the text is AA processed, making the glyphs a
bit thicker or thinner depending on how the grids align?

Or, you know, I could be reaching out and firmly grasping the wrong
end of the stick, here...

[Greg Ercolano]

On 12/2/22 15:56, Albrecht Schlosser wrote:

On 12/3/22 00:03 Greg Ercolano wrote:

    See my recent two comments using a text grid.

    Really seems to show, at least regarding drawing text at an x,y position with fl_draw(s,x,y),
    that things are more stable now.

I'm a little surprised to see that because my previous tests with the latest commit seemed to show that Manolo's fix moved the text even a little more down than before (x was good but y seemed to be too large, text moved downwards). I'll have to look at your test program but it's too late here now, so maybe tomorrow...

    If you mean your test with the radio buttons, that's a different animal because
    the drawing of the label uses FL_ALIGN_XXX, which probably pulls in all kinds
    of calculations based on things like fl_measure(), fl_width(), etc, which is where
    I suspect that issue is coming from.

    To prove it I'm trying to do the deep dive down the label drawing code to
    see where the discrepancy is coming in.

    So far, going down the calling stack:

        The label drawing code calls fl_draw() with an x,y value that is the same
        in both the pango and non-pango code, with an align value of 20 (decimal)
        (which seems to be (FL_ALIGN_INSIDE|FL_ALIGN_LEFT))

        The fl_draw() code that handles the align calculations ends up drawing text
        at a slightly different y position:

                pango off: 208,29     - fl_descent() is 4
                pango on: 208,31     - fl_descent() is 3
       
    So when pango is ON: there's a y offset of +2, and a -1 for the value of fl_descent().

    I'll add these oberservations to the issue, as there's obviously some weirdness going
    on with the font measuring stuff, which is causing trouble with the positions of labels
    that are 'aligned'.

[Albrecht Schlosser]

On 12/4/22 01:46 Greg Ercolano wrote:
>
> On 12/2/22 15:56, Albrecht Schlosser wrote:
>
>> On 12/3/22 00:03 Greg Ercolano wrote:
>>>     See my recent two comments using a text grid.
>>>
>>>     Really seems to show, at least regarding drawing text at an x,y
>>> position with fl_draw(s,x,y),
>>>     that things are more stable now.
>>
>> I'm a little surprised to see that because my previous tests with the
>> latest commit seemed to show that Manolo's fix moved the text even a
>> little more down than before (x was good but y seemed to be too
>> large, text moved downwards). I'll have to look at your test program
>> but it's too late here now, so maybe tomorrow...
>
>     If you mean your test with the radio buttons, that's a different
> animal because
>     the drawing of the label uses FL_ALIGN_XXX, which probably pulls
> in all kinds
>     of calculations based on things like fl_measure(), fl_width(),
> etc, which is where
>     I suspect that issue is coming from.

Yes, I believe that's the reason - measurement, font metricx, alignment, ...

>     To prove it I'm trying to do the deep dive down the label drawing
> code to
>     see where the discrepancy is coming in.
>
>     So far, going down the calling stack:
>
>         The label drawing code calls fl_draw() with an x,y value that
> is the same
>         in both the pango and non-pango code, with an align value of
> 20 (decimal)
>         (which seems to be (FL_ALIGN_INSIDE|FL_ALIGN_LEFT))
>
>         The fl_draw() code that handles the align calculations ends up
> drawing text
>         at a slightly different y position:
>
>                 pango off: 208,29     - fl_descent() is 4
>                 pango on: 208,31     - fl_descent() is 3
>
>     So when pango is ON: there's a y offset of +2, and a -1 for the
> value of fl_descent().

I don't know where the y position is aligned, is it the bottom of the
printing area, or is it the baseline of the font, or ... ?

> I'll add these oberservations to the issue, as there's obviously some
> weirdness going
>     on with the font measuring stuff, which is causing trouble with
> the positions of labels
>     that are 'aligned'.

I thought about closing the issue as resolved (because the positioning
issue seems to be clear) and opening another issue for font measurement
and label alignment. What do you think about this?

[Greg Ercolano]

I'll add these oberservations to the issue, as there's obviously some weirdness going
    on with the font measuring stuff, which is causing trouble with the positions of labels
    that are 'aligned'.

I thought about closing the issue as resolved (because the positioning issue seems to be clear) and opening another issue for font measurement and label alignment. What do you think about this?

    Meh, that ship has sailed since I already added the analysis to the issue.
    Also, I think the problem with measurement is part of that issue.

    Manolo definitely nailed one part of it for sure, the other part with measurement
    is simply the second part.

    I'm not familiar with the two calls that seem to be the issue, fl_height() and fl_descent(),
    but those are the things that appear to be causing that particular issue with the radio labels;
    see my recent additional comment.

[Greg Ercolano]

On 12/3/22 16:54, Albrecht Schlosser wrote:

I don't know where the y position is aligned, is it the bottom of the printing area, or is it the baseline of the font, or ... ?

    The docs for fl_draw() say: Text is aligned to the left and to the baseline of the font.
    And this seems to be true; if I draw an fl_line() at the y position I pass to fl_draw(),
    the line goes through the baseline of the font.

    I'm not sure what the low level drawing code expects the Y value to be, but it
    seems to be much larger. In my case the y value passed to fl_draw() was 10,
    but after alignment and such, the final Y value was 29, which is what the low
    level drawing code wants. Since that's so much lower than the baseline,
    I'm not really sure what that is, actually. I didn't try to figure that out.

[imm]

My memory, going back to the extents work, was that the (Xft) interface would return a bounding box that had quite a lot of space around the glyphs, and that's what fl_measure uses, more or less.

For the string, XFT returns values for an origin at the lower left I think, which we then change into fltk coordinates.

Do not know what pango does...

The bounding boxes shown in the text unit test might show the Xft Vs pango effects?

[imm]

So the bounding box returned by fl_measure does differ a bit (but not
that much...) between XFT and Pango cases.
Still, I wonder if that might be enough to throw the widget labels off here?

As an aside, Greg mentioned drawing a baseline for the fonts, and I do
wonder if that is something we should add to the render text Unit
test?
It does seem like a handy thing to have - in this case it might show
us whether the assumed baseline was different between the two cases.
If the pango baseline is in a different place from what we assume
(based on the XFT or X11 behaviour) then that would certainly push
things out of whack!

[Albrecht Schlosser]

On 12/4/22 20:11 imm wrote:
> So the bounding box returned by fl_measure does differ a bit (but not
> that much...) between XFT and Pango cases.
> Still, I wonder if that might be enough to throw the widget labels off here?

I still don't know if we see two different physical fonts selected in
the Xft vs. Pango case. The latest fix should at least make the font
sizes the same, but different font metrics might still cause some
differences.

> As an aside, Greg mentioned drawing a baseline for the fonts, and I do
> wonder if that is something we should add to the render text Unit test?

+1

That would be nice to have. Maybe with a light or check button to switch
the baseline drawing on/off.

[imacarthur]

On Sunday, 4 December 2022 at 20:11:50 UTC Albrecht Schlosser wrote:

> As an aside, Greg mentioned drawing a baseline for the fonts, and I do
> wonder if that is something we should add to the render text Unit test?

+1

That would be nice to have. Maybe with a light or check button to switch
the baseline drawing on/off.

Attached, a suggestion of how the revised test would work...

 

[Albrecht Schlosser]

On 12/5/22 15:04 imacarthur wrote:

On Sunday, 4 December 2022 at 20:11:50 UTC Albrecht Schlosser wrote:

> As an aside, Greg mentioned drawing a baseline for the fonts, ...

Attached, a suggestion of how the revised test would work...

Thanks, I tried it and it looks good. However, on Linux (X11 and Wayland) the button box is too small to hold the label. I fixed this in my attached patch(es). Now it is not filled completely on Windows but this is IMHO the minor issue (sure, we could measure the text ... :-)).

Then I stumbled across your casts. I looked at the CMP and saw that we do not really forbid 'reinterpret_cast' (it is not mentioned in the STR) but I didn't find it anywhere in the sources. While trying to change this to an old style cast I noticed the (Fl_Callback *) cast as well and (SCNR) I "simplified" the code: I changed the first parameter of cb_base_bt to Fl_Widget* to comply with (Fl_Callback *) and used it then with parent() to get the group pointer. This is less code and no casts, but parent() is yet another function call. Anyway, this is in unittest_text_v1.diff.

Further I wasn't sure if the baseline would overwrite the lower border of the green fl_text_extents box (it did not in my tests). But since the baseline is longer anyway (good idea, BTW) I thought it would be a better to draw the baseline first. If it is not visible below the text, then the fl_text_extents() box must have been drawn over it. This needed separate variables for the text measurement and I rearranged the measurement and drawing.

The result is in unittest_text_v2.diff and I personally prefer this one. What do you think?

[Albrecht Schlosser]

On 12/6/22 12:13 Albrecht Schlosser wrote:
> I looked at the CMP and saw that we do not really forbid
> 'reinterpret_cast' (it is not mentioned in the STR)

The last part should read "in the CMP", of course, please see
https://www.fltk.org/cmp.php#CS_FLTK_1_1_X_RESTRICTIONS

> ... but I didn't find it anywhere in the sources. ...

The question is: do we allow or forbid 'reinterpret_cast' in FLTK 1.4?

Note: according to
https://en.cppreference.com/w/cpp/language/reinterpret_cast
reinterpret_cast seems to be C++ 98 or even older, hence we might allow
it? Or is it "allowed" anyway because it's not forbidden in the CMP?


If we can find an answer I would want to update the CMP for FLTK 1.1.x
up to 1.4.x and add 'reinterpret_cast' as allowed or forbidden.

Comments, opinions?

[imacarthur]

On Tuesday, 6 December 2022 at 11:13:52 UTC Albrecht Schlosser wrote:

On 12/5/22 15:04 imacarthur wrote:

Attached, a suggestion of how the revised test would work...

Thanks, I tried it and it looks good. However, on Linux (X11 and Wayland) the button box is too small to hold the label. I fixed this in my attached patch(es). Now it is not filled completely on Windows but this is IMHO the minor issue (sure, we could measure the text ... :-)).

Indeed.

Which is pretty much where this thread started...

 

Then I stumbled across your casts. I looked at the CMP and saw that we do not really forbid 'reinterpret_cast' (it is not mentioned in the STR) but I didn't find it anywhere in the sources. While trying to change this to an old style cast I noticed the (Fl_Callback *) cast as well and (SCNR) I "simplified" the code: I changed the first parameter of cb_base_bt to Fl_Widget* to comply with (Fl_Callback *) and used it then with parent() to get the group pointer. This is less code and no casts, but parent() is yet another function call. Anyway, this is in unittest_text_v1.diff.

FWIW (and as you note in your later post) the C++ casting style static_cast<> reinterpret_cast<>  (and the hideous const_cast<>) are "pretty ancient" now, and are more explicit about what the cast should do, addressing the inherent ambiguity in the old C-style cast mechanism.

So I'm in favour of at least allowing (if not mandating!) the C++ style casts. (And those of us who have to deal with MISRA, Autosar, etc. have to deal with this all the time...)

As regards the casting of the callback method, TBH I started doing it in that style because that's what fluid does for the code it generates, much of the time. So I took to doing the same for consistency. But yes, I guess it is at odds with what a lot of our other code and samples actually do!

 

Further I wasn't sure if the baseline would overwrite the lower border of the green fl_text_extents box (it did not in my tests). But since the baseline is longer anyway (good idea, BTW) I thought it would be a better to draw the baseline first. If it is not visible below the text, then the fl_text_extents() box must have been drawn over it. This needed separate variables for the text measurement and I rearranged the measurement and drawing.

Drawing the baseline before we draw any of the boxes seems like a good idea, yes.

The result is in unittest_text_v2.diff and I personally prefer this one. What do you think?

Haven't actually looked yet! (Will now...)

 

[imacarthur]

On Tuesday, 6 December 2022 at 11:13:52 UTC Albrecht Schlosser wrote:

The result is in unittest_text_v2.diff and I personally prefer this one. What do you think?

Yes, looks good. Let's do that...

FWIW, we probably could just use the fl_measure width to draw the baseline, since we are extending the baseline some way beyond the boxes anyway, and that would save us the extra parameters you were worried about.

Or not, since this works well!

[melcher....@googlemail.com]

Albrecht Schlosser schrieb am Dienstag, 6. Dezember 2022 um 12:33:07 UTC+1:

The question is: do we allow or forbid 'reinterpret_cast' in FLTK 1.4?

AFAIK, reinterpret_cast is pretty much the same as the cast operator, which we use extensively. I don't see much harm, but no much benefit over the operator either. dynamic_cast OTOH is not allowed because we don't have RTTI. So, I am '0' on this. I sometimes dearly wish we could use Lambdas and the std library though ;-) .

[imacarthur]

Hmm, no, I don't think reinterpret_cast is "the same" as the C cast operator, it is more restricted.

Consider this sample:

#include <stdio.h>

#include <stdint.h>

int main (void)

{

    float f1 = 3.456;

    int i1 = 0;

    int i2 = 0;

    int *p3 = NULL;

    printf ("%X %X %.3f\n", i1, i2, f1);

    i1 = static_cast<int>(f1);

    p3 = reinterpret_cast<int*>(&f1);

    i2 = *p3;

    printf ("%X %X %.3f\n", i1, i2, f1);

    return 0;

}

/* end of file */

 Here "static_cast" is saying "copy the VALUE of this variable to this other different type of variable" which is pretty much what the C-style cast "usually" does.

But you can’t "static_cast" a pointer type, that fails, so use "reinterpret_cast" instead which is "treat this pointer as if it were a pointer of this other type, even though it isn't". Which is what the C-style cast does *only sometimes", but is conceptually a different behaviour from the "static cast" case - it was this ambiguity that Stroustroup et al were trying to make explicit...

As Matt says, "dynamic_cast" is a trickier one as it may need RTTI etc., which we probably do not want in the library - though I do use it a fair bit in my own code. (It may also be "expensive" as it is often evaluated at runtime rather than at compile time if the code analysis can't unambiguously determine the type at compile time, so this has a performance penalty.)

And the "last" case is "const_cast" which basically means "I did something stupid and declared this variable as const but now I'm going to try and write to it anyway" which is essentially a sign that it is time to refactor your code... 

[Albrecht Schlosser]

On 12/6/22 16:22 imacarthur wrote:

On Tuesday, 6 December 2022 at 14:24:24 UTC Matt wrote:

Albrecht Schlosser schrieb am Dienstag, 6. Dezember 2022 um 12:33:07 UTC+1:

The question is: do we allow or forbid 'reinterpret_cast' in FLTK 1.4?

AFAIK, reinterpret_cast is pretty much the same as the cast operator, which we use extensively. I don't see much harm, but no much benefit over the operator either. ...

The question was whether to allow or disallow it, not whether one or the other has benefits. See further below...

Hmm, no, I don't think reinterpret_cast is "the same" as the C cast operator, it is more restricted.

Consider this sample:

[example code snipped]

Here "static_cast" is saying "copy the VALUE of this variable to this other different type of variable" which is pretty much what the C-style cast "usually" does.

But you can’t "static_cast" a pointer type, that fails, so use "reinterpret_cast" instead which is "treat this pointer as if it were a pointer of this other type, even though it isn't". Which is what the C-style cast does *only sometimes", but is conceptually a different behaviour from the "static cast" case - it was this ambiguity that Stroustroup et al were trying to make explicit...

@Ian: thanks for this detailed explanation.

My question was more about: do we *allow* to use it in the code? This was meant to discuss whether our users might use ancient compilers that don't support it.

I'm really not a "C++ guru" and reading the standards with all their different cases makes me ... hmm, say, impatient, and I don't think many C++ programmers understand all the intrinsics. Anyway:

*IF* "reinterpret_cast" is more restricted than the C-style cast *AND* we don't need to worry about ancient compilers then I'd say using it can't be wrong in cases where it is allowed. This would make our code gradually safer if the compiler could find code (some types of casts) that are not going to work as intended.

I'm particularly concerned about maintenance: when you use a C-style cast that is correct at the time you write the code and later someone changes the argument of the cast to another type then this can and will often fail (if you miss to change the cast because the compiler doesn't flag an error). The more the compiler can detect, the better it is.

In other words: the benefit would be that it is more restricted and it wouldn't do any harm if all our target systems could compile it. Hence it should IMHO be allowed to use in the library code where applicable.

Or am I missing a point?

[Albrecht Schlosser]

On 12/6/22 12:59 imacarthur wrote:

On Tuesday, 6 December 2022 at 11:13:52 UTC Albrecht Schlosser wrote:

The result is in unittest_text_v2.diff and I personally prefer this one. What do you think?

Yes, looks good. Let's do that...

Please feel free to commit and push (it's basically your work, even with my mods, and I'm pretty busy with other FLTK stuff) - or drop me a note here and I will do it if you like.

[imacarthur]

What's policy on this? Can I simply pull into master (I assume I can, but is it "permitted"?) or should I do a branch and PR?

[imacarthur]

On Tuesday, 6 December 2022 at 16:36:01 UTC Albrecht Schlosser wrote:

My question was more about: do we *allow* to use it in the code? This was meant to discuss whether our users might use ancient compilers that don't support it.

It is common for the "coding standards" (I'm thinking e.g. MISRA, Autosar, etc.) to require C++ style casts be used in C++ code to reduce ambiguity, so I see this a lot...

The oldest compiler I regularly have to work with dates to about 2008, and supports the C++ style no problem. I think that cast style is really pretty ancient now.

I pretty much always use the C++ style as a result of continued exposure (in C++ code; they do not work in plain-C code...)

They are a bit more verbose to type, but they do carry some additional information about the action and do detect some edge cases, so may be worthwhile.

FWIW, having "converted" a fairly large body of code from one style to the other, the resulting stripped binary objects were identical before/after, indicating that there is no runtime consequence to this, it is all information guide the compiler - and programmer!

(That's not true of dynamic_cast<> of course, but for the other 3 cases it is.)

Overall, my feeling is that we should not make it mandatory or anything, but on the other hand we should not preclude it - I think it is widely supported and does provide some maintenance value.

[Manolo]

All FLTK developers are entitled to push to master. I'd say the policy is to push there changes a developer

is confident they are functional. In case of doubt, a dedicated branch in the developer's own fltk fork  is handy

to allow further work until confidence arrives.

[Albrecht Schlosser]

Yes, that's correct for those FLTK devs with write access to the FLTK repo (and Ian is one of them).

The further policy is to push commits always to the tip of the master branch and never to merge origin/master "downstream" into your local development branch (if any). We don't like "unnecessary merges", i.e. one strategy is something like this in your local repo:

$ git checkout master
$ git pull
$ git checkout feature-branch   # assuming you have created one
$ git rebase master
$ git checkout master
$ git merge --ff-only feature-branch
$ git push

Instead of creating, rebasing, and merging a feature-branch it is possible to commit directly to your local master branch. If you run `git push` and it is rejected, run `git pull --rebase` and the `git push` again, in other words:

$ git checkout master
$ git pull
$ # work ...
$ git commit
$ git pull --rebase
$ # fix merge conflicts, test ...
$ git push

If `git push` is rejected because someone else pushed meanwhile, run `git pull --rebase` again until `git push` succeeds.

And, as a third option: create a feature branch in your own fork, make a pull request, and merge it using the GitHub merge button, choosing the "rebase" option. This is what Matthias does often.

It's your choice.

[melcher....@googlemail.com]

Manolo schrieb am Mittwoch, 7. Dezember 2022 um 11:17:40 UTC+1:

All FLTK developers are entitled to push to master. I'd say the policy is to push there changes a developer 

is confident they are functional. In case of doubt, a dedicated branch in the developer's own fltk fork  is handy

to allow further work until confidence arrives.

I like to go through a branch and make it into a pull request on GitHub. Even if I pull my request myself later, it gives CI a chance to run on all platforms and warn me if I missed a MSWindows peculiarity again, or such.