Opened 11 months ago

Closed 11 months ago

Last modified 7 weeks ago

#15792 closed defect (fixed)

Add WXWIN_COMPATIBILITY_3_0, remove WXWIN_COMPATIBILITY_2_6

Reported by: vadz Owned by:
Priority: normal Milestone: 3.1.0
Component: build Version:
Keywords: Cc:
Blocked By: Blocking: #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625, #15625
Patch: no

Description

We need a new symbol to be able to conditionally compile stuff we want to remove in 3.6 and we can now finally remove 2.6 compatibility. Both should be simple to do but just need to be done carefully, if anybody is willing to spend time on this, it would be welcome.

Attachments (1)

wxwin_compatibility_3_0.patch download (37.8 KB) - added by awi 11 months ago.
Patch that adds WXWIN_COMPATIBILITY_3_0 macro.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 11 months ago by vadz

  • Blocking 15625 added

(In #15625) 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!

comment:2 Changed 11 months ago by awi

  • Blocking

(In #15625) Please find attached two patch files:

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

Changed 11 months ago by awi

Patch that adds WXWIN_COMPATIBILITY_3_0 macro.

comment:3 Changed 11 months ago by awi

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:4 Changed 11 months ago by PC

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

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

comment:5 Changed 11 months ago by vadz

  • Blocking

(In #15625) 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!

comment:6 Changed 11 months ago by awi

  • Blocking

(In #15625) 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:7 Changed 11 months ago by vadz

  • Blocking

(In #15625) 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.

comment:8 Changed 11 months ago by awi

  • Blocking

(In #15625) 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:9 Changed 11 months ago by VZ

  • Blocking

(In #15625) (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:10 Changed 11 months ago by VZ

  • Blocking

(In #15625) (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:11 Changed 9 months ago by StepanHrbek

  • Blocking

(In #15625) 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:12 Changed 9 months ago by awi

  • Blocking

(In #15625) 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?

comment:13 Changed 9 months ago by StepanHrbek

  • Blocking

(In #15625) 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:14 Changed 9 months ago by awi

  • Blocking

(In #15625) 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:15 Changed 9 months ago by vadz

  • Blocking

(In #15625) 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:16 Changed 9 months ago by StepanHrbek

  • Blocking

(In #15625) 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:17 Changed 9 months ago by vadz

  • Blocking

(In #15625) 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...

comment:18 Changed 9 months ago by awi

  • Blocking

(In #15625) 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:19 Changed 9 months ago by StepanHrbek

  • Blocking

(In #15625) I confirm that it solves problems I had. Good work, thanks!

comment:20 Changed 9 months ago by vadz

  • Blocking

(In #15625) 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:21 Changed 9 months ago by VZ

  • Blocking

(In #15625) (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:22 Changed 9 months ago by VZ

  • Blocking

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

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

See #15625.

comment:23 Changed 9 months ago by VZ

  • Blocking

(In #15625) (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:24 Changed 9 months ago by awi

  • Blocking

(In #15625) 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:25 Changed 9 months ago by vadz

  • Blocking

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

comment:26 Changed 7 weeks ago by vadz

  • Blocking
Note: See TracTickets for help on using tickets.