[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [lmi] Latent lmi anomaly unmasked by new wx
|
From: |
Vadim Zeitlin |
|
Subject: |
Re: [lmi] Latent lmi anomaly unmasked by new wx |
|
Date: |
Wed, 15 Jul 2020 16:48:33 +0200 |
On Wed, 15 Jul 2020 14:19:35 +0000 Greg Chicares <gchicares@sbcglobal.net>
wrote:
GC> I've upgraded wx{Widgets,PdfDoc}, and all automated GUI tests pass.
GC> The only difference I've noticed so far is that a particular anomaly
GC> is reported: some fields are specified as too narrow in XRC files.
Yes, I'm really sorry, I should have mentioned this, but I didn't want to
mix up everything together, so I didn't do it in my initial post and then
forgot to make a separate one about it.
GC> To reproduce, run lmi from the command line, and do:
GC> File | New | Illustration
GC> Formerly, there was no message on the console.
Yes, because the (potential) problem was silently ignored. We've decided
to add them to indicate that something might be wrong in wxWidgets commit
1be43ed67b (Improve best size determination for wxSpinCtrl, 2019-10-03).
GC> Now I observe:
GC>
GC> wxSpinCtrl "SolveBeginYear": initial width 20 is too small, at least 45
pixels needed.
GC> wxSpinCtrl "SolveBeginAge": initial width 20 is too small, at least 45
pixels needed.
GC> wxSpinCtrl "SolveEndYear": initial width 20 is too small, at least 45
pixels needed.
GC> wxSpinCtrl "SolveEndAge": initial width 20 is too small, at least 45 pixels
needed.
GC> wxSpinCtrl "SolveTargetYear": initial width 20 is too small, at least 45
pixels needed.
GC> wxSpinCtrl "SolveTargetAge": initial width 20 is too small, at least 45
pixels needed.
GC>
GC> at 96 DPI, or at 192 DPI, the same set of messages, but with
GC> ... initial width 40 is too small, at least 91 pixels needed.
GC> i.e., {40,91} instead of {20,45}.
This is as expected, because all sizes in XRC are specified in so-called
"DIP"s, i.e. device-independent pixels, which are the same as "pixels at 96
DPI", i.e. all pixel sizes in XRC are automatically scaled by the DPI
scaling factor.
GC> I imagine this means that 20 pixels is too narrow for a spinctrl
GC> (as seems quite plausible), and that formerly wxXRC fixed the
GC> width silently, whereas now it advises us (usefully) of that error.
Almost: it didn't fix anything before, i.e. the actual behaviour didn't
change. What happened then, and still happens now, is that the controls are
created with too small size, but were later resized to be (much) bigger, as
they're inside a wxSizerItem with wxGROW flag. So there was no real problem
and there still isn't now.
However wxSpinCtrl doesn't know if it's going to be resized later or not
when it's created, so it seemed better to err on the side of caution and
warn about it being possible too small, just in case it wouldn't be
enlarged later, which is why we've decided to add this warning.
In my forgotten message about this problem, I was going to recommend the
following fix for it: just remove all <size>20,-1</size> lines from
skin.xrc. They're completely unnecessary and it would be better to just let
the control determine its size automatically (which it does depending on
its range, which is correctly specified in XRC) in any case, but
considering that the control will be expanded to fill all the available
sizer space anyhow, it's doubly unnecessary. Below[*] is a trivial patch
doing this, but you could also just do "%g/<size>20,-1<\/size>/d" in Vim as
I did.
GC> I'm not going to fix it immediately, because introducing a new
GC> commit now changes the base commit
This is not going to create any problems for the future merge, so I'm not
going to rebase anything as you would still be able to merge the current PR
without any problems.
If you'd like to understand why is this so, instead of just taking my word
for it, let me try to explain why: I've only rebased the PR yesterday
because the commit updating wxWidgets and wxPdfDoc versions doesn't really
belong to this PR, it only was included in them because later commits
simply wouldn't compile without it (because they use new wxWidgets API
which didn't exist in the wxWidgets version previously used by lmi). So,
after you've cherry-picked this commit in master, I could rebase the PR on
the latest master to exclude it commit and leave only the "real" changes in
it. But this logic doesn't apply for any future changes, i.e. if you make
some changes to skin.xrc right now, there is no need to rebase the PRs
again. It could be done, but nothing would be really gained from it.
When you merge the PR branch later, it will just result in the picture I
drew yesterday, with a merge commit with 2 parents: the latest lmi master
(whatever it is by then) and the latest commit from the PR. And this is
perfectly fine and doesn't create any problems at all, as long as there are
no conflicts, i.e. as long as there are no changes in master to the same
lines that were also changed by the PR (i.e. basically census_view.?pp
files).
So, again, please don't hesitate to make whatever changes you see fit in
master, this won't have any negative effects on the merge later (unless,
again, you change the code in census_view.?pp, which would almost certainly
result in conflicts).
Please let me know if you have any questions about this,
VZ
[*] Patch below:
diff --git a/skin.xrc b/skin.xrc
index 3b7f0595b..8d63930af 100644
--- a/skin.xrc
+++ b/skin.xrc
@@ -2008,7 +2008,6 @@ here, but it looks weird if we don't make this look like
its siblings.
<flag>wxGROW</flag>
<object class="wxSpinCtrl"
name="SolveBeginYear">
<help>Solve beginning
duration</help>
- <size>20,-1</size>
<style>wxSP_ARROW_KEYS</style>
<max>100</max>
<value>99</value>
@@ -2024,7 +2023,6 @@ here, but it looks weird if we don't make this look like
its siblings.
<flag>wxGROW</flag>
<object class="wxSpinCtrl"
name="SolveBeginAge">
<help>Solve beginning
age</help>
- <size>20,-1</size>
<style>wxSP_ARROW_KEYS</style>
<max>100</max>
<value>99</value>
@@ -2069,7 +2067,6 @@ here, but it looks weird if we don't make this look like
its siblings.
<flag>wxGROW</flag>
<object class="wxSpinCtrl"
name="SolveEndYear">
<help>Solve ending
duration</help>
- <size>20,-1</size>
<style>wxSP_ARROW_KEYS</style>
<max>100</max>
<value>99</value>
@@ -2085,7 +2082,6 @@ here, but it looks weird if we don't make this look like
its siblings.
<flag>wxGROW</flag>
<object class="wxSpinCtrl"
name="SolveEndAge">
<help>Solve ending
age</help>
- <size>20,-1</size>
<style>wxSP_ARROW_KEYS</style>
<max>100</max>
<value>99</value>
@@ -2130,7 +2126,6 @@ here, but it looks weird if we don't make this look like
its siblings.
<flag>wxGROW</flag>
<object class="wxSpinCtrl"
name="SolveTargetYear">
<help>Duration to
achieve target surrender value</help>
- <size>20,-1</size>
<style>wxSP_ARROW_KEYS</style>
<max>100</max>
<value>99</value>
@@ -2146,7 +2141,6 @@ here, but it looks weird if we don't make this look like
its siblings.
<flag>wxGROW</flag>
<object class="wxSpinCtrl"
name="SolveTargetAge">
<help>Attained age to
achieve target surrender value</help>
- <size>20,-1</size>
<style>wxSP_ARROW_KEYS</style>
<max>100</max>
<value>99</value>
pgpOzykHgDIUB.pgp
Description: PGP signature