Opened 9 months ago

Last modified 5 months ago

#15625 portneeded defect (port to stable)

Problems with SetAttribute() in wxFloatProperty with SpinCtrl, MotionSpin for specific Min, Max values

Reported by: StepanHrbek Owned by:
Priority: normal Milestone: 3.0.2
Component: wxPropertyGrid Version: stable-latest
Keywords: wxPropertyGrid wxFloatProperty SpinCtrl attributes Cc:
Blocked By: #15792 Blocking:
Patch: yes

Description

Hello,

let there be wxFloatProperty with

SetAttribute("Min",1e-20);
SetAttribute("Max",1);
SetAttribute("Step",1);
SetAttribute("Wrap",false);
SetAttribute("MotionSpin",true);
SetEditor("SpinCtrl");

When I drag spinbutton down until property value reaches zero, wx asserts (src/common/wincmn.cpp 3287).

When I click spinbutton down until property value reaches zero, close "Property error" message and then move mouse over spinbutton without clicking it, wx asserts (src/common/wincmn.cpp 461).

There is no assert if "Min" is 0 or 1e-2.

Tested with MSVC, svn revision 75075.

After looking into bool NumericValidation() in src/propgrid/props.cpp, I can't spot what's wrong. I see suspicious but probably innocent wxINT64_MIN applied even if T is double; and suspicious "value = max - (min-value)" which might be wrong when value gets far below min (new value stays outside <min,max> range); but still nothing that would explain why 1e-20 is bad and 1e-2 is ok.

Attachments (10)

propgrid-bug-example.patch download (978 bytes) - added by awi 7 months ago.
Propgrid example to reproduce bug.
propgrid-valid.patch download (1.5 KB) - added by awi 7 months ago.
Patch to fix out of value issues.
num-formatter.patch.patch download (4.3 KB) - added by awi 7 months ago.
Replacing DoubleToString with wxNumberFormatter::ToString in wxPropertyGrid
propgrid-samples.patch.patch download (3.1 KB) - added by awi 7 months ago.
Replacing DoubleToString with wxNumberFormatter::ToString in 'propgrid' samples
numformatter-with-inc.patch download (4.8 KB) - added by awi 7 months ago.
Replacing DoubleToString with wxNumberFormatter::ToString in wxPropertyGrid (with include file)
numformatter-test.patch download (1.8 KB) - added by awi 7 months ago.
Additional tests for wxNumberFormatter::ToString
numformatter-test-2.patch download (1.8 KB) - added by awi 7 months ago.
Tests for wxNumberFormatter::ToString (fixed)
numformatter-test-3.patch download (1.8 KB) - added by awi 7 months ago.
Next version of tests for wxNumberFormatter::ToString
propgrid_err.diff download (725 bytes) - added by StepanHrbek 6 months ago.
shows error with Min=0.1f
propgrid-floatprop.patch download (7.8 KB) - added by awi 5 months ago.
Patch implementing WYSIWYG for wxFloat property with SpinCtrl.

Download all attachments as: .zip

Change History (42)

comment:1 Changed 7 months ago by vadz

  • Keywords repro-needed added

It would be great to have a minimal patch to the propgrid sample reproducing the problem as it risks being very difficult to do anything about it otherwise, especially considering that nobody here knows this code that well (its original author not being available to work on it any more...).

Changed 7 months ago by awi

Propgrid example to reproduce bug.

Changed 7 months ago by awi

Patch to fix out of value issues.

comment:2 Changed 7 months ago by awi

  • Patch set

I prepared on my own a test case to reproduce the issue (patch file to propgrid sample attached).
In fact, the issue occurs when "Min" value for wxFloatProperty object is less then 1e-6 .

This issue occurs because after unsuccessful validation a new correct value is calculated with precision of 6 digits (call of wxPropertyGrid::DoubleToString function which returns number as a numeric string) and small numbers (<1e6) are truncated to value zero which is out of range value and all the logic of the control is broken this way.
Moreover, current wxPropertyGrid::DoubleToString function can produce numbers only in decimal representation what is problematic for very small and very big numbers.

Changes made:

  • wxPropertyGrid::DoubleToString is changed to produce numeric strings in shorter of two representations: scientific or decimal (%g format) if decimal format is not required.
  • After unsuccessful validation wxPropertyGrid::DoubleToString function is called in a way that precision is not fixed to 6 digits.

Patch file attached.

comment:3 Changed 7 months ago by vadz

Thanks for working on this! I'm a bit confused by the terminology used (is "decimal" what printf functions call "fixed" representation?), but if it fixes the problem, the patch should be probably applied as I don't see any drawbacks to it.

However beyond this I wonder if we couldn't replace this code with wxNumberFormatter, as it seems to be reimplementing its functionality. Or is there something it does that that class can't do?

comment:4 Changed 7 months ago by vadz

  • Keywords repro-needed removed
  • Status changed from new to confirmed

comment:5 Changed 7 months ago by awi

Sorry for the mess with terminology! :)

Regarding wxPropertyGrid::DoubleToString function: you are right, in this context it could be easy replaced with wxNumberFormatter::ToString which is much more readable. (I was not aware that this function even exists!)
DoubleToString has only one additional feature: it can return format string used to convert a number.
For what reason, I don't know. Perhaps it's a kind of legacy feature which is never used in the library.
DoubleToString is also called (twice) in propgrid sample but could be easily replaced with wxNumberFormatter.

So, there are two functions doing practically the same. I would tend to replace DoubleToString with wxNumberFormatter but decision is yours.

comment:6 Changed 7 months ago by vadz

I'd like to drop DoubleToString(). Unfortunately we probably can't do this immediately as there may be some code using it, as it occurs in the sample (and people tend to copy the code from the samples -- rightly so, of course). But we can deprecate it and put it inside #if WXWIN_COMPATIBILITY_3_0 (which needs to be added as I've just realized, see #15792) and replace all the uses of it inside wxWidgets itself with wxNumberFormatter::ToString.

So if you could please update your patch to use the latter, it would be great, TIA!

Changed 7 months ago by awi

Replacing DoubleToString with wxNumberFormatter::ToString in wxPropertyGrid

Changed 7 months ago by awi

Replacing DoubleToString with wxNumberFormatter::ToString in 'propgrid' samples

comment:7 Changed 7 months ago by awi

Please find attached two patch files:

  1. Patches replacing DoubleToString with wxNumberFormatter::ToString and fixing its potential issues:
  • Current wxNumberFormatter::ToString with option Style_NoTrailingZeroes removes trailing zeroes even if numeric string is in scientific format (eg. for value 1e-20 is returned wrong string "1e-2").
  • Current wxNumberFormatter::ToString with option Style_NoTrailingZeroes throws assertion error for integral numeric strings (it assumes that digital point is always present, what is not always the case, eg. for value 1.0 with precision -1).
  • Current wxNumberFormatter::ToString with option Style_NoTrailingZeroes returns "-0" string when all trailing zeroes are removed from rounded result value (eg. for value -0.000123 with precision of 3 digits).
  • Current wxNumberFormatter::ToString with option Style_WithThousandsSep tries to apply thousands separators even for numeric strings in scientific format (what is not used in practice).
  1. Patches for propgrid samples where DoubleToString is replaced with wxNumberFormatter::ToString (using conditional blocks controlled by WXWIN_COMPATIBILITY_3_0).

comment:8 Changed 7 months ago by vadz

  • Blocked By 15792 added

Thanks for the patches!

The only minor things to change are:

  1. We need #if WXWIN_COMPATIBILITY_3_0 check around DoubleToString() in the header as well.
  2. We don't need the compatibility check in the samples however, they should just use the latest and greatest API.
  3. We do need the extra tests but could you please put them in source:wxWidgets/trunk/tests/strings/numformatter.cpp instead of the propgrid sample?

Thanks again!

Changed 7 months ago by awi

Replacing DoubleToString with wxNumberFormatter::ToString in wxPropertyGrid (with include file)

Changed 7 months ago by awi

Additional tests for wxNumberFormatter::ToString

comment:9 Changed 7 months ago by awi

Please find attached two patch files:

  1. Patches replacing DoubleToString with wxNumberFormatter::ToString (also changes in header)
  2. New tests for wxNumberFormatter::ToString

comment:10 Changed 7 months ago by awi

  • Blocked By

(In #15792) Please find enclosed a patch file that adds WXWIN_COMPATIBILITY_3_0 macro to source and configuration files:

  • It replaces FUTURE_WXWIN_COMPATIBILITY_3_0 with WXWIN_COMPATIBILITY_3_0 macro in source files.
  • It replaces wxDEPRECATED_FUTURE with wxDEPRECATED macro in source files.
  • It adds handling of WXWIN_COMPATIBILITY_3_0 macro in configuration files.

@vadz: Could you please assign WXWIN_COMPATIBILITY_2_6 removal to a separate ticket? It seems that both questions are independent of each other and can be done separately.

comment:11 Changed 7 months ago by PC

  • Blocked By

(In #15792) (In [75532]) remove WXWIN_COMPATIBILITY_2_6, add WXWIN_COMPATIBILITY_3_0
closes #15792

comment:12 follow-up: Changed 7 months ago by vadz

Sorry, actually, having started to test this, I have another question: why does a newly added test expects wxNumberFormatter::ToString(12345.012, -1) to produce "12345" and not "12,345"? It seems like it ought to be the latter and this is what the current code does, so why does it need to be changed?

TIA!

Changed 7 months ago by awi

Tests for wxNumberFormatter::ToString (fixed)

comment:13 in reply to: ↑ 12 Changed 7 months ago by awi

Replying to vadz:

Sorry, actually, having started to test this, I have another question: why does a newly added test expects wxNumberFormatter::ToString(12345.012, -1) to produce "12345" and not "12,345"? It seems like it ought to be the latter and this is what the current code does, so why does it need to be changed?

TIA!

You are right.
I haven't noticed that there is no thousands separator set on my machine.
Fixed patch attached.

comment:14 follow-up: Changed 7 months ago by vadz

Sorry, there is another unit test failure:

.../tests/strings/numformatter.cpp:155:Assertion
Test name: NumFormatterTestCase::DoubleToString
equality assertion failed
- Expected: 0.0
- Actual  : -0.0

Again, it's the test which seems wrong to me here and not the code, as I don't see anything wrong with formatting "-0.02" as "-0.0" with 1 digit of precision. But I'm less sure here... What do you think?

In any case, either the test or the code ought to be changed.

Changed 7 months ago by awi

Next version of tests for wxNumberFormatter::ToString

comment:15 in reply to: ↑ 14 Changed 7 months ago by awi

Replying to vadz:

Sorry, there is another unit test failure:

.../tests/strings/numformatter.cpp:155:Assertion
Test name: NumFormatterTestCase::DoubleToString
equality assertion failed
- Expected: 0.0
- Actual  : -0.0

Again, it's the test which seems wrong to me here and not the code, as I don't see anything wrong with formatting "-0.02" as "-0.0" with 1 digit of precision. But I'm less sure here... What do you think?

In any case, either the test or the code ought to be changed.

I think number in "-0.0" form is acceptable.
Patch file with updated tests attached.

comment:16 Changed 7 months ago by VZ

(In [75560]) Fix several problems with number formatting in wxNumberFormatter.

We shouldn't add thousands separators nor remove trailing zeros for the
numbers in scientific format.

Also avoid "-0" as output.

See #15625.

comment:17 Changed 7 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [75561]) Deprecate wxPropertyGrid::DoubleToString().

Simply use wxNumberFormatter instead, this reduces code duplication and avoids
bugs due to formatting inconsistencies in DoubleToString().

Closes #15625.

comment:18 Changed 6 months ago by StepanHrbek

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thanks for your work!

I just updated to trunk, rev 75875, and it works perfectly for "Min" 1e-20.

But now it asserts for "Min" 0.1.

comment:19 Changed 6 months ago by awi

I have just tested wxFloatProperty and SpinCtrl with following parameters:
Min: 0.1
Max: 1.0
Step 1.0
Initial value: 0.1 and 0.2
An it works fine.

Could you tell for what parameters an assertion is thrown in your case?

Changed 6 months ago by StepanHrbek

shows error with Min=0.1f

comment:20 Changed 6 months ago by StepanHrbek

I'm sorry, it asserts for Min 0.1f.

It is critical to use float precision, I did not notice this when reporting. Now I see how important it is to send patch.

comment:21 Changed 5 months ago by awi

  • Blocked By
  • Keywords wxPropertyGrid wxFloatProperty SpinCtrl attributes added
  • Summary changed from asserts in wxFloatProperty with SpinCtrl, MotionSpin, SetAttribute("Min",1e-20) to Problems with SetAttribute() in wxFloatProperty with SpinCtrl, MotionSpin for specific Min, Max values

OK, the issue with "0.1f property value" is somehow related to the originally reported issue but in fact it's nature is different.
Previous problem occurs when property value was < 1e-6 (and for this case already provided fix is still valid) but now the problem occurs if floating point property value has more then 6 significant digits.

The problems can be observed for (some) 'float' values by accident only due to the conversion issues: wxVariant (which is a type of value argument of SetProperty()) supports floating point numbers of type 'double' only. If number of 'float' type is provided then it is converted to double and the value 0.1f is stored as 0.1000000049... due to the known limitations of floating point numbers representation model.
The problem can be observed in the test sample also if value of 'double' type like 0.1000000049 is provided for "Min" property or e.g. value 0.9999992 is provided for "Max" property.

Two factors are the root cause of the problem:

  1. The logic of SpinCtrl is based on the assumption that current(pending) value of property never exceeds Min, Max values and hence no error messages would be displayed in a separate window and focus never be transferred from SpinCtrl to any window.
  2. Current (pending) value of the property set by SpinCtrl is not stored internally as floating point variable but after every change directly converted to text representation which is rounded to 6 digits by default.

The flow of operations which leads to the observed problem for test sample is as follows:

  1. SpinCtrl changes current value 0.1 down by step 1 to the pending value -0.9
  2. Value -0.9 is validated inside SpinCtrl against (0.1000000049, 1.0) range and the validated output value taken from the validation is 0.1000000049
  3. This value is drawn in associated edit control as nice-looking string "0.1" (with rounding)
  4. High-level event handler procedure is notified that edit control has changed and validation for edit control is triggered.
  5. String "0.1" is taken form the control, converted to the floating point value 0.1 and validated against (0.1000000049, 1.0) range.
  6. Validation fails, error message is displayed and focus is released from SpinCtrl and assertion is thrown because this completely breaks the logic of active SpinCtrl.

I see few options to fix this issue but some design decisions must be made before (@VZ) because some solutions are quick and simple and some are difficult and rather intrusive:

  1. Resign from rounding the pending values displayed in the edit control. Some numbers can look ugly but their maximal precision would be preserved.
  1. Extend edit control class associated with SpinCtrl and add internal variable (property) holding pending value in the binary form with maximal precision regardless of current text representation for display purposes.
  1. Change validation flow for SpinCtrl associated with edit control from current:

SpinCtrl event -> SpinCtrl validation -> changing Edit control -> get value from Edit Control -> Edit control validation
to something like:
SpinCtrl event -> SpinCtrl validation -> changing Edit control
Looks simple but can be troublesome because this is about event handling.

comment:22 Changed 5 months ago by vadz

I don't know this code at all, so I'm afraid I can't help much. But it seems clear to me that we do not want to show 0.100000049 to the user, so the solution (1) is not acceptable.

I think both (2) and (3) could work but I also think that perhaps the simplest solution would be

  1. Make the range checks work with 1e-6 precision.

What do you think?

comment:23 Changed 5 months ago by StepanHrbek

Let me describe my use case. I have a property similar to scale, it must not be 0. It is usually 1, but some users might need very small scale, so I set Min 1e-20.

(4) would break my code if it somehow rounds 1e-20 to 0. On the other hand, using floats for range check would solve my case, 1e-20 Min would work as expected. Still, both ways of reducing precision look wrong, as result can fall outside original double precision range sent to SetAttribute(). We can see SpinCtrl as an example of code that asserts and crashes when some number falls very slightly out of range due to limited precision.

comment:24 Changed 5 months ago by vadz

I guess we should do (2) then. Conceptually this does seem the right thing to do: we can't rely on being always able to recover the value from the text, so we have to store it independently...

Changed 5 months ago by awi

Patch implementing WYSIWYG for wxFloat property with SpinCtrl.

comment:25 Changed 5 months ago by awi

In the documentation http://docs.wxwidgets.org/trunk/classwx_p_g_property.html#wxFloatProperty it is stated that "Default float-to-text precision is 6 decimals, but this can be changed by modifying wxPG_FLOAT_PRECISION attribute".
This is about text representation of the entered value but I think this can be also applied to the real value because it could be misleading if entered and displayed value would be different from the real one (wxFloatProperty is an interactive control and WYSIWYG concept applies here).

If the precision of displayed numbers (controlled by wxPG_FLOAT_PRECISION attribute) would be the same as precision of stored values then the control would behave in a consistent way and its output would be predictable.

wxFloatProperty already works this way if numbers are entered directly into the edit field.
Unfortunately, in the current code SpinCtrl doesn't take advantage of the precision set via wxPG_FLOAT_PRECISION attribute.

Attached patch implements WYSIWYG concept also for values set through SpinCtrl.
Fixes and changes implemented:

  1. In the current code the precision can be set by calling SetAttribute(wxPG_FLOAT_PRECISION, n) function but for unknown reasons it is not possible to obtain its value via GetAttribute(wxPG_FLOAT_PRECISION) call. This feature is implemented.
  2. In the validation process there are used values (minimal, maximal, current, pending) rounded to the precision set via wxPG_FLOAT_PRECISION attribute. To do so, NumericValidation() function responsible for validating values of 'double' type was separated from general template (with use of template specialization).
  3. Numbers changed by SpinCtrl are displayed also with precision set via wxPG_FLOAT_PRECISION attribute.

This way the current (pending) value should never fall out of range also if SpinCtrl is in use.

comment:26 Changed 5 months ago by StepanHrbek

I confirm that it solves problems I had. Good work, thanks!

comment:27 follow-up: Changed 5 months ago by vadz

Thanks @awi! I don't fully understand how does this code work but I'm going to commit it to the trunk nevertheless as it fixes an important problem. I hesitate about committing it to 3.0 branch though, do you think it's safe to do it or is there a possibility of breaking something previously working with these changes?

comment:28 Changed 5 months ago by VZ

(In [75978]) Allow retrieving wxPG_FLOAT_PRECISION and not just setting it.

It was possible to call SetAttribute() to change this attribute value but not
to get it back. Override DoGetAttribute() to also allow the latter.

See #15625.

comment:29 Changed 5 months ago by VZ

(In [75979]) Use symbolic attributes names in wxPropertyGrid code.

No real changes, just use constants instead of hardcoding their values.

See #15625.

comment:30 Changed 5 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [75980]) Fix several rounding problems with float values in wxPropertyGrid.

Loss of precision when converting floating point numbers to text and back
could result in several problems, notably comparing a valid value with the
minimum could fail after a round trip through wxSpinCtrl.

Fix this by using a specialization of NumericValidation() handling floating
point values specially and correctly.

Closes #15625.

comment:31 in reply to: ↑ 27 Changed 5 months ago by awi

Replying to vadz:

Thanks @awi! I don't fully understand how does this code work but I'm going to commit it to the trunk nevertheless as it fixes an important problem. I hesitate about committing it to 3.0 branch though, do you think it's safe to do it or is there a possibility of breaking something previously working with these changes?

I think that introduced changes shouldn't break any existing code. Within the scope of standard floating point parameters (values > 1e-6, significant digits <= 6) it works like base code does. Non-standard parameters are not handled properly by the base code so it's not likely that someone use it in the real application.

comment:32 Changed 5 months ago by vadz

  • Milestone set to 3.0.2
  • Resolution changed from fixed to port to stable
  • Status changed from closed to portneeded

OK, let's try to backport it -- but probably not for 3.0.1 if it's to be released soon.

Note: See TracTickets for help on using tickets.