Tk Source Code

View Ticket
Login
Ticket UUID: 3c6660b6f0bed337b8a40b3335f8f5469ec9d842
Title: Wrong ttk::checkbutton and ttk::radiobutton scaling on Windows 10
Type: Bug Version: 8.6.9
Submitter: nemethi Created on: 2020-05-18 19:39:59
Subsystem: 88. Themed Tk Assigned To: bll
Priority: 5 Medium Severity: Severe
Status: Closed Last Modified: 2020-05-28 20:47:30
Resolution: Fixed Closed By: fvogel
    Closed on: 2020-05-28 20:47:30
Description:
Environment: Windows 10 64-bit, with 150 % display scaling, ActiveState Tk 8.6.9.
Test script:

ttk::style layout My.TCheckbutton { Checkbutton.indicator }
  
proc createCheckbutton w {
    set ::$w 1
    ttk::checkbutton $w -text Checkbtn ;# -style My.TCheckbutton
    pack $w -padx 20 -pady 10
}

. configure -background white
createCheckbutton .ck1
after 1000 [list createCheckbutton .ck2]

The script creates a ttk::checkbutton, and 1000 ms later a second one.  Notice that for the first test the -style option is commented out.  The resulting checkbuttons are displayed with the wrong indicator size:  The squares are 13 x 13 pixels small, which is the size corresponding to the scaling level 100 %.  For 150 % scaling the size of both squares should be 20 x 20 pixels.  In addition, strangely enough, the overall width of the second checkbutton is 7 pixels smaller than that of the first one.

For the second test remove the ";#" preceding the -style option.  This time both checkbuttons consist of the indicator element only.  As a result, the first checkbutton appears with the right square size of 20 x 20 pixels, while the second square is again 13 x 13 pixels small.

With both tests, the drawing of the incorrectly sized indicator elements is in addition broken.

The first attached screenshot shows the results of the two tests described above, while the second one compares the incorrectly sized and drawn indicator for 150 % scaling with the correctly sized and drawn indicator for 100 % scaling.
.
The ttk::radiobutton widget exhibits quite similar problems, which can be reproduced with the following script:

ttk::style layout My.TRadiobutton { Radiobutton.indicator }
  
proc createRadiobutton w {
    set ::selectedButton .rb1
    ttk::radiobutton $w -text Radiobtn -value $w ;# -style My.TRadiobutton
    pack $w -padx 20 -pady 10
}

. configure -background white
createRadiobutton .rb1
after 1000 [list createRadiobutton .rb2]
User Comments: fvogel added on 2020-05-28 20:47:30:
I think I'm convinced :-) although I couldn't test myself.

This is now merged in core-8-6-branch and trunk.

Many thanks to both of you!

nemethi (claiming to be Csaba Nemethi) added on 2020-05-28 14:36:03:
Just for completeness:  I have been able to reproduce the reported problems on an ancient laptop running Windows XP, too.  And I can confirm that the bug-fix consisting in passing NULL as device context handle to GetThemePartSize() eliminates these problems not only for the vista theme, but also for xpnative.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-25 20:09:53:
Yes, the resulting size is always 13 x 13, regardless of whether hDC is NULL or not.

fvogel added on 2020-05-25 19:37:54:
Thank you for this test. Did you also log the same thing with 100% scaling? Just double-checking...

nemethi (claiming to be Csaba Nemethi) added on 2020-05-25 19:19:31:
Sorry, in the 2nd case the number of GenericElementSize() invocations is 1 + 2 less than in the first case.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-25 19:14:59:
I have inserted a few logging calls into the body of the original version of the GenericElementSize() function and then ran my test script with 150 % scaling.  Here are the checkbutton-related lines of the log:

Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x31010b49
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x31010b49
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x35010b49
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x35010b49
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x35010b49
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x8f011153
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x8f011153
  size.cx = 13  size.cy = 13

After that I removed the ";#" preceding the -style option and ran the script again, with the following result:

Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x0
  size.cx = 20  size.cy = 20
Checkbutton.indicator:  elementData->hDC = 0x7001099d
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x7401099d
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0x7401099d
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0xec0102c9
  size.cx = 13  size.cy = 13
Checkbutton.indicator:  elementData->hDC = 0xf00102c9
  size.cx = 13  size.cy = 13

In both cases, the size is correct (20 x 20) if hDC is NULL and wrong (13 x 13) otherwise.  In the 2nd case (where the checkbuttons consist of the indicator element only) the number of GenericElementSize() invocations is 1 + 1 less than in the first case (and the upper checkbutton appears correctly sized).

nemethi (claiming to be Csaba Nemethi) added on 2020-05-25 09:40:19:
Some more examples of calling GetThemePartSize() with NULL as 2nd argument:

https://stackoverflow.com/questions/8775428/what-uxtheme-function-i-must-use-to-get-the-default-size-of-the-minimize-maximi

https://stackoverflow.com/questions/14111926/how-to-retrieve-the-size-of-the-minimize-maximize-and-close-button-of-a-window

https://www.codota.com/code/java/methods/org.eclipse.swt.internal.win32.OS/GetThemePartSize

https://www.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=&ved=2ahUKEwj3uarK0s7pAhUGXRoKHf-KDYg4ChAWMAN6BAgEEAE&url=http%3A%2F%2Fwww.reactos.freedoors.org%2FReactos%25200.4.2%2FReactOS-0.4.2-src%2Fdll%2Fwin32%2Fcomctl32%2Ftheme_scrollbar.c&usg=AOvVaw1dzToNjhQ5rz5Cp7nJl5yf

https://geek-tips.github.io/articles/587580/index.html

fvogel added on 2020-05-25 06:16:43:
The hDC is a handle on a device context (something onto which drawing occurs).

In Tk it is used in ttkWinXpTheme.c, and set in InitElementData() only. I see that such a new "Drawable" is created in DrawWidget() each time a ttk widget is redrawn, so I guess it's consistent if it changes "all the time" as you say.

Another note: If the NULL hdc is the correct fix, perhaps the IGNORE_THEMESIZE thing could be removed as well since that NULL could fix this issue as well. Worth to be checked I think.

bll added on 2020-05-24 22:44:44:
I don't know what an hDC is, but it seems to be constantly changing all the time.

fvogel added on 2020-05-24 21:51:51:
Hmmm, perhaps...

Brad or Csaba, I don't have access to either Win 7 or Win 10 at the moment, meaning I cannot test anything related to this ticket. Could you please check whether the value of elementData->hDC changes between two calls of GetThemePartSize(), which could explain the difference in the returned size?

bll added on 2020-05-24 21:00:28:
Remember, no one uses vsapi any more, any internet information is from four+
years ago, and finding solutions on the internet is difficult.

I have updated the branch with the new solution.

I am not seeing any size changes other than cb/rb/treeview, looks ok to me.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-24 19:48:05:
You can see an example for a GetThemePartSize() invocation with NULL as the 2nd argument towards the end of the page https://trac.wxwidgets.org/ticket/17290 . That example also uses TS_DRAW instead of TS_TRUE, but that doesn't seem to be relevant in this context (I have tested this, too).

Why the NULL value solves *this* problem, and whether other element sizes could become wrong with it, is a good question.  I agree with you that this should be investigated so we can be sure that the fix won't break anything.

BTW, with this NULL value also the treeview indicator element will scale as expected.

fvogel added on 2020-05-24 18:54:24:

the drawback of the patch proposed by Brad [...] would be no big problem, being that, AFAIK, there is no notification in Tk that would enable an application to react to DPI scaling changes

Yes I'm aware of this. But I thought the patch would block the way to implement such a notification and follow the changes. I'm probably wrong on this tough, after all if such a hook would exist it would suffice to reset elementData->fetched.

In the invocation of the elementData->procs->GetThemePartSize() function replace the 2nd argument (elementData->hDC) with NULL. With this change the checkbuttons and radiobuttons of my test scripts will appear correctly sized, and the test script from the other ticket works as expected, too.

Aha. Very very interesting. So this finding could well explain why there are no (to my knowledge) reports on the internet about that same issue in other software than Tk: are we really simply misusing GetThemePartSize()?

I'm interested in how you found out that not providing a handle on a device context (an hDC) when calling GetThemePartSize() could fix the issue? The MS documentation does not say that hDC may be NULL, and even less does it tell what would be the effect of a NULL here. And I didn't find anything about this topic on the internet, but I surely didn't search thoroughly enough since you must have found it.

Also, could there be some side effects to this NULL value? Other elements sizes that could become wrong with this NULL parameter, for example? While this change would be consistent with the lack of other reports about GetThemePartSize() providing wrong sizes when called more than once, I'm not very comfortable in making this change without understanding exactly what the implications are.

This makes me think: from your finding with such a NULL value, it would also be possible that the value of elementData->hDC changes between two calls of GetThemePartSize(), which could explain the difference in the returned size. So instead of putting a NULL there (undocumented value with unknown potential regressions implied - AFAIK), wouldn't it be best to check this hypothesis first, and if elementData->hDC has changing values to understand why they are changing, and finally fix this?


nemethi (claiming to be Csaba Nemethi) added on 2020-05-24 17:37:10:
On Windows 7 the user is required indeed to log out and in again so the scaling change can take effect.  On Windows 10 this is no longer needed for "standard" scaling percentages like 125 % or 150 %.  But even on Windows 10, the drawback of the patch proposed by Brad (namely that it "will prevent widget size updates when the screen DPI changes") would be no big problem, being that, AFAIK, there is no notification in Tk that would enable an application to react to DPI scaling changes.

That said, I think we don't need Brad's patch at all.  I seem to have found a quite simple solution, which consists of a single change:  In the invocation of the elementData->procs->GetThemePartSize() function replace the 2nd argument (elementData->hDC) with NULL.  With this change the checkbuttons and radiobuttons of my test scripts will appear correctly sized, and the test script from the other ticket works as expected, too.

bll added on 2020-05-24 15:01:30:
Tested the treeview indicator also, and I have added FETCH_ONCE for the
treeview indicator to the bugfix branch.

bll added on 2020-05-24 14:43:00:
Yes, apparently, the bug is out there in the wild (treeview, 2016).
It doesn't really surprise me, the code may be using GetThemePartSize() 
in a method that may not be common.

https://stackoverflow.com/questions/38772670/ctreectrl-with-explorer-theme-not-dpi-aware

The user is required to log out to change the display setting.  Size changes
are not an issue.

fvogel added on 2020-05-24 10:57:58:

I think the patch if OK. I have just made a minor change, see [824c2c6a].

Random thoughts:

- Isn't it strange we have to do this? A windows bug, yes, probably. Are we aware of any other report on the internet around the same thing? Are we really alone to hit this GetThemePartSize() bug?
- The patch will prevent widget size updates when the screen DPI changes (and this is a user setting). Are we comfortable with this? We seem to have no other choice, having properly scalable elements sizes seems more important. Do you agree with this?


bll added on 2020-05-22 03:21:07:
Looks ok to me also.
I will assign to François for review and merge.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-21 13:27:40:
Tested the fix successfully for the scaling levels 100 %, 125 %, 150 %, 175 %, and 200 %.  The corresponding indicator heights are 13, 16, 20, 20, and 26 px.  For me it is somewhat strange that the indicator for 175 % display scaling has the same size as that for 150 % (for 175 % scaling, 23 px would be more logical than 20 px), but this inconsistency is a Windows issue and has absolutely nothing to do with Tk.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-20 16:43:00:
Many thanks, your patch looks OK.  I will test it ASAP

bll added on 2020-05-20 15:44:55:
Added bug fix branch: bug-3c6660b6

Please review ttkWinXPTheme.c

bll added on 2020-05-20 15:38:59:
My best guess at this point is that it is a windows bug, and
calling GetThemePartSize() more than once on certain vsapi elements does
not work properly.  I put in some awful code to keep the initial
size fetch (which would not work properly due to padding changes), 
and everything sizes properly.

I shall rewrite the code properly and see how things work.

bll added on 2020-05-20 14:45:16:
Well, that was the wrong chunk of code from the wrong file.

Looking in the right place...

It looks like the fetch of the initial size of the checkbutton is correct, 
it returns 20 at first, but something else somewhere is resetting the 
size to be 13.

In some cases, the height and width stay stable, even though the fetch
returns the size 13 (post initial fetch).
  got w:13 h:13
  h: 20 w: 20
In most other cases the height and width are not stable.

bll added on 2020-05-19 16:15:02:
Right, whereas the sizing is consistent, the size itself is incorrect.

Added comparison images against what the checkbutton size should be (using control panel/sound).

Occasionally, the checkbutton is drawn in the correct size.

It is interesting (i.e. totally weird) that using the original test script,
the correctly sized checkbutton stays the correct size, but using the other
test script, the mouse-over causes the correctly sized button to revert.

The code has:

    { "Checkbutton.indicator",
        DFC_BUTTON, DFCS_BUTTONCHECK, FIXEDSIZE(13), FIXEDSIZE(13),
        checkbutton_statemap, {0,0,4,0} },

Found some code on the internet, though I'm not entirely sure that 
this code is compatible:
( https://devblogs.microsoft.com/oldnewthing/20171201-00/?p=97505 )

  // If there is a theme, then ask it for the size
  // of a checkbox and use that size.
  if (htheme) {
    SIZE siz;
    GetThemePartSize(htheme, hdcScreen,
      BP_CHECKBOX, CBS_UNCHECKEDNORMAL, nullptr,
      TS_DRAW, &siz);
    cx = siz.cx;
    cy = siz.cy;
  }

This may be a better way to set the size of the check/radio button.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-19 14:37:48:
Are you sure your statement "Unable to re-create with 8.6.10 on windows 7." is correct?  As seen in image cb-a.png, the indicator square of both checkbuttons has the wrong size 13 x 13 (for 150 % scaling it should be 20 x 20 pixels).  For me, this reproduces the problem on Windows 7, too.

bll added on 2020-05-19 14:03:32:
Using the alternate styling in the original script, the problem does
show up (windows 7, 150%, 8.6.10).
Image: cb-d.png

bll added on 2020-05-19 13:59:03:
The test script from the other ticket:

package require Tk 
set rb rbNone 
ttk::radiobutton .rb1 -text "Radiobutton 1" -variable rb -value rb1 
ttk::radiobutton .rb2 -text "Radiobutton 2" -variable rb -value rb2 
grid .rb1 
grid .rb2 

shows that the different sized radiobutton problem is still extant.
The second button is scaled larger, but when mouse-over occurs shrinks
to an incorrect size.

windows 7, 150%, 8.6.10

Images: rb-b.png rb-c.png

bll added on 2020-05-19 13:52:27:
Unable to re-create with 8.6.10 on windows 7.
Note that something is wrong with the padding on the second checkbutton.
(See image cb-a.png).

nemethi (claiming to be Csaba Nemethi) added on 2020-05-19 12:38:25:
The problems reported here can be reproduced on Windows 7, too.  Also, it makes no difference whether a 64-bit or 32-bit Tk is being used.  The Tk version is not relevant either.  The problems seem to be specific to the "vista" theme (not sure about "xpnative").

I have set the severity to "Severe" for good reason:  More and more laptops have relatively small displays with a high (at least full HD) resolution and have Windows 10 pre-installed, with 125 % or 150 % scaling.  Scalability of applications on such systems is a must.

bll added on 2020-05-18 21:19:41:
See also:

https://core.tcl-lang.org/tk/tktview/601cead1d05b0d641f65

https://core.tcl-lang.org/tk/tktview/4b50b7602849f327892a

I don't think anyone has come up with a resolution for this problem.

nemethi (claiming to be Csaba Nemethi) added on 2020-05-18 19:45:52:
Here are the links for the two screenshots:

https://www.nemethi.de/vista_scaling/checkbtn1.png

https://www.nemethi.de/vista_scaling/checkbtn2.png

Attachments: