Improve handling toolbar tools bitmap size (PR #22488)
VZ
To continue the discussion from your comment in the other PR:
i.e. sizeOrig*int(scale) instead of using ToDIP(). Do you see any reason not to do this?
I'm more than a little confused by the code involved:
After thinking about this all again, I've realized that I couldn't really explain what was this code doing myself neither, which seems like a good reason not to keep it -- so I didn't. As you can see, I've changed it significantly now and so none of these points:
1. ~The default size is 16x15 — how is that not causing problems, but other sizes are?~ (because default size is minimum) 2. Why is the code explicitly allowing downscaling only in the very edge case of a) different tool sizes, b) with equal count of both, c) that doesn't happen to match the ideal size https://github.com/wxWidgets/wxWidgets/blob/731d29baceda04c5781c6e204cec3d9df8fe9d53/src/common/bmpbndl.cpp#L670-L677 I mean, why even bother? 3. Why is it feeding the determined size back as the ideal size next time this function runs? Should the input to GetConsensusSizeFor be `m_requestedBitmapSize` only?
should be relevant any longer.
I would make a different choice in the snippet above - in a tie, I would pick the size closer to
sizeIdeal, instead of the larger one.
I couldn't even decide what sizeIdeal was supposed to be, so I've just dropped it, making this one moot as well.
More generally speaking, I've made GetConsensusSizeFor() testable now and while the existing tests are pretty trivial, it should be easy enough to add more test cases, in particular if we find any cases where it doesn't return the expected result. And if we restore sizeIdeal parameter in the future, we'd definitely need to add tests verifying that it works as expected (which would at least force us to define what is the expected behaviour).
As for using
sizeOrig*int(scale)instead ofToDIP(), unless I'm missing something it would be incorrect:1. `sizeOrig` (aka m_defaultWidth/m_defaultHeight) is expressed in physical pixels
Just to clarify a possible misconception: no, it's expressed in logical pixels. I've agonized a long time over this, but I'm relatively confident that this was the best choice and, at the very least, it's documented as being in LPs:
wxWidgets/interface/wx/toolbar.h
Lines 904 to 905 in 34fa234
| @param size | |
| The size of the bitmaps in the toolbar in logical pixels. |
2. `ToDIP(sizeOrig)` converts it to DIP 3. In `GetConsensusSizeFor` it is immediately converted back by multiplying it by `win->GetDPIScaleFactor()` 4. (…and isn't actually used for much)
(and now not at all)
You're right that calling
SetToolBitmapSize()may be common.
Yes, see also this post.
Unless I misunderstand the problem, I would:
1. Outright deprecate `SetToolBitmapSize()`
We could do this, but it would be better not to and I think we may keep it with this PR.
2. Either ignore the value provided or only use it as hint for `GetConsensusSizeFor`, but don't `IncTo` to it (possibly only if scaling factor > 1.0)
None of this would help with actually selecting the correct bitmap size. E.g. doing this would mean that toggling the bitmap size in the toolbar sample doesn't work at all.
3. When adjusting the size, use resize, not rescale — I'm not sure if it is feasible, if this is done in win32, probably not
It would be feasible, but I don't see what's the point of having big toolbar buttons with small bitmaps in the middle of them? The current behaviour seems better to me and more consistent with the other controls.
I know I'm asking for a lot, but it would be great if you could please check how does it affect Poedit. Thanks again!
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22488
Commit Summary
- 907e4ea Add wxBitmapBundle::GetConsensusSizeFor(double) and test it
- e13b4f8 Don't force fractional scale when toolbar bitmap size is given
- 72f851f Remove the size parameter of wxBitmapBundle::GetConsensusSizeFor()
File Changes
(7 files)
- M docs/doxygen/overviews/high_dpi.md (3)
- M include/wx/bmpbndl.h (8)
- M interface/wx/toolbar.h (13)
- M src/common/bmpbndl.cpp (45)
- M src/common/bmpcboxcmn.cpp (3)
- M src/common/tbarbase.cpp (43)
- M tests/graphics/bmpbundle.cpp (31)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22488.patch
- https://github.com/wxWidgets/wxWidgets/pull/22488.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Sorry, I'm finally merging this to avoid doing it at the very last moment before the release (mid-Sunday on its eve is clearly much better), but we can still revert it before 3.1.7 if you (or anybody else) see/find any big problems with it, so please let me know if you do. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #22488 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Václav Slavík
@vslavik approved this pull request.
Looks good to me, thanks!
It would be feasible, but I don't see what's the point of having big toolbar buttons with small bitmaps in the middle of them? The current behaviour seems better to me and more consistent with the other controls.
To be honest, I guess I don't really understand the circumstances in which you'd have differently-sized bitmaps in the toolbar to begin with. I assumed it's all about gracefully handling incorrect behavior from user code (e.g. accidental 1px smaller bitmaps), in which case avoiding scaling would make sense.
I know I'm asking for a lot, but it would be great if you could please check how does it affect Poedit. Thanks again!
I didn't test this yet (I was waiting with replying until I do — sorry, didn't realize that wasn't the right call today). But Poedit doesn't explicitly set toolbar size, so it shouldn't be affected.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks for looking at this!
I'm relatively sure that this improves things for the code which does call SetToolBitmapSize() so my main worry was to not break the code not using it somehow. I really don't think it should do that, but testing would be still welcome.
As for why we're doing all this: it's to take into account a scenario when we use 150% scaling and some bitmaps are available in x1.5 size, but others are available in x2 (only). It would seem to make sense to use the scaling factor preferred by the majority of the bitmaps in such a case, I think.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalph
3 tool bitmaps (in this order)
18x18
16x16
24x24
No call to SetToolBitmapSize
#1 & #2 scaled to 24x24
SetToolBitmapSize(16x16)
#1 & #3 scaled to 16x16
SetToolBitmapSize(16x15)
#1 & #2 scaled to 24x24
SetToolBitmapSize(16x18)
#1 & #2 scaled to 24x24
SetToolBitmapSize(18x18)
#2 & #3 scaled to 18x18
SetToolBitmapSize(32x30)
#1 #2 & #3 scaled to 32x30
SetToolBitmapSize(32x-1) or SetToolBitmapSize(32x18)
#1 & #2 scaled to 24x24
SetToolBitmapSize(32x25)
#1 #2 & #3 scaled to 32x25
There were no differences between the PR and 3.1.6.
However, putting on my novice wxWidgets hat, this is not what I expected based on the documentation. As long as the dimensions set are less than the largest bitmap, then the call has no effect unless it exactly matches one of the existing bitmaps, in which case it sets/scales all the bitmaps to that size. In this case the documentation stating "Sets the default size of each tool bitmap." is confusing because it will have no effect if the dimensions are smaller than the largest bitmap unless it exactly matches one of the smaller bitmap sizes. If, however, both dimensions are larger than all of the bitmaps, then all of the bitmaps are scaled.
I am by no means suggesting changing the behaviour, since that would break existing code, but I do think the docs for this could be changed (for 3.2.0) to more accurately reflect when the bitmaps will be changed and when the call will essentially be ignored.
BTW, one valuable use for this function is to standardize on an existing bitmap size. For example, you might plan for all of your bitmaps to be 22x22. You then accidentally create one bitmap at 24x24. If you called SetToolBitmapSize(22x22), then only one bitmap gets scaled. If you didn't call it, then all but one of the bitmaps get scaled to 24x24.
--
You received this message because you are subscribed to the Google Groups "wx-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to wx-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/wx-dev/wxWidgets/wxWidgets/pull/22488/c1146799227%40github.com.
Vadim Zeitlin
R> I created at toolbar with three different size bitmaps, and checked with
R> the PR and 3.1.6 on Windows 10 with at 100% (no scaling):
Thanks for testing!
R> 3 tool bitmaps (in this order)
R> 18x18
R> 16x16
R> 24x24
R>
R> No call to SetToolBitmapSize
R> #1 & #2 scaled to 24x24
This seems logical. Not ideal, of course, but when SetToolBitmapSize() is
called, this is what you're requesting.
R> SetToolBitmapSize(16x16)
R> #1 & #3 scaled to 16x16
I think this one is already a bug. I have somehow forgotten about it, but
originally SetToolBitmapSize() wasn't supposed to decrease the bitmaps
size, only increase it. I.e. I think that in 3.1.5 (before wxBitmapBundle
changes in 3.1.6) this would have used 24x24 size, which is probably
better.
R> SetToolBitmapSize(16x15)
R> #1 & #2 scaled to 24x24
This one is definitely a bug, it should have the same effect as 16x16, but
probably doesn't because the code considers that the bitmap size hasn't
changed (as 16x15 is the default).
R> SetToolBitmapSize(16x18)
R> #1 & #2 scaled to 24x24
This one is also a bug, but I don't understand at all why does it happen
because with the latest code (i.e. from this PR, which has been already
merged in master in the meanwhile), the bitmaps really should have exactly
the requested bitmap size... The code is really very simple now and if
m_requestedBitmapSize is given, it's simply used.
I also can't reproduce this in the toolbar sample: if I replace the
argument of the SetToolBitmapSize() call there with wxSize(16, 18), the
icons appear as (very ugly) 16x18 ones.
R> SetToolBitmapSize(18x18)
R> #2 & #3 scaled to 18x18
R>
R> SetToolBitmapSize(32x30)
R> #1 #2 & #3 scaled to 32x30
R>
R> SetToolBitmapSize(32x-1) or SetToolBitmapSize(32x18)
R> #1 & #2 scaled to 24x24
R>
R> SetToolBitmapSize(32x25)
R> #1 #2 & #3 scaled to 32x25
Those look either fine or similar to SetToolBitmapSize(16x18) case.
R> *There were no differences between the PR and 3.1.6.*
Thanks again for confirming this, but I actually think we may have broken
the old behaviour in 3.1.6 already. OTOH I'm not sure if it's something
that matters enough to justify changing it again.
R> However, putting on my novice wxWidgets hat, this is *not *what I expected
R> based on the documentation. As long as the dimensions set are less than the
R> largest bitmap, then the call has no effect unless it exactly matches one
R> of the existing bitmaps, in which case it sets/scales all the bitmaps to
R> that size. In this case the documentation stating "Sets the default size of
R> each tool bitmap." is confusing because it will have no effect if the
R> dimensions are smaller than the largest bitmap unless it exactly matches
R> one of the smaller bitmap sizes. If, however, both dimensions are larger
R> than all of the bitmaps, then all of the bitmaps are scaled.
I don't think it's the documentation problem, or at least not only. I'm
not sure if specifying the size smaller than the bitmap size should work or
not, but I'm pretty sure that if it does, then it should work for all sizes
and not just those that correspond to the bitmap sizes.
R> BTW, one valuable use for this function is to standardize on an existing
R> bitmap size. For example, you might plan for all of your bitmaps to be
R> 22x22. You then accidentally create one bitmap at 24x24. If you called
R> SetToolBitmapSize(22x22), then only one bitmap gets scaled. If you didn't
R> call it, then all but one of the bitmaps get scaled to 24x24.
The problem is that this function is really incompatible with the idea of
wxBitmapBundle providing the bitmaps of the needed size on request. E.g. if
you have 32, 48 and 64px versions of your bitmaps, then everything should
work pretty good at all reasonable DPI scaling levels (we'll use 32 instead
of 40 that we should really use at 125% and 48 instead of 56 at 175% but,
frankly, this is almost unnoticeable).
However if you call SetToolBitmapSize(32, 32), now you're not going to be
using 48px version at all because SetToolBitmapSize() will force using
either 100% or 200% scaling (before the changes of this PR it was worse,
because it would have forced the use of 150% scaling even if you did not
have 48px bitmaps, so you'd be seeing ugly fractionally upscaled images).
We could improve this by checking if 48px is available in all (or the
majority of?) the bundles, but this would make things more complicated and
still not cover the other cases, e.g. 175% would not be available, but we
wouldn't use 150% neither.
Regards,
VZ
Randalph
R> SetToolBitmapSize(16x18)
R> #1 & #2 scaled to 24x24
This one is also a bug, but I don't understand at all why does it happenbecause with the latest code (i.e. from this PR, which has been alreadymerged in master in the meanwhile), the bitmaps really should have exactlythe requested bitmap size... The code is really very simple now and ifm_requestedBitmapSize is given, it's simply used.
Vadim Zeitlin
R> Hmm, I'm not sure why building from the PR didn't change this, but now
R> that I've picked up the merged code, in 3.1.7 it scales all 3 to 16x18,
R> whereas in 3.1.6, it scales #1 and #2 to 24. Ditto for calling this with
R> 16x15 -- it does indeed scale all 3 in 3.1.7 whereas 3.1.6 scales to 24.
R> That seems to be specific to 3.1.6 -- I tried this out in a different
R> application using an older version of wxWidgets, and it did indeed scale
R> to 16x15. Sorry for the false alarm about the PR -- I clearly did
R> something wrong when I rebuilt the library from that branch.
Thanks for rechecking, I was really worried I was missing something
fundamental here. It's a pity that we can't create window mock ups with the
given DPI scaling factor, as it means that we can't have any automatic
tests for this stuff. I wonder if it could be worth it to have some kind of
WX_DPI_SCALING environment variable that could be set to override the real
scaling factor and be used for testing.
Anyhow, to return to wxToolBar, I'm pretty sure that 3.1.6 was wrong,
which is why I've started changing it so late before the release, so it's
good that the behaviour is different now. I'm still not sure if the new
behaviour is 100% correct, but I'm not going to change it any more right
now, let's see if anybody complains about SetToolBitmapSize() unexpectedly
resizing the bitmaps to a smaller size. My feeling is that at least if it
happens, it should be relatively simple to realize what's going on -- and,
hopefully, remove SetToolBitmapSize() or <bitmapsize> from XRC entirely.
Thanks again for testing,
VZ