Skip to content

Conversation

@zz912
Copy link
Contributor

@zz912 zz912 commented Aug 24, 2024

Previously:
tooledit_widget displayed float numbers with decimal dot for localizations that use decimal dot tooledit_widget displayed float numbers with decimal comma for localizations that use decimal comma

Now:
tooledit_widget displays float numbers with a decimal dot for all localizations

The functionality is preserved, when it is possible to enter a float with both a comma and a dot for localizations with a decimal comma

Reason for change:
Gmoccapy displayed some numbers with a decimal dot and some numbers with a decimal comma. This is a unification. More here: #2966 (comment)

Copy link
Member

@hansu hansu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this way it doesn't work unfortunately:

gmoccapy_tool_table_1

Also this leads to unwanted results when attempting to change values:

gmoccapy_tool_table_2

This shows the general problem when showing dots but entering commas:

gmoccapy_touch off-2

But here it not a big problem as the values in the input mask always keep their commas.

else:
try:
array[offset]= locale.format_string("%10.4f", float(word.lstrip(i)))
array[offset]= locale.delocalize(locale.format_string("%10.4f", float(word.lstrip(i))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

locale.delocalize(locale.format_string()) is a bit ... hmm ... twofold.
First add the proper locale format and then remove it?
A better style would be to use simply the string format function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can string format solve problems with , and . ?
In CS localization it works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but if we want the tooltable always show dots, we don't need localization at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we need decimal point "comma" akceptable for DE and CS and other localization.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that folks have dumped particular platforms for less of a reason than for not allowing them to regularly use their number pad.

@zz912
Copy link
Contributor Author

zz912 commented Aug 25, 2024

I tested this PR with this localizations:
#2966

cs localization - it works
de localization - it not works
en localization - it not works
It is very weird.

So I started using pure branche 2.9 I focused only on the DRO problem and the result is the same:
cs localization - it works
de localization - it not works
en localization - it not works

I want to ask you for a test:
Use DE localization
Use pure branch 2.9
Test only DRO
Is 1.5 = 1.5 ?
Is 1,5 = 1.5 ?

@hansu
Copy link
Member

hansu commented Aug 25, 2024

I want to ask you for a test:
Use DE localization
Use pure branch 2.9
Test only DRO
Is 1.5 = 1.5 ?
Is 1,5 = 1.5 ?

For de_DE: 1,5 results in 1.5 and 1.5 in 15.
For en_US the other way round: 1.5 results in 1.5 and 1,5 in 15.

IMHO 1,5 and 1.5 should be read as 1.5 like some CAD systems do.

@zz912
Copy link
Contributor Author

zz912 commented Aug 25, 2024

Thank you for results of my test.

IMHO 1,5 and 1.5 should be read as 1.5 like some CAD systems do.

I think, this is source of problem.

1,5 and 1.5 should not be read as 1.5.
1,5 and 1.5 MUST be read as 1.5.

For CS localization 1,5 and 1.5 is 1.5 and this PR works. This PR only shows deeper problem.

What is your view on this problem?

@hansu
Copy link
Member

hansu commented Aug 25, 2024

IMHO 1,5 and 1.5 should be read as 1.5 like some CAD systems do.

I think, this is source of problem.

I think this is just a nice to have.
This is not directly connected to the problem of your implementation.
But I am not really willing to dig deeper into this just because there is a comma. There are other more important issues. Sorry.

IMHO 1,5 and 1.5 should be read as 1.5 like some CAD systems do.

This I might implement

@zz912
Copy link
Contributor Author

zz912 commented Aug 26, 2024

IMHO 1,5 and 1.5 should be read as 1.5 like some CAD systems do.

This I might implement

This help me.

@smoe
Copy link
Collaborator

smoe commented Aug 30, 2024

I think you have come to a reasonable practical solution. Should we not use this thread to quickly sketch what an ideal solution would like like?

@zz912
Copy link
Contributor Author

zz912 commented Aug 30, 2024

If you would be willing to help, I would be very happy. I understand that this is not Hans' priority. Python, on the other hand, is my nemesis.

The current status is as follows:

  1. I think that the basic problem is that in DE localization
    1.5 = 15
    This issue is not relevant to this PR. I think that if this were fixed, this PR would also work in the DE localization. HansU disagrees with this.

  2. Find out why the locale library works correctly in the CS localization and badly in DE. I plan to write a program in the console outside of LCNC and investigate how the locale library behaves. If there will be 1.5 = 15

  3. Cough out the locale library for string to float and float to string transformation.
    It would be necessary to provide a comma replacement for a dot in the string to float transformation.

This is a long run for me.

@smoe
Copy link
Collaborator

smoe commented Aug 30, 2024

If you would be willing to help, I would be very happy. I understand that this is not Hans' priority. Python, on the other hand, is my nemesis.

:) I cannot help with coding for any foreseeable future, but let us have a look at what we want.

The current status is as follows:

  1. I think that the basic problem is that in DE localization
    1.5 = 15
    This issue is not relevant to this PR. I think that if this were fixed, this PR would also work in the DE localization. HansU disagrees with this.

I am not sure if he disagrees. In my mind any field that is expecting a number should not accept anything that it does not want to parse because of non-numerical characters or "ungrammatical" use of a thousands/decimal separator. So, "1.5" IMHO should not have been accepted in a German localisation.

  1. Find out why the locale library works correctly in the CS localization and badly in DE. I plan to write a program in the console outside of LCNC and investigate how the locale library behaves. If there will be 1.5 = 15
  1. Cough out the locale library for string to float and float to string transformation.
    It would be necessary to provide a comma replacement for a dot in the string to float transformation.

That is already all very operational. Those comma/decimal separators are confusing everyone everywhere. And we have the extra difficulty that the CNC folks may be expecting their beloved "." decimal operator even when everything is entered with "," because of the numerical keypad offering it that way. G code for instance only uses the point, right?

So, I think I would have a rule like, every number is entered the exact same way (and only that way) it is displayed. Character strings that not producing the same number as displayed (after transitioning to float and back to char) are not valid numbers and should not be accepted but trigger a beep or so.

Ideally, the active localization should somehow be shown in the window accepting the values.

@zz912
Copy link
Contributor Author

zz912 commented Aug 30, 2024

Reading float with comma is Norbert's request.
#2966 (comment)

@zz912 zz912 marked this pull request as draft August 30, 2024 20:25
@zz912
Copy link
Contributor Author

zz912 commented Aug 30, 2024

I made some research:

locale_format.py

#!/usr/bin/env python3

# formating without locale
print(f"{4.456:10.4f}"),

# localization
# maby you will have to run: apt-get install -y locales locales-all
import locale

locale.setlocale(locale.LC_ALL, 'de_DE')
print("locale de_DE:")
number_string = locale.format_string("%10.4f", float("10.125"))
print(number_string)
number_string = locale.delocalize(number_string)
print(number_string)
print(locale.delocalize(locale.format_string("%10.4f", float("10.125"))))

float_number = locale.atof("10.125")
print(float_number)
float_number = locale.atof("10,125")
print(float_number)


locale.setlocale(locale.LC_ALL, 'cs_CZ')
print("locale cs_CZ:")
number_string = locale.format_string("%10.4f", 10.125)
print(number_string)
number_string = locale.delocalize(number_string)
print(number_string)
print(locale.delocalize(locale.format_string("%10.4f", float("10.125"))))

float_number = locale.atof("10.125")
print(float_number)
float_number = locale.atof("10,125")
print(float_number)

locale.setlocale(locale.LC_ALL, 'en_US')
print("locale en_EN:")
number_string = locale.format_string("%10.4f", 10.125)
print(number_string)
number_string = locale.delocalize(number_string)
print(number_string)
print(locale.delocalize(locale.format_string("%10.4f", float("10.125"))))

float_number = locale.atof("10.125")
print(float_number)
float_number = locale.atof("10,125")
print(float_number)

Results:

    4.4560
locale de_DE:
   10,1250
   10.1250
   10.1250
10125.0
10.125
locale cs_CZ:
   10,1250
   10.1250
   10.1250
10.125
10.125
locale en_EN:
   10.1250
   10.1250
   10.1250
10.125
10125.0

I already know why 1.5 = 15 for DE localization.
It just doesn't read dots, so:
1.523 = 1523
2.4586 = 24586

The problem is not in locale.format_string
The problem is in the different behavior of locale.atof

Now I know why this PR is causing problems. I changed the decimal point for the source of locale.atof.

I don't know how to solve it at the moment.
locale.atof EN needs dot
locale.atof DE needs a comma

The solution can be more:

  1. replace dots with commas in the string and do not use locale.atof, but only float()
  2. replace dots with commas in the string and temporarily set locale.setlocale(locale.LC_ALL, 'en_US')

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Aug 31, 2024 via email

@zz912
Copy link
Contributor Author

zz912 commented Aug 31, 2024

Thanks everyone for the comments.
This patch PR has been tested:
LANG=cs_CZ.UTF-8
LANG=de_DE.UTF-8
LANG=en_US.UTF-8
and it works now. (I hope)

@zz912 zz912 marked this pull request as ready for review August 31, 2024 08:55
@hansu
Copy link
Member

hansu commented Sep 2, 2024

Looks good to me 👍 .
It would be nice if you could squash your two commits into one to eliminate the non-working one.

@zz912
Copy link
Contributor Author

zz912 commented Sep 2, 2024

It would be nice if you could squash your two commits into one to eliminate the non-working one.

How should I do it?

Previously:
tooledit_widget displayed float numbers with decimal dot for localizations that use decimal dot
tooledit_widget displayed float numbers with decimal comma for localizations that use decimal comma

Now:
tooledit_widget displays float numbers with a decimal dot for all localizations

The functionality is preserved, when it is possible to enter a float with both a comma and a dot for localizations with a decimal comma

Reason for change:
Gmoccapy displayed some numbers with a decimal dot and some numbers with a decimal comma. This is a unification. More here:
LinuxCNC#2966 (comment)
@hansu
Copy link
Member

hansu commented Sep 2, 2024

I have done it now for you.
It is basically this in this case:

  1. git rebase -i HEAD~2 (assuming you are on top of your branch)

  2. Edit the file that will be opened:

pick 3bc36a8456 tooledit_widget - decimal point = dot
- pick 353a76b62f Update tooledit_widget.py
+ s 353a76b62f Update tooledit_widget.py

# Rebase f7cbbcce36..353a76b62f onto f7cbbcce36 (2 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
#                    commit's log message, unless -C is used, in which case
  1. Edit the commit message

  2. git push -f (be careful with force push!)

@andypugh
Copy link
Collaborator

andypugh commented Sep 2, 2024

We have the option to "squash and merge" when merging a PR, but it's neater (and preserves better descriptions) if you do it yourself.

@andypugh
Copy link
Collaborator

andypugh commented Sep 2, 2024

The next question is... should it go in 2.9 instead? Is it a bugfix? It's one of those grey-areas, to me.

@hansu
Copy link
Member

hansu commented Sep 2, 2024

Not really a bugfix. It is more about to normalize the dots and commas.
Gmoccapy uses dots as decimal separator independent of the locale setting but the tooltable widget uses the locale settings. This is to have dots 'everywhere' like in the G-code.

@zz912
Copy link
Contributor Author

zz912 commented Sep 2, 2024

I have done it now for you. It is basically this in this case:

  1. git rebase -i HEAD~2 (assuming you are on top of your branch)
  2. Edit the file that will be opened:
pick 3bc36a8456 tooledit_widget - decimal point = dot
- pick 353a76b62f Update tooledit_widget.py
+ s 353a76b62f Update tooledit_widget.py

# Rebase f7cbbcce36..353a76b62f onto f7cbbcce36 (2 commands)
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
#                    commit's log message, unless -C is used, in which case
  1. Edit the commit message
  2. git push -f (be careful with force push!)

Exist way in web github? I dont use git.

@hansu
Copy link
Member

hansu commented Sep 2, 2024

Exist way in web github? I dont use git.

That one Andy mentioned, but that can be only done by the person who merges it.

@hansu hansu merged commit a2fa6f9 into LinuxCNC:master Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants