Tk Source Code

There's a regression with Tk_DrawChars() and possibly with Tk_DrawAngledChars()
regarding clipping glyph boxes. It can be observed with the tktreectrl widget
when doing pixel-wise horizontal scrolling. Single glyphs at the left and/or
right edges of the window are left out due to premature stop of rendering
caused by wrong checks of coordinates.

The attached patch reorders and corrects these checks but is not at all optimized.

Thanks for your review, much appreciated!

I have now merged this bugfix from Christian Werner to core-8-6-branch and then to trunk.

+1. After reading all remarks, I would say: good work! Just go ahead merging it!

Francois, thank you for your careful and precise wording.

That's a sensible remark, and I have moderated the impact of the comment Gregor requested me to insert, see [698a03d78e].

I think there's nothing left to be done before the merge. However, I doubt that
the comment provides added value given that NUM_SPECS is 1024 and thus able to
cover about 6000 pixels for a 6 pixel wide font (which is IMO pretty unreadable
anyway).

Gregor Cramer, who originally made the change that triggered this ticket, has reviewed the fix and reports that it is OK. He has requested I introduce a comment in the code though, see [555014b9a0].

Is there anything still left to be done on this before we merge?

Thanks for the explanations.

Patch committed in a bugfix branch bug-71b1319acc for easy review.

fvogel, regarding your last question: my test setups are

  1. release tarballs 8.6.7 on CentOS 6.9 x86_64
  2. release tarballs 8.6.8 on CentOS 6.9 x86_64

Test snippet:

  pack [text .text -wrap none]
  .text insert end [string repeat "text;" 1000]

Result:

  * no visual differences using the wishes from 1. and 2.
  * no drawing artefacts observed in both cases

Building tktreectrl in both setups and running the explorer demo gives

  * no artefacts when scrolling horizontally for 1.
  * missing characters when scrolling horizontally for 2.

Testing the differences before [c3e68e30] and core-8-6-branch with patch applied will take some time.

A little more explanation of the main problem of [c3e68e30]:

  if (metrics.xOff > 0 && cx >= 0 && cy >= 0) { ... }

This line sorts out glyphs with a small negative x,y and affects pixel-wise
scrolling in tktreectrl. In the angled version the bound for negative coords
is at least minCoord = -1000 which wouldn't prevent these glyphs to be sorted
out (and fix the tktreectrl issue). Maybe it would be sufficient to change
the test to

  if (metrics.xOff > 0 && cx >= minCoord && cy >= minCoord) { ... }

Then there's the break condition in Tk_DrawAngledChars() at

  if ((x += metrics.xOff*cosA + metrics.yOff*sinA) > maxCoord
     || (y += metrics.yOff*cosA - metrics.xOff*sinA) > maxCoord) {
     break;
  }

which I doubt was ever tested. It would be sufficient here to sort out the
glyphs which are out of bounds but continue to try to render the remaining
glyphs instead of breaking the entire process.

> With the test snippet from [d9fdfa435d] on CentOS 6.9 x86_64 no problems seen.

This is unclear for me, sorry.

a. In a Tk version before [c3e68e30] was committed, do you see on your CentOS 6.9 x86_64 the problem reported in [d9fdfa435d]?

b. Do you see this same problem with your patch applied on top of the current core-8-6-branch?

If your answers for a. is yes and for b. is no then your patch does not regress [d9fdfa435d].

If your answer for a. is no then we don't know if your patch makes a regression for [d9fdfa435d] or not (whereas it fixes the present ticket), whatever your answer for b. is. In such a case I'm not so comfortable in changing things, because I can't see the problem and check the fixes on any of my systems: your patch fixes the present ticket but we don't know for sure whether it breaks [d9fdfa435d] again or not.

Regarding optimizations, I would prefer you to work on this rather than me since I don't know this part of the code at all. Could you take this on your shoulders perhaps?

With the test snippet from [d9fdfa435d] on CentOS 6.9 x86_64 no problems seen.
The 8.6.8 code breaks tktreectrl which worked with 8.6.7 and older releases.

By "not at all optimized" I meant:

 * nothing done to shorten the loops by breaking early as
   in the current implementation

 * take the glyph box info from the respective Xft functions
   and see if it still fits in 16 bit signed without shrinking
   this range and pick glyphs which fit but still continue with
   all remaining glyphs

Hmmm, the code you are changing was backported (by myself) from the revised_text branch in order to fix [d9fdfa435d]. I never saw the issue that this is supposed to fix though (see discussion in the mentioned ticket).

Could you perhaps check that your fix does not imply regression on [d9fdfa435d]?

Re: not at all optimized: anything you can do for this?