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

Fix decimal separator handling when loading real numbers #4547

Merged
merged 3 commits into from Sep 12, 2018

Conversation

Projects
2 participants
@PhysSong
Member

PhysSong commented Aug 21, 2018

  • Remove broken DataFile::LocaleHelper.
    When saving float values into QDomElement, Qt uses QString::setNum. As of Qt 4, the function always uses the C locale, so we don't need to worry about decimal separators when saving files.
    The problem is Qt5 doesn't provide a way to handle both period-separated(1.2) and comma-separated(1,2) real numbers in one function call because QString::toFloat always use the C locale only. QLocale::toFloat always tries its specific locale only.
  • Add new LocaleHelper.h and use it
    To solve the problem, I created a function which tries both period and comma. Using it will work on any platforms.

Fixes #4442.

@PhysSong PhysSong changed the title from Fix float point handling due to locale issues to Fix decimal separator handling due to locale Aug 21, 2018

@PhysSong PhysSong added this to In progress in Release LMMS 1.2.0-RC7 Aug 21, 2018

@PhysSong PhysSong changed the title from Fix decimal separator handling due to locale to Fix decimal separator handling when loading real numbers Aug 21, 2018

@JohannesLorenz

This comment has been minimized.

Show comment
Hide comment
@JohannesLorenz

JohannesLorenz Aug 21, 2018

Contributor

I could code-read and test this branch. Let me know when it's ready.

Contributor

JohannesLorenz commented Aug 21, 2018

I could code-read and test this branch. Let me know when it's ready.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 22, 2018

Member

I used inline keyword to avoid errors for now. Should I move the definition to a source file instead?

Member

PhysSong commented Aug 22, 2018

I used inline keyword to avoid errors for now. Should I move the definition to a source file instead?

@JohannesLorenz

This comment has been minimized.

Show comment
Hide comment
@JohannesLorenz

JohannesLorenz Aug 22, 2018

Contributor

@PhysSong Sounds good. Also maybe add doxygen-like comments so one can guess what the functions do by just looking at the header.

Contributor

JohannesLorenz commented Aug 22, 2018

@PhysSong Sounds good. Also maybe add doxygen-like comments so one can guess what the functions do by just looking at the header.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 10, 2018

Member

There are some functions defined in other header files. I think my helper functions are not too large and it's fine to leave them inline.

Also maybe add doxygen-like comments so one can guess what the functions do by just looking at the header.

Updated the description at the beginning.

Member

PhysSong commented Sep 10, 2018

There are some functions defined in other header files. I think my helper functions are not too large and it's fine to leave them inline.

Also maybe add doxygen-like comments so one can guess what the functions do by just looking at the header.

Updated the description at the beginning.

@PhysSong PhysSong merged commit f37ca49 into LMMS:stable-1.2 Sep 12, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PhysSong PhysSong deleted the PhysSong:locale branch Sep 12, 2018

@PhysSong PhysSong moved this from In progress to Done in Release LMMS 1.2.0-RC7 Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment