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

Wrong conversion from grams to kilograms in hops input #65

Closed
renato opened this Issue Jul 31, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@renato
Copy link

renato commented Jul 31, 2015

I've been using the .deb downloaded version so far, but today I tried the repository version. This bug happens on both branches (master and stable/2.2.0).

When trying to input an amount that has 1g precision in the hop amount field, Brewtarget automatically converts the input value to kilograms.

Multiples of 10 (10, 20, 30...) are accepted, anything else is converted to kilograms (1, 5, 7, 9, 15, 44, etc).

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Aug 1, 2015

Is is "5" or "5g" or "5 g" or something else that gets converted to 5 kg?

Also, is this happening in the hops tab or somewhere else?

@renato

This comment has been minimized.

Copy link

renato commented Aug 1, 2015

"5", "5g", "5 g", all of them get converted to 5 kg.

When I type "10", it gets converted to 10 kg. But "10g" and "10 g" are correctly accepted.

I'm talking about the Hops tab, where I assume should consider grams automatically, as a kilogram hop use is unlikely for homebrewers.

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Aug 1, 2015

I'm curious what is the result when you right click the "Amount" then select Scale->Grams.

@renato

This comment has been minimized.

Copy link

renato commented Aug 1, 2015

With the "Default" Scale set, I typed 5g and got 5kg. Then I changed to Grams and it showed as "5.000,000 g". Typing "5", "5g" or "5 g" gives me, again, "5.000,000 g".

@renato

This comment has been minimized.

Copy link

renato commented Aug 3, 2015

I tried debugging the situation and realized that the value in the database is correct.

In https://github.com/Brewtarget/brewtarget/blob/master/src/hop.cpp#L299, get("amount") returns "0.005" kilograms, correctly.

Brewtarget::toDouble() converts it to "5", because it uses QLocale.toDouble(): https://github.com/Brewtarget/brewtarget/blob/master/src/brewtarget.cpp#L1069 and I'm using "pt_BR.UTF-8", which uses period as thousands separator and comma as decimal separator.

I commented out the QLocale.toDouble(), going straight to text.toDouble(), and the values are correctly shown.

This problem isn't reproducible with LC_ALL=en_US.UTF-8.

Even when the problem doesn't happen (using en_US, for instance), I need to type "5g" for 5 grams, otherwise Brewtarget sets the amount to 5 kg. As a suggestion, I don't think one would use kg for hops amount for any batch <1000L, so it shouldn't be the default input.

@renato

This comment has been minimized.

Copy link

renato commented Aug 6, 2015

So @rocketman768, do we need the QLocale.toDouble()? It is the reason "0.005" is becoming "5". Any reason not to go straight to QString.toDouble()?

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Aug 6, 2015

Code to force the problem

double Brewtarget::toDouble(QString text, bool* ok)
{
   double ret = 0.0;
   bool success = false;
   // Option 1
   //QLocale sysDefault(QLocale::English, QLocale::UnitedStates);
   // Option 2
   QLocale sysDefault(QLocale::Portuguese, QLocale::Brazil);

   ret = sysDefault.toDouble(text,&success);

   qDebug() << "text: " << text << " success: " << success << " ret: " << ret;
   ...
}

Procedure

Enter "3 g" into the field.

Option 1: en_US

text: "0.003" success: true ret: 0.003

Option 2: pt_BR

text: "0.003" success: true ret: 3

Analysis

Each time, the string "0.003" is being passed into toDouble() as an unlocalized string. Need to find where this string originates and either localize it there, or use QString::toDouble() in this situation.

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Aug 7, 2015

Found the problem, and it's a big one, so thank you very much Renato!

double Hop::amount_kg()          const { return Brewtarget::toDouble(get("amount").toString(), "Hop::amount_kg()"); }

This is just supposed to return a simple double but the Brewtarget::toDouble is a localized conversion now. I changed it to the following, and it works

double Hop::amount_kg()          const { return get("amount").toDouble(); }

This needs to be done for all the double properties of all the ingredient classes.

@rocketman768

This comment has been minimized.

Copy link
Member

rocketman768 commented Aug 8, 2015

@renato can you see if the pull request #71 fixes the issue for you?

@rocketman768 rocketman768 self-assigned this Aug 8, 2015

@rocketman768 rocketman768 added this to the v2.2.0 milestone Aug 8, 2015

@rocketman768 rocketman768 added the bug label Aug 8, 2015

rocketman768 added a commit that referenced this issue Aug 15, 2015

Merge pull request #71 from rocketman768/bug/hop-amount
Fixes #65: bad ingredient amount behavior in non-US locales

rocketman768 added a commit that referenced this issue Aug 15, 2015

Fixes #65: bad ingredient amount behavior in non-US locales
The problem was that simple double properties of the ingredients
were being passed through the localization procedures in the
Brewtarget class when they should not have been.
@renato

This comment has been minimized.

Copy link

renato commented Aug 17, 2015

@rocketman768 It does, thank you very much.

The only thing still subject to improvement in amounts input, IMHO, is the "default by type". I mean, the defaults are Kg and L, which works for fermentables or overall batch volumes, but not for hops (which are probably in grams), miscellaneous or yeasts. Of course needing to type the unit is not a big deal, but it is something to think of for future improvements.

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