brewtarget-devs team mailing list archive

[mik firestone <mikfire@xxxxxxxxx>]

https://gitorious.org/brewtarget/mikfires-brewtarget/commits/feature/unitrefactor

I don't think a complete and utter rewrite is required. Check that branch
to see what I am thinking about. It mostly does the right thing -- if you
enter 5 gal into a field, it will find the appropriate unit based on system
defaults. If you are using US, you will get 5 us gallons, if you are using
imperial you will get 5 imperial gallons.

It will do that regardless of what you have selected to use as the display
unit for the field -- eg, if your calculated batch size is set to
"Imperial" and your system default is set to "US Customary" and you enter 5
gal, you will get 5 US gallons. I think I can address that but haven't yet,
which is why I haven't made the merge request.

It also doesn't handle entering just "5" gracefully, but I think I can fix
that too.

A few questions. If you dig into the new code, you will find a method
called loadMap(). What it does is load a QMap with Unit pointers. I would
really like that to be in the constructor, but when I tried that all of the
Unit pointers were null. Any idea if I can fix that? I am guessing it is a
late/early binding kind of problem but ...

Mik


On Wed, Oct 8, 2014 at 7:52 PM, Philip Lee <rocketman768@xxxxxxxxx> wrote:

> So, this has been an unreported issue. Welp, file a bug. Maybe it's time
> to sketch up a replacement for the crappy unit system code now.
>
> On Tue, Oct 7, 2014 at 9:06 AM, Mik Firestone <mfirestone@xxxxxxxx> wrote:
>
>>  Hmm. Up for a quick thought experiment?
>>
>> Assume that I am using SI as my default unit for all things, and have
>> never played with the units&scales. I read an interesting recipe on line
>> for a 5 gallon batch and enter it, verbatim, into brewtarget. In the target
>> batch field, do I get 19L or 22L? Just telling me "gallons" is not enough
>> to determine if you want US gallons or Imperial gallons. The default
>> behavior right now is to just assume you meant US gallons. Unless....
>>
>> Another experiment, this one not so thoughty. Open any version of
>> brewtarget after I implemented units&scales. Right click the target batch
>> size label and set it to US Customary. Enter 5 gal into the field. Right
>> click the label again, and set the unit to British Imperial. Enter 4.5 gals
>> (roughly the same size). Right click the label one last time, set it back
>> to US Customary and enter 5 gal. I am willing to bet the results will be a
>> bit surprising.
>>
>> The nameToUnits hash is keyed by the unit name, e.g., "L" will map to
>> Units::liters. If you first load the US volume system and then the
>> Imperial, the Imperial unit system over-writes the entries for the US
>> volume system. Since each unit is only loaded once, this has some very
>> interesting side effects. I haven't tried it yet, but I am willing to bet
>> you can cause the same problem by switching the default from US Customary
>> -> British Imperial -> US Customary with data entry at each setting. If
>> that works, it means the "bug" has been around for a while.
>>
>> The reason the new code exercises this bug so easily is I am using the
>> same code path for the right click magic as I am for data input which
>> causes the hash to be loaded by simply right clicking instead of needing
>> the data entry. It has taken me weeks to figure that out, by the way.
>>
>> Because we have two systems using the same names, I can't really fix the
>> hash. I simply have no way of understanding what a user means with "5 gal"
>> -- 19L and 22L are both correct and acceptable. I am going to poke at this
>> a bit longer to see if I can at least reduce the bug to its original form
>> but no promises.
>>
>> Mik
>>
>>  On 10/05/2014 12:22 PM, Philip Lee wrote:
>>
>> Bottom line is that it needs to work.
>>
>> Can you put this up as a merge request so we can take a look? I won't merge
>> until your work is done. If I merge before we fix the bug, I'll open a bug
>> ID to track it and make sure it doesn't go out in a release.
>>
>> On Fri, Oct 3, 2014 at 11:34 PM, mik firestone <mikfire@xxxxxxxxx> <mikfire@xxxxxxxxx> wrote:
>>
>>
>>  Ok. I am in the middle of a *huge* refactoring. In the course of simply
>> (bwahahaha) enabling units and scales for any and all appropriate input
>> fields, I have pretty much rewritten some of the core functionality in
>> brewtarget (like Brewtarget::displayAmount()).
>>
>> This works, and it works rather nicely and elegantly. But. There is a bug.
>>
>> If you switch a volume input field from SI -> us customary -> british
>> imperial  -> us customary -> SI things will go somewhat pear shaped. This
>> is a side effect of how the QMap UnitSystem::nameToUnit works, the fact
>> that us customary and british imperial use the same names for the volume
>> measures and the way I've changed things around. If any one of those things
>> changes, the bug goes away.
>>
>> This is obviously a bug, but how much of a bug is it? I am not 100%
>> certain it can be easily fixed. I am 100% certain I do not want to refactor
>> the UnitSystem stuff right now. If it is considered a big bug, I will
>> invest the time to determine if I can fix it. If we think it isn't a likely
>> bug, I will continue with what I'm doing.
>>
>> These changes are rather sweeping, so it won't be a simply matter of
>> backing them out. If this is a big problem and I cannot fix it, I will have
>> to stop working on this branch.
>>
>> Anybody have suggestions?
>>
>> mik
>>
>> --
>> In a world of ninja v. pirate, I pilot a Gundam
>>
>>
>> ------------------------------------------------------------------------------
>> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
>> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
>> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
>> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
>> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
>> _______________________________________________
>> brewtarget-devs mailing listbrewtarget-devs@lists.sourceforge.nethttps://lists.sourceforge.net/lists/listinfo/brewtarget-devs
>>
>>
>>
>
>
> --
> Philip G. Lee
> www.linkedin.com/in/philipgreggorylee
>
>
> ------------------------------------------------------------------------------
> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
>
> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
> _______________________________________________
> brewtarget-devs mailing list
> brewtarget-devs@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/brewtarget-devs
>
>


-- 
In a world of ninja v. pirate, I pilot a Gundam