Add support for Pyeong, a Korean floorspace unit.

[TurtleShip]

Fixes #382

Description of the changes:

• Add Pyeong as an Area conversion unit.
• Pyeong shows up only if the user's current region is Korea ( i.e. region is either KP or KR ).
• Added Korean translation for Pyeong (평). For other locales, we default to English ( Pyeong ).

How changes were validated:

Manually tested the below

1. For non-Korean regions, Pyeong does not show up.

2. Korean region with Korean locale => Pyeong shows up and Pyeong is correctly translated.
   

3. Korean region with English locale => Pyeong shows up and Pyeong is in English.
   

4. Korean region with simplified Chinese locale => Pyeong shows up and Pyeong is in English.
   

[msftclas]

All CLA requirements met.

[rudyhuyn]

Good job!!

Can you also modify UnitConverter::RestoreUserPreference so it will ignore an optional unit if not available for the current culture? (In case a user selects an optional unit, close the app, select a region not supporting this unit, and open the app again)

[TurtleShip]

Thanks @rudyhuyn !

So I've been reading through how Calculator preferences work, and I noticed 1 bug and 1 possible improvement. But before I go into them, I would like to note that ignore an optional unit if not available for the current culture already happens through UnitConverter::InitializeSelectedUnits().

So in case a user choose Pyeong, closes Calculator, changes the current region to non-Korea (e..g US), and starts up Calculator, the app won't attempt to load Pyeong. In fact, at the moment, the app will fall back to the default conversion values ( it looks like that may not be intended behaviour though - I think the current intended behaviour, at least from the code, was to show no unit (EMPTY_UNIT and let the user pick one ).

Circling back to the bug & improvement I mentioned earlier, here are what I found.
I also uploaded demo videos here for bug & possible improvement. Gif I attached in the comment are of poor quality so I recommend viewing the videos.

Bug 1: saving user preference doesn't seem to work correctly.

Specifically,

1. UnitConverter::RestoreUserPreferences is working correctly.
2. UnitConverter::SaveUserPreferences() is working correctly.
3. However, after restoring saved values on calculator startup, Something is overriding the restored value to the default value ☹️
   Here is a demo
   

Possible improvement 1: Instead of showing no unit, we should display a default value (or the first available value if any) if we fail to find units that we saved from last session.

So this behaviour where we don't show any unit probably never really got observed in real life because of Bug 1. I made a little tweak, and the below is what is looks like if we actually defaulted to EMPTY_UNIT.

I am not sure if the above was a product decision though ( hence I labeled it as a possible improvement instead of a bug ).

@grochocki What are your opinions on the above mentioned bug & possible improvement?
I can file them separately and make PRs as well unless of course they are known issues and already being addressed.

Cheers!

[TurtleShip]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ TurtleShip sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

Hmm, I tried 3 times already with an hour or so interval, but I agree button seems perma disabled. I can't click it. 😢

[grochocki]

Unfortunately, our Localizaton system is not yet able to directly accept localization changes directly through this repo, so we should only add the english resource.

[TurtleShip]

Gotcha. I removed translations for Korean :)

[grochocki]

So in case a user choose Pyeong, closes Calculator, changes the current region to non-Korea (e..g US), and starts up Calculator, the app won't attempt to load Pyeong. In fact, at the moment, the app will fall back to the default conversion values

We do not expect users to change region often, so resetting to the defaults is fine.

What are your opinions on the above mentioned bug & possible improvement?
I can file them separately and make PRs as well unless of course they are known issues and already being addressed.

Good catch! There is not anything tracking this, so lets create a new issue and handle that separately from this PR.

IRT CLA agreement issues, I would maybe try again and see if it was just a temporary issue. Otherwise, @HowardWolosky, do you have any ideas?

[rudyhuyn]

So in case a user choose Pyeong, closes Calculator, changes the current region to non-Korea (e..g US), and starts up Calculator, the app won't attempt to load Pyeong.

Luckily (or unfortunately) InitializeSelectedUnits is called just after RestoreUserPreferences, resetting FromUnit and ToUnit to their default values. It seems to not be intentional, this is due to the boolean m_IsFirstTime and OnCategoryChanged called twice when the application starts.

Any future PR (to fix the restoration or any changes on UnitConverter) can modify this behavior, so it 's better to prevent FromUnit and ToUnit to be set to an unsupported unit when we restore them. The same issue would happen if a unit is removed by an update, the app would unfortunately crash.

[TurtleShip]

@grochocki
Looks like I have triggered an abuse detection mechanism.
I will try signing it again tmrw

[TurtleShip]

@rudyhuyn I addressed your comment regarding not restoring from saved units if they are not valid with the current available units!

[TurtleShip]

@grochocki
I created issue for not restoring user preferences #445 .

I haven't created one for replacing EMPTY_UNIT with a valid unit. Shall I file it under Bug or Feature request?
Also, this issue where we may show no unit when calculator launches (because of EMPTY_UNIT) is not currently happening, but will start happening once #445 is fixed. Is there a way to strongly suggest to fix this issue first before #445, or concurrently?

Cheers!

[rudyhuyn]

The change is great!

Also, this issue where we may show no unit when calculator launches (because of EMPTY_UNIT) is not currently happening, but will start happening once #445 is fixed. Is there a way to strongly suggest to fix this issue first before #445, or concurrently?

It should be fine, if UnitConverter::RestoreUserPreference isn't able to set m_fromType or m_toType (because the saved value is Empty or the unit is not available), we should keep values set by InitializeSelectedUnits, so if the issue is currently not happening, this should still not happen after #445 in this case.

[TurtleShip]

Thanks, @rudyhuyn !

So to confirm my understanding of what you said, an issue where no unit is selected should not occur after fixing user preferences issue because of the below flow?

1. Calculator fails to restore saved values ( either there are no saved values or saved values are not valid )
2. Calculator will select default values ( this seems to be the case from manual testing but I haven't confirmed in code )
3. InitializeSelectedUnits will choose default values

[rudyhuyn]

I'm AFK, but in my memory InitializeSelectedUnits is first called, then RestoreUserPreference, so if we can't restore, we just need to keep the current values as is. The issue is that OnCategoryChanged is called twice and the way we detect if the category has been changed by the code or by the user (via UI) is wrong (see code related to m_isFirstTime). Let's chat about it tomorrow IRL 😀📦

[TurtleShip]

For prosperity, @rudyhuyn and I talked and we agree that an issue where no unit is selected is not a concern ( my understanding of InitializeSelectedUnits was incorrect ).

So to sum up, the only real issue is preferences not being correctly restored, and we created issue for it - #445.

[TurtleShip]

btw, is there a further action I need to take for review? 😄

[TurtleShip]

Hi, @danbelcher-MSFT

Thanks for your reviews! :)
I've addressed your comments and left one clarifying question.

[danbelcher-MSFT]

Thanks, Seulgi!