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

Remove "approx." keyword and improve decimal point/comma handling #70

Merged
merged 3 commits into from
Jan 24, 2022

Conversation

nathansgithub
Copy link
Contributor

@nathansgithub nathansgithub commented Dec 10, 2021

This changes the calculator behavior to stop showing "approx." in front of numbers when it has more decimal places than can be shown, by enabling qalc's Unicode support. It also explicitly sets the decimal mark to decimal points when that's preferred over commas.

For issue #71

…d forced reset to decimal dots instead of commas when needed
@13r0ck
Copy link

13r0ck commented Dec 14, 2021

Thank you for the PR!
It seems though that this bug doesn't exist in Pop!_OS 21.10. As that version will be released in < 24 hours, it doesn't make sense to merge this PR.

@jacobgkau
Copy link
Member

It seems though that this bug doesn't exist in Pop!_OS 21.10. As that version will be released in < 24 hours, it doesn't make sense to merge this PR.

We also want to consider Pop!_OS 20.04 LTS as well as other distributions. It looks like this is still happening in 20.04 LTS, so we do want to review the PR.

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

Before:
Screenshot from 2021-12-14 13-08-58

After:
Screenshot from 2021-12-14 13-14-41

@nathansgithub Your issue #71 indicated this should use a ≈ symbol instead of the word "approx." I don't see that ≈ symbol present, and it seems like there should be some indication when the result isn't exact. Do you know why that isn't showing?

@jacobgkau
Copy link
Member

Oh, I see, you were saying the ≈ is stripped the same was a = currently is. I think this would be a good change, if we can get the ≈ to show up in the result visually.

@nathansgithub
Copy link
Contributor Author

nathansgithub commented Dec 14, 2021

@jacobgkau Yeah, I meant it uses the ≈ symbol behind the scenes. Do you think it's important to distinguish an approximate result, and how would you want to do that? I do want to keep the use case where you can hit return and immediately use the previous result as part of a new calculation (and that's the main reason I suggested this change).

@jacobgkau
Copy link
Member

I do want to keep the use case where you can hit return and immediately use the previous result as part of a new calculation

100% agreed on that point, that is an intended feature.

Do you think it's important to distinguish an approximate result, and how would you want to do that?

It does seem important. I would think just having at the front of the result would be sufficient. (I'm pretty sure that was the behavior at some point; it might have changed when it moved from MathJS in pop-shell to qalc in pop-launcher.)

If making the launcher treat as a prefix for this plugin is the simplest way to accomplish that, then that sounds fine (pending engineering review, of course.) The other option would be getting it to only bring the rest of the result after the into the search field when pressing enter.

@nathansgithub
Copy link
Contributor Author

nathansgithub commented Dec 14, 2021

@jacobgkau I think this fixes it. It will leave the ≈ in front of approximate results but keep the same behavior for other results. The existing code already filters out ≈ before the value is sent to the Fill() function that fills the search field.

@nathansgithub
Copy link
Contributor Author

I upgraded my system to 21.10 and noticed some changes in qalc's formatting, so I added a couple changes to the code.

  1. It is showing the calculation in multiple steps when the fractions can be simplified. So I changed the code to look from the right of the string for ≈ or = instead of from the left. Examples of equations that this helps normalize:
7 / 6 = 1 + 1/6 ≈ 1.166666667
100 / 40 = 5/2 = 2 + 1/2 = 2.5
  1. The maximum digits after the decimal increased from 7 to 9. I explicitly set that to 9 so tests are consistent.

@nathansgithub
Copy link
Contributor Author

@jacobgkau Let me know if there's anything else you want me to change. I think this PR will make the results clearer on both 21.10 and 20.04. Right now, the equation 7/6 resolves to:
approx. 1.1666667 in 20.04 (the last I checked, unless qalc was updated there)
1 + 1/6 ≈ 1.166666667 in 21.10

This change will make it so the answer on both is just ≈ 1.166666667, which I think makes more sense and gives a more consistent behavior if you are making several calculations based off each other in the launcher.

Thanks!

@jacobgkau jacobgkau self-assigned this Jan 24, 2022
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

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

21.10: working as expected. Querying 2/3 returns ≈ 0.666666667, pressing Enter on that result pre-fills = 0.6666666667, and from there, continuing to press Enter cycles between that and 6666666667/10000000000.

approx

On 20.04 LTS, the behavior now looks nearly identical, only (due to the older Qalculate! version) the decimal is not converted back to a fraction, so the decimal is as far as the results can go without modifying the query again:

approx-2004

Other calculations are still being solved as expected. This change looks good to me.

@jacobgkau jacobgkau removed their assignment Jan 24, 2022
@jacobgkau jacobgkau requested a review from a team January 24, 2022 23:23
@mmstick mmstick merged commit 1d031d9 into pop-os:master Jan 24, 2022
@nathansgithub
Copy link
Contributor Author

Awesome, thank you!

@jacobgkau jacobgkau linked an issue Mar 28, 2022 that may be closed by this pull request
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.

Remove "approx." keyword and improve decimal point/comma handling
4 participants