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.

Sign up for GitHub

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

Jump to bottom

User preferences are not restored between sessions. #445

Closed
TurtleShip opened this issue Apr 7, 2019 · 8 comments · Fixed by #456
Closed

User preferences are not restored between sessions. #445

TurtleShip opened this issue Apr 7, 2019 · 8 comments · Fixed by #456
Labels
Area: User Experience Issue primarily affects end user experience and are not covered by other areas fixed Pri: 2

Comments

@TurtleShip
Copy link
Contributor

Describe the bug
Looking at Calculator source code, it looks like we meant save a user's last selected category and units, and restore them when a calculator starts up. In other words, Calculator meant to support saving & restoring user preferences between sessions.
However, calculator is failing to restore saved units. It does restore saved category.

Steps To Reproduce

  1. Launch Calculator.
  2. Pick a category (ex> Area) and units (ex> Hectares to Acres )
  3. Close & re-launch Calculator.

Expected behavior
A user's last selected category and units should be restored when a calculator starts up (i.e. if you chose category Area and unit Hectares to Acres, and restarts Calculator, then Calculator should show category Area and unit Hectares to Acres ).
However, Calculator reverts back to the default units ( it does seem to restore category though ).

Screenshots
Please refer to this video.

For quick reference, refer to the below
preferences_bug_demo

Device and Application Information (please complete the following information):

  • OS Build: 10.0.17763.0
  • Architecture: X64
  • Application Version: 10.1812.10048.0

Additional context

  • UnitConverter::RestoreUserPreferences() is working correctly.
  • UnitConverter::SaveUserPreferences() is working correctly.
  • However, after restoring saved values on Calculator startup, Something is overriding the restored value to the default value.

Note: I would like to tackle this one and make a PR 🙂 if this issue gets accepted.
@TurtleShip TurtleShip mentioned this issue Apr 7, 2019
Merged
@grochocki grochocki added Area: User Experience Issue primarily affects end user experience and are not covered by other areas Bug Issue is a bug Pri: 2 triage approved Issue has been approved by Calculator team member labels Apr 8, 2019
@MicrosoftIssueBot
Copy link
Collaborator

This is your friendly Microsoft Issue Bot. I've seen this issue come in and have gone to tell a human about it.

@AbduAmeen1
Copy link
Contributor

Are you still planning on fixing this?

@ramank775
Copy link

ramank775 commented Apr 9, 2019 •
edited
Loading

Hi, i would like to work on this, if no one started on this already.

@TurtleShip
Copy link
Contributor Author

TurtleShip commented Apr 9, 2019 •
edited
Loading

Hi, there @cheezwhines , @ramank775

Yes, I am planning on fix this 😄

@TurtleShip
Copy link
Contributor Author

@grochocki could you assign this to me if possible please?

@grochocki
Copy link
Contributor

If you have write access to a repository, you can assign issues and pull requests to yourself, collaborators on personal projects, or members of your organization with read permissions on the repository. (Source)

Unfortunately, we cannot assign issues to folks outside of Microsoft due to this limitation. In general, if you are interested in working on an issue, leave a comment to let everyone know to prevent duplicate efforts.

@TurtleShip already expressed interest in working on this one after it was approved, so they can take first pass on it. Let us know if you get stuck or could use help from anyone else!

@TurtleShip TurtleShip mentioned this issue Apr 15, 2019
Merged
@ghost
Copy link

ghost commented Jul 21, 2019

Is @TurtleShip still working on this, or should someone else take over?

@HowardWolosky
Copy link
Member

Is @TurtleShip still working on this, or should someone else take over?

At this time, there is no additional work for a contributor to do on this PR. It's up to me to complete the historical analysis of how the code got into the state it currently is in, to ensure that this change isn't regressing an explicit change that was made in the past. I'll be setting aside dedicated time this week to complete that investigation. After that, we'll know if anything more needs to be done with this change, or if it can be merged as-is. If changes do remain to be made, those will be for @TurtleShip to do as this is their PR and they've exercised great patience while waiting for me to find the time to do that analysis.

@ghost ghost mentioned this issue Jul 21, 2019
Closed
HowardWolosky pushed a commit that referenced this issue Jul 29, 2019
@TurtleShip @HowardWolosky
Restore user preferences (#456)
af83226 
### Description of the changes:
**1) Do not set units to default values if they already have valid values** 
This fixes the actual issue. `UnitConverter::InitializeSelectedUnits()` ( this function resets all units to their default units if available for the current category ) gets called after `UnitConverterViewModel::RestoreUserPreferences()` ( this function restores user preferences ).
So Calculator has been restoring saved values, and then overriding the restored values with default values.
I modified `InitializeSelectedUnits()` so that we only initialize units only when they are not already set to valid units for the current category.

**2) Removed `m_isFirstTime`** 
I noticed that we are calling `RestoreUserPreferences()`  twice when Calculator starts up, and the function is restoring the same value both times

The below happens when Calculator starts up
1) On startup, in `UnitConverterViewModel::InitializeView()`, `RestoreUserPreferences()` is called.
2) `RestoreUserPreferences()` in turn triggers `OnUnitChanged()`
3) During the first call to `OnUnitChanged()`, m_IsFirstTime is `True`, so we call `RestoreUserPreferences()` again while also setting `m_IsFirstTime` to `False`. 
4) `RestoreUserPreference()` again triggers `OnUnitChanged()`
5) During the second call to `OnUnitChanged()`,  m_IsFirstTime is `False`, so we call `SaveUserPreferences()`

I think we should only call `SaveUserPreferences()` inside `OnUnitChanged()` since we already restored user preferences during view initialization. I can't really think of a reason to restore units after view has been initialized. This led me to just delete `m_isFirstTime`.


### How changes were validated:
Manually tested that units and the current category are properly selected when you quit and start Calculator.

![GifMaker_20190414182150911](https://user-images.githubusercontent.com/3166423/56102706-88f73400-5ee3-11e9-8bbf-7a0c8e051a5c.gif)

![GifMaker_20190414183403644](https://user-images.githubusercontent.com/3166423/56102763-f0ad7f00-5ee3-11e9-99ef-3b932f587393.gif)

## Fixes #445.
@MicrosoftIssueBot MicrosoftIssueBot added fixed and removed triage approved Issue has been approved by Calculator team member labels Jul 29, 2019
@MicrosoftIssueBot MicrosoftIssueBot removed the Bug Issue is a bug label Mar 30, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Area: User Experience Issue primarily affects end user experience and are not covered by other areas fixed Pri: 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Restore user preferences TurtleShip/calculator
Restore user preferences
6 participants
@grochocki @TurtleShip @HowardWolosky @ramank775 @MicrosoftIssueBot @AbduAmeen1