Gmoccapy fix various (localization) bugs

[Sigma1912]

• Calculator_widget: Fix locale handling
• Offsetpage_widget: force display of unlocalized float values, remove unused format template
• Tooledit_widget: use local.atof() instead of replacing commas with dot
• Gmoccapy offsetpage: Fix 'Rot' column not calling the calculator on editing

[zz912]

Will it works corectly with DE localization?

[Sigma1912]

I did when I tested it with de_DE

[zz912]

Step 1:
Edit: I entered the float number here manually. I intentionally wrote it with a decimal point.

Step 2:

Using "atof" is useless for us, put "replace" back there.

[hansu]

"Calculator_widget: Fix locale handling"

Which case does this fix? (A short description would fit perfectly in the extended commit description 😉 )

[hansu]

Suggested change

	self.model[path][col] = "%10.4f" % locale.atof(new_text)
	self.model[path][col] = f"{locale.atof(new_text):10.4f}"

Please not re-introduce the old style formatting

[Sigma1912]

As there is general disagreement with this commit you might want to cherry pick the others for now.

[hansu]

It's just about the line, not the whole commit.
I thought it's easy to change that line...

[hansu]

"Gmoccapy offsetpage: Fix 'Rot' column not calling the calculator on editing"

Good catch 👍 - didn't notice that

[hansu]

[zz912)

Using "atof" is useless for us, put "replace" back there.

Yeah we have to find a solution that the calculator also accepts a dot with "decimal-comma-locales", but only one and so on...

[Sigma1912]

Using "atof" is useless for us, put "replace" back there.

Not sure I would agree with this statement.

1. as noted in the commit message: 1.000,23 is a valid local value in eg 'de_DE', replacing the comma with a dot will result in 1.000.23 which is invalid.
   local.atof(1.000,23) will return the correct float 1000.23

2. I'm not interested in 'fixing' something for some locales and have it break on others.

However, since I'm in the decimal dot space I'll leave the decisions to you guys who live in the decimal comma space.

[Sigma1912]

Just to be clear, I'm not insisting on using 'locale.atof() but using this will give consistent behavior for all location settings.
I'm not against allowing the use of decimal dot AND decimal comma but just replacing commas with dots is not a sound solution.
Thanks

[Sigma1912]

Maybe something like this:

def check_decimal(value):
    # check decimal symbol
    comma_index = 0
    dot_index = 0
    try:
        comma_index = value.index(',')
    except ValueError:
        pass
    try:
        dot_index = value.index('.')
    except ValueError:
        pass
    if value:
        if comma_index > dot_index:
            value = value.replace('.', '').replace(',', '.')
    try:
        value = float(value)
        return value
    except:
        return 'Error'

[zz912]

[Sigma1912]

as noted in the commit message: 1.000,23 is a valid local value in eg 'de_DE',

That's just a theory. #3499 (comment)

Maybe something like this:

def check_decimal(value):
    # check decimal symbol
    comma_index = 0
    dot_index = 0
    try:
        comma_index = value.index(',')
    except ValueError:
        pass
    try:
        dot_index = value.index('.')
    except ValueError:
        pass
    if value:
        if comma_index > dot_index:
            value = value.replace('.', '').replace(',', '.')
    try:
        value = float(value)
        return value
    except:
        return 'Error'

This code tries to solve the problem with numbers in the format 1.523.785,45 .
But this format is only used in economics, humanities, books, ... , but not in technical software.

If (number of decimal points + number of decimal comma) > 1
I would consider it an error.

I would name the function something other than "check".
I would expect the result "Yes","No","Error" from the "check" function.
For example: gui_stof (gui convert string to float)

[Sigma1912]

I would name the function something other than "check".

That is just example code in case somebody wants to try it out.
As I said, I'm happy to let you guys figure this out.

[zz912]

As I said, I'm happy to let you guys figure this out.

It was solved by replace(',', '.') in try function.

You want make it better. I'm just trying to explain information that you can't possibly know. Please don't take it as criticism. On the other hand, you're thinking the same way I did when I was dealing with this.

[Sigma1912]

No problem, the contentious commit has been removed

[zz912]

Do you want to consider remaking the other "atof" as well? While you're at it?

If not, I'll focus on testing them thoroughly.

I'd like to give you some more information so you can understand the context:

float = locale.atof("1.54")
float = locale.atof("1,54")

results in DE localization (Hans):
154
1.54

results in CS localization (me):
1.54
1.54

[hansu]

If (number of decimal points + number of decimal comma) > 1
I would consider it an error.

Agree, It would also do it this way

[Sigma1912]

If not, I'll focus on testing them thoroughly.

Sure, go ahead.

[zz912]

After your force-pushed I cloned your linuxcnc fork and switched to the Gmoccapy_Fix_various_bugs branch.
Unfortunately this bug #3500 (comment) still persists.

I think, this is not your fault, but you are fixing something that works wrong by default.

[Sigma1912]

In order to allow dot AND comma decimals in the calculator we will likely need to remove all localization from calculatorwidget.py. Since we currently share this widget with some legacy guis like 'gscreen' and 'woodpecker' I'll need to have Gmoccapy use it's own version of calculator. I'll have a look at it tomorrow.

[zz912]

In order to allow dot AND comma decimals in the calculator we will likely need to remove all localization from calculatorwidget.py.

You cannot remove all localization. Remove only the localization related to float number.

Since we currently share this widget with some legacy guis like 'gscreen' and 'woodpecker' I'll need to have Gmoccapy use it's own version of calculator.

I think you won't get away with it when others try to standardize things. ISTAT for example.

[Sigma1912]

[zz912]

The behavior of calculator in *.gif picture is exactly as I imagined and exactly as I think it is correct.

I don't understand why you keep using "locale.atof" in your fix code?
Do you know a way to prevent "locale.atof" from doing 1.25 => 125?
The fact that "locale.atof" can do 1.25 => 125 makes it a very dangerous function for CNC machines. I think that "locale.atof" should be removed from the entire LCNC. It is clear to me that it will not be immediately, but gradually.

The fact that your code works is only because you hid "locale.atof" under the self.allow_dot_and_comma option.

            if not self.allow_dot_and_comma:
                try:
                    # convert to decimal dot format
                    i = str( locale.atof( i ) )
                except:
                    pass

1. I think we don't need the switch self.allow_dot_and_comma
   allowing dot and comma in input should always be there.
2. I think self.allow_dot_and_comma = False is just opening Pandora's box

If you want to make the calculator module perfect, the switch self.allow_show_comma would be useful
This switch would
if self.allow_show_comma == 1:
show float number with decimal comma by locale setting

if self.allow_show_comma == 0:
show float number only with decimal dot

In both cases, the calculator would be able to accept decimal comma and decimal dot.

Gmoccapy will use self.allow_show_comma == 0

I would like to ask @hansu to comment on my opinions. I am convinced that my opinions are correct, but I don't want Sigma1912 to keep rewriting the codes.

[Sigma1912]

I don't understand why you keep using "locale.atof" in your fix code?

I'm not. If you compare the current code and my PR then you see that I'm just moving the existing code into an 'if' clause so this part of the code is no longer used in gmoccapy.
https://github.com/LinuxCNC/linuxcnc/blob/master/lib/python/gladevcp/calculatorwidget.py#L162

I'm not willing to change the behavior of the calculator widget for the other guis if that is what you are expecting.

[zz912]

I don't understand why you keep using "locale.atof" in your fix code?

I'm not. If you compare the current code and my PR then you see that I'm just moving the existing code into an 'if' clause so this part of the code is no longer used in gmoccapy. https://github.com/LinuxCNC/linuxcnc/blob/master/lib/python/gladevcp/calculatorwidget.py#L162

I'm not willing to change the behavior of the calculator widget for the other guis if that is what you are expecting.

I understand what you're doing. But I think this is the wrong approach to fix.

If there's a bug somewhere, it should be fixed, not hidden under an if statement.

I don't think any GUI wants to have 1.25 => 125 happen.

[Sigma1912]

A consequence of sharing these widgets with other GUIs is that changes to the default behavior would need to be tested in ALL of those GUIs.
Since I know that 'gscreen' and 'woodpecker' use it I would be responsible to test these two with the modified caclulator widget.
Besides not being familiar with those GUI's they are largely unmaintained (have been for a number of years) and have deteriorated to the point of being (in my opinion) derelict and obsolete.
I have suggested to remove these from the source code but others have thought otherwise. Dragging these legacy guis along makes any update to a shared widget a pain in the butt. A pain that I am simply not willing to take upon myself.

Thank you for your understanding.

[zz912]

I think a simple rule should be made:
If we can't find someone willing to at least test a GUI, that GUI should be removed from LCNC.
Unfortunately, this is also just my wish and probably won't happen.

I understand that it is impossible to test all GUIs. I know how much time it takes me to "only" test Gmoccapy. I always wonder how much time it must take to maintain Gmoccapy.

I think that common widgets would deserve a history in the header of the file. Here you can write what you did and in which GUIs you tested it.

Another source of information for maintainers could be here:
http://linuxcnc.org/docs/devel/html/gui/gui-dev-reference.html
A chapter on Widgets could be created here, where some tricky things could be pointed out.

[c-morley]

You should always code on gladevcp general widgets as if they are used by other GUIs/panels.
Linuxcnc's super long time between releases makes removal of code troublesome.
I intend to remove Gscreen - I was waiting for a release first.
But that does not change the fact that gladevcp widgets should try hard not to be too screen specific.

I agree, the GUI reference could be expanded to include practical 'coding' idiosyncrasy to prevent multiple reinventing of the wheel. QtVCP was a doc section on code snippets. Maybe that might be better to have similar for GladeVCP.

[gmoccapy]

Sorry for jumping in so late.
IMHO atof is the way to go, entering 1.234 must result in 1234 as with De_de locale settings the "dot" is the thousand separator and the "komma" is the decimal separator.

IMHO entering 1.23+2,34 is an operator error and that may happen.
But with De_de locale an 1.234 + 2,345 "must" result in 1236,345

This is just my personal opinion!

Norbert

[zz912]

[Norbert]

with De_de locale settings the "dot" is the thousand separator and the "komma" is the decimal separator.

[Norbert]

I do not like the DRO with comma as decimal separator.

#2966 (comment)

Right now I feel like an absolute idiot for trying to convince Sigma1912 that a dot is a decimal separator.

Have you changed your mind? And wouldn't you mind the decimal comma in the DRO?
Or do you want the decimal dot to have a different meaning somewhere in Gmoccapy than somewhere else?

I think someone will crash the machine if they copy the DRO, but the machine will work with a different value:

The decision to consider "dot" as the decimal separator is not just mine:
#3086

[hansu]

Sorry for jumping in so late. IMHO atof is the way to go, entering 1.234 must result in 1234 as with De_de locale settings the "dot" is the thousand separator and the "komma" is the decimal separator.

IMHO entering 1.23+2,34 is an operator error and that may happen. But with De_de locale an 1.234 + 2,345 "must" result in 1236,345

This is just my personal opinion!

Norbert

So

• display all numbers with a dot regardless of localization, like it has always been the case for the DRO, and lately also for the Tool information area.
• accept the input only in the correct locale format

But that is weird especially when editing the tool table. You see a dot - edit it and keep the dot and then the value is 1000 times greater than before.
Or the dot needs to be converted to the correct locale decimal separator when the cell is edited.

[zz912]

Hi Norbert,

I wanted to ask you for instructions on how to continue with this fix.
You ended your post saying that it was just your opinion.
On the other hand, you are the author of Gmoccapy and no one wants to ignore your opinions.
I would like to convince you that the way Sigma1912 modified the widget calculator is correct. I mean the behavior according to the gif #3500 (comment)
What can I do to convince you? More tests? More examples?

My arguments are:

• that it was your wish in the past
• that many members of the LinuxCNC community agree with this solution

I am convinced after a long time dealing with the float numbers issue that locale.atof should be removed from the entire LCNC.

1. locale.atof is very problematic to test because it behaves differently in each localization. Here I paradoxically support the German localization, because in the Czech localization 1,25 => 1.25 and 1.25 => 1.25
2. It is possible to make the code containing locale.atof able to accept decimal commas and dots, but the code is then unnecessarily complex and unclear.
3. I think that a lot of code in LCNC with locale.atof works only because the input for the locale.atof function was generated by some other locale function. So it is compatible. The problem arises when we want to switch to a single decimal separator. Suddenly the atof function does not get what it needs and crashes the program. I think that in the future such problems as we are solving with the calculator now will increase.
4. Programmers who use decimal dot localization will not realize they have created a bug.

I am convinced that value.replace('.', '') is the right way to go.

1. this solution looks amateurish and simple, but it is very robust and can be easily tested
2. I realize that value.replace('.', '') needs to be supplemented with a "string to float" function and a "space removal" function. All of this must be placed in a Try function, in case the input contains letters, for example.
3. I cannot judge whether it would be appropriate to make some general function that would include the previous point and be placed in some common library

Zdeněk