GetSizeFromTextSize() priorities

[Manolo]

Hi
In the next minutes I'm going to submit a patch implementing
GetSizeFromTextSize() in wxComboCtrl.

As usual, I found some bugs like:
-> Height of arrow is, for some cases, less than control's height.
-> Changing the font does not invalidate cached sizes.
-> wxVListBoxComboPopup::CalcWidths() is not optimized, in the
sense of using wxTextMeasure optimizations.
-> The combopop does not use a right space between lines.
-> Margins are set, but they are not used at all.

What are the priorities?
Do I fix these bugs (and some others I'm sure they will expose),
or do I continue with wxSpinCtrl and wxSearchCtrl?

TIA,
Manolo

[Vadim Zeitlin]

On Wed, 14 Nov 2012 01:26:34 +0100 Manolo wrote:

M> In the next minutes I'm going to submit a patch implementing
M> GetSizeFromTextSize() in wxComboCtrl.

Thanks! FWIW I'm applying the wxMSW part of this patch right now.

M> As usual, I found some bugs like:
M> -> Height of arrow is, for some cases, less than control's height.
M> -> Changing the font does not invalidate cached sizes.
M> -> wxVListBoxComboPopup::CalcWidths() is not optimized, in the
M> sense of using wxTextMeasure optimizations.
M> -> The combopop does not use a right space between lines.
M> -> Margins are set, but they are not used at all.
M>
M> What are the priorities?
M> Do I fix these bugs (and some others I'm sure they will expose),
M> or do I continue with wxSpinCtrl and wxSearchCtrl?

This depends on whether this makes this control DoGetBestSize() better or
worse than before. If it doesn't make it better, it's probably not worth
applying the patch for now. And if it makes it worse, then we definitely
shouldn't apply it to avoid knowingly introducing new regressions.

FWIW I think having this functionality in wxSpinCtrl is more important
than in wxComboCtrl (wxSearchCtrl probably not so much).

Regards,
VZ

[Manolo]

> M> What are the priorities?
> M> Do I fix these bugs (and some others I'm sure they will expose),
> M> or do I continue with wxSpinCtrl and wxSearchCtrl?
>
> This depends on whether this makes this control DoGetBestSize() better or
> worse than before. If it doesn't make it better, it's probably not worth
> applying the patch for now. And if it makes it worse, then we definitely
> shouldn't apply it to avoid knowingly introducing new regressions.

You can judge on yourself. Run the combo sample (open comparison dlg).
In the some months ago version, you won't see any issue because of
sizers hiding any size issue.
After my change to the sample, you'll see bad sizes.
With "part_3" patch things go a bit better, but still are bad.

> FWIW I think having this functionality in wxSpinCtrl is more important
> than in wxComboCtrl (wxSearchCtrl probably not so much).

OK. I continue with wxSpinCtrl. If after running combo sample you change
priorities, tell me.

Regards,
Manolo

[Vadim Zeitlin]

On Wed, 14 Nov 2012 01:49:17 +0100 Manolo wrote:

M> After my change to the sample, you'll see bad sizes.

Yes, I do see it. But OTOH wxComboCtrl doesn't seem to handle non-default
font at all because the strings in drop down are just truncated in the
middle control. So fixing the size computations in this case is not a high
priority as long as the drop down is completely unusable anyhow...

M> With "part_3" patch things go a bit better, but still are bad.

If you think there are no regressions compared to r72934 (just before
GetSizeFromTextSize() was added), I'll apply it.

M> > FWIW I think having this functionality in wxSpinCtrl is more important
M> > than in wxComboCtrl (wxSearchCtrl probably not so much).
M>
M> OK. I continue with wxSpinCtrl. If after running combo sample you change
M> priorities, tell me.

So far I didn't...

Thanks,
VZ

[Manolo]

> M> After my change to the sample, you'll see bad sizes.
>
> Yes, I do see it. But OTOH wxComboCtrl doesn't seem to handle non-default
> font at all because the strings in drop down are just truncated in the
> middle control. So fixing the size computations in this case is not a high
> priority as long as the drop down is completely unusable anyhow...

Whit default font it "seems" OK. Setting a different font is not very
usual...

> M> With "part_3" patch things go a bit better, but still are bad.
>
> If you think there are no regressions compared to r72934 (just before
> GetSizeFromTextSize() was added), I'll apply it.

Regressions? Me? ;) Now seriously, I hope there's none. What the user
may notice is that now default width is the same as wxChoice: 80 px
instead of 150. And height is more close to native's one.

Regards,
Manolo