Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow comma as a decimal separator for SpinBox #80699

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

aXu-AP
Copy link
Contributor

@aXu-AP aXu-AP commented Aug 16, 2023

Fixes #80664
Add support for comma , as a decimal separator for SpinBox. This implementation allows for expressions like pow(2, 3) to be used as well. If you use comma to separate decimals, use semicolon ; to separate function parameters.
Change EditorSpinSlider behavior to match (currently EditorSpinSlider accepts commas but not expression with multiple parameters).
comma

Caveats: As comma replacing is triggered only in absence of parentheses, there's some cases where replacing doesn't trigger even if it would still make sense for example (1 + 0,5) * 2. However, if user needs to write something more complex than what can be inputted via numpad they probably don't mind using dot for decimals. Newer version handles these cases too.
This is also a change to an existing Control, but I can't see anybody relying on input failing whenever commas are present.

Bugsquad edit: Fixes #81989

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master a278c1b), it works as expected.

@MewPurPur
Copy link
Contributor

MewPurPur commented Aug 16, 2023

I have an implementation in #80541 which I think is better, even if a bit overkill. This implementation just checks for (s, while mine checks for functions so math stuff like (2 + 3,5) * 5 work still.

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Aug 17, 2023

Oh, I didn't see that! Good work. I just came up with a third option, which is elegant and handles even edge cases: What if we tried parsing the text and if parse fails then try replacing commas and reparse? I'm testing this rn (ie. waiting to build 😴).

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Aug 17, 2023

It seems that expression succeeds even if there is some commas just lying around - the rest just gets cut off (eg. 5,5 evaluates as 5). So my idea for checking for failed expression doesn't work out of the box.

Oh well, I think this is good enough, or use Mew's fancier version if you want to handle more cases 😀

@aaronfranke aaronfranke changed the title Use comma as a decimal separator for SpinBox Allow comma as a decimal separator for SpinBox Aug 17, 2023
@MewPurPur
Copy link
Contributor

Awh, the idea sounded very good in practice. Maybe there's some way to find whether Expression needs to do some meddling to make the string valid, so after two fails it trims off the "invalid part"? I don't know if the functionality exists in practice

@aXu-AP aXu-AP force-pushed the spin-box-comma-decimals branch from 1826b1a to 16a301a Compare August 17, 2023 07:16
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Aug 17, 2023

It worked out in the end! I just needed to try first converting commas, then in case of failure parse again without conversion. Now I couldn't find any edge cases, seems to work in all (sensible) cases.
Also I added conversion from semicolons ; to commas ,. This is how we do it in Finland if we need to separate multiple decimal numbers. After quick googling it seems to be the standard for French and some other countries as well.

@aXu-AP aXu-AP requested a review from Calinou August 17, 2023 07:30
@MewPurPur
Copy link
Contributor

Yay! This supersedes my PR beautifully then

Add support for comma ',' as a decimal separator for SpinBox. This implementation allows for expressions like `pow(2, 3)` to be used as well. If you use comma to separate decimals, use semicolon `;` to separate function parameters.
Change EditorSpinSlider behavior to match.
@aXu-AP aXu-AP force-pushed the spin-box-comma-decimals branch from 16a301a to 4d3dc0e Compare September 21, 2023 12:46
@aXu-AP
Copy link
Contributor Author

aXu-AP commented Sep 21, 2023

Rebased

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Seems fine, but seeing the duplicate code in EditorSpinSlider and SpinBox I wonder if maybe it should be a static SpinBox method, with text as a parameter?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 21, 2023

Apparently this also fixes #81989 (and consequently, supersedes #82020).

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 28, 2023
@YuriSizov YuriSizov merged commit 4f0e2ea into godotengine:master Sep 28, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@aXu-AP aXu-AP deleted the spin-box-comma-decimals branch September 28, 2023 18:29
@Crystalwarrior
Copy link

This problem came back in Godot 4.3 :/

@aXu-AP
Copy link
Contributor Author

aXu-AP commented Nov 16, 2024

Can you elaborate? For me it seems to be working both in 4.3 and in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants