Tk Source Code

Example:

% entry .e -borderwidth 3m
.e
% pack .e
% .e cget -borderwidth
11

We can see that the advise in the documentation is not followed here. Therefore the number of millimeters is converted to pixels. If - later - the scaling factor of the screen changes, the borderwidth of the entry will not scale correctly.

Solution: Add `Tcl_Obj *` fields to the structures to store the unmodified value.

Now [788c1c6615751bc0|fixed] for listbox too.

>> In tkText.h, line 775, why is heightObj a Tcl_Obj* but width still an int?
>> If correct, this looks asymmetrical.
>
> The reason for this is that width is counted in characters while the height
> is counted in pixels. Might be worth a separate ticket, I don't want to
> touch/change this as part of this ticket.

OK but then the comment on line 769-770 (in trunk, now) has become incorrect for the height. This is now misleading and should be fixed I think.

>> I don't understand the BUILD_tk thing at all
>
> Let me explain. Fields like padX are cached versions of padXObj. It's not a
> good idea to cache this value: If the scaling changes it must be re-calculated,
> otherwise the screen-distance is not accurate any more.
>
> Using BUILD_tk means that those fields are only visible when building Tk
> (On any platform, not only Windows), not when building extensions (which
> shouldn't use these internal values anyway). So, just read them as a kind
> of 'private' fields.

Thanks for the explanation. I didn't notice this BUILD_tk thing was already existing before your changes. I believe its use should be documented but I don't know where, perhaps in the makefiles.

I continue to observe it seems to be used (defined) when building Tk on Windows and Linux but not on macOS. There is no occurrence of BUILD_tk in the ./macos directory. What am I missing?


Last thing: could you please merge trunk into revised_text? This looks non-trivial to me, there are lots of conflicts. Thanks!
</p>


jan.nijtmans added on 2024-08-11 13:53:08:

Merged to trunk now.



jan.nijtmans added on 2024-08-11 13:52:29:

> I don't understand the BUILD_tk thing at all

Let me explain. Fields like padX are cached versions of padXObj. It's not a good idea to cache this value: If the scaling changes it must be re-calculated, otherwise the screen-distance is not accurate any more.

Using BUILD_tk means that those fields are only visible when building Tk (On any platform, not only Windows), not when building extensions (which shouldn't use these internal values anyway). So, just read them as a kind of 'private' fields.



jan.nijtmans added on 2024-08-09 08:54:49:

> Last but not least: the test suite hangs for me on Windows during spinbox-8.4

Found that bug! Fixed now.

> ... Why isn't it the same also in tkEntry.c with ...

Fixed now as well

> In tkText.h, line 775, why is heightObj a Tcl_Obj* but width still an int? If correct, this looks asymmetrical.

The reason for this is that width is counted in characters while the height is counted in pixels. Might be worth a separate ticket, I don't want to touch/change this as part of this ticket.

> About the changed test cases: .e configure -borderwidth 1.3 ; .e cget -borderwidth now returns the given value (1.3), which is the goal. But does this really make sense since this value represents pixels?

Yes, it actually makes sense: Some screens even have sub-pixels ... why not?

> Why is the patch testing for things like TCL_MAJOR_VERSION > 8 or TCL_MAJOR_VERSION < 9 at several places since the patch is against trunk (Tcl 9) only? Was any other case than Tcl 9 tested?

Tk 9 runs/compiles with Tcl 8.7 too. But testing TCL_MAJOR_VERSION  indeed doesn't make much sense. Removed now.



jan.nijtmans added on 2024-08-08 15:39:59:

> When given <0, the insertWidth for entries seems to be changed by the patch from 2 to 0. See tkEntry.c line 1194

That deserves a [Separate ticket]. More is going on, separate from this ticket.



fvogel added on 2024-07-22 19:49:44:

Reviewed. Here are some comments:

I don't understand the BUILD_tk thing at all.


BUILD_tk seems to be defined when building on Windows and Linux but not on macOS. Is this correct? I suggest we schedule a CI run on all platforms before merging (I'm not doing it now because of my last bullet point below).


When given <0, the insertWidth for entries seems to be changed by the patch from 2 to 0. See tkEntry.c line 1194


In tkFrame.c, Tcl_DecrRefCount(framePtr->highlightWidthObj); is guarded with an if (framePtr->highlightWidthObj), and this is the same in many other changed places.  Why isn't it the same also in tkEntry.c with Tcl_DecrRefCount(entryPtr->insertWidthObj); (line 1195), and two other similar occurrences just below? Are we sure these pointers are never NULL? Same issue in tkTextTag.c line 369.


In tkText.h, I think Tcl_Obj *padXObj, *padYObj; are identically declared whether TCL_MAJOR_VERSION > 8 or TCL_MAJOR_VERSION < 9. Couldn't this be simplified (factorized out) then? There are two occurrences of this (TkTextEmbWindow and TkTextEmbImage).


In tkText.h, line 775, why is heightObj a Tcl_Obj* but width still an int? If correct, this looks asymmetrical.


Why is the patch testing for things like TCL_MAJOR_VERSION > 8 or TCL_MAJOR_VERSION < 9 at several places since the patch is against trunk (Tcl 9) only? Was any other case than Tcl 9 tested?


About the changed test cases: .e configure -borderwidth 1.3 ; .e cget -borderwidth now returns the given value (1.3), which is the goal. But does this really make sense since this value represents pixels?


Last but not least: the test suite hangs for me on Windows during spinbox-8.4


Given the above I will refrain from merging to trunk. I'm bouncing the ticket back to you, Jan.



jan.nijtmans added on 2024-07-12 06:49:38:

François, I did some more work on the bug-23ba539501 branch: All testcases are now working.

Please review, if you are OK, please merge it to trunk.

What's not done yet is correct the caching, but I made a [separate] ticket for that.



jan.nijtmans added on 2024-07-08 22:09:38:

Correction (I was confused with another branch)

The bug-23ba539501 works, but is not done yet for all widgets. Also, for several fields it introduces both a Tcl_Obj field and a pixel field. Experimenting more shows that it's better to use a Tcl_Obj field only, that would make the cashing of the scaling factor simpler. But that's a large rewrite which I don't want to delay Tk 9.0 for. So, better not merge the branch yet.



jan.nijtmans added on 2024-07-07 20:32:06:

Branch bug-23ba539501 is a fix attempt, but it doesn't work as expected. Still some debugging to be done, but I won't have time to look at that any time soon. Low prio for 9.0, just leave it open. Don't merge the non-working branch!



fvogel added on 2024-07-07 20:13:30:

Is your branch bug-29ba539501 ready to be merged or is there something remaining to be done?



jan.nijtmans added on 2024-06-07 07:31:10:

Proposed solution [cbe80fc5ce4cd326|here]