osmandapp / OsmAnd

OsmAnd
https://osmand.net
Other
4.67k stars 1.02k forks source link

Presets for Navigation Widgets Somehow Broken #2744

Closed sonora closed 8 years ago

sonora commented 8 years ago

I just notice that for a fresh installation of OsmAnd the appearance of all (left stack) navigation widgets for all app modes seems to be set to off. (We have a bunch of new users who did not even know they exist, due to this bug.)

Defaults should actually be: regWidget("next_turn", exceptPedestrianAndDefault); regWidget("next_turn_small", pedestrian); regWidget("next_next_turn", exceptPedestrianAndDefault);

But this seems somehow not to work any more.

sonora commented 8 years ago

PS: Looks like we have issues with more default values: Things which make really sense for e.g. the car mode like speed limit or estimated navigation arrival time also seem not be configured anymore by default, so it looks like at least both the left and the right stack widgets are affected.

sonora commented 8 years ago

@GaidamakUA, @crimean : Could this be a leftover from refactoring MapWidgetRegistry.java in March and April?

vshcherb commented 8 years ago

Can't confirm the issue, all widgets appear once you turn on navigation. Just installed fresh installation, indeed for default mode we don't have any widgets but that's not an issue. Tested on latest nightly default build (fresh installation was done).

sonora commented 8 years ago

It is true, I also tested the latest nightly now, and cannot reproduce anymore either. I will keep an eye open if this comes up under some circumstances, but I wouldn't now which, we are talking fresh installs after all. My actually be a phantom issue. :)

sonora commented 8 years ago

I have received yet another complaint about this very issue, and I think that it may actually be as follows:

User has a completely fresh install of OsmAnd, and makes some changes in "Configure screen" for the Browse map profile (like maybe adding the altitude widget to the screen, or changing the Map style). Since we sort of use the "Browse map" profile as a default, some settings from there are propagated to the other profiles, and maybe we accidentally propagate the settings for the "Next Turn", "Next turn (small)", and "Second next turn" all as "OFF".

User showed me a freshly installed OmAnd instance where in deed all these were off for all 3 profiles Car/Bicycle/Pedestrian, contrary to our installation defaults.

Will re-open this so we remember to investigate.

vshcherb commented 8 years ago

Need to double check.

sonora commented 8 years ago

@vshcherb, @crimean : I investigated how to reproduce this, and it turns out the glitch occurs after making a change to the default (Browse map's) "Configure screen" settings, and then only after the next app restart (and only If you had not previously changed any "Configure screen" settings for any other app profiles).

Here is how to reproduce: Steps in brackets [ ] are for verification only, but not needed to trigger the bug:

Now, as you can verify in the Car, Bicycle, Pedestrian profiles' Configure screen settings, the formerly correct presets have all been set to "off" for many widgets (I think for exactly all we parameterize in ApplicationMode.java)

Interestingly, the issue will not (and also subsequently never) show up for any app mode for which you have first made a "Configure screen" setting before you made one for the default, profile.

I think it is about like this: The widget visibility defaults for all profiles are being correctly initialized, but are not being written to memory, until you make a change on a profile's "Configure screen" menu. So for all profiles you have never made a change for, when you first make a change to the default profile, its settings take precedence over non-present settings for the other profiles.

sonora commented 8 years ago

I could verify that the fix could be as follows:

Upon the first app start after fresh install, we trigger for each profile the "Reset to defaults" action, i.e. exactly the code (I think MapWidgetRegistry.java resetToDefault() which is performed if a user taps the "Defaults" button in the top-right corner of the "Configure screen" dialog.

This seems to perform the "write to settings" operations needed to avoid the glitch.

vshcherb commented 8 years ago

That's a feature for settings. We need to treat it carefully. I propose it to change to the following: Profile setting S, profies A (default profile!), B, C (1) Only global default setting is specified (to v1), then in A profile -v1, B profile - v1 (2) Global Default Setting - v1, A default setting - v2, then A - v1, B - v2 (3) Global Default Setting - v1, Manually A setting - v3, then A - v3, B - v3, C - v3 (this is correct and intended behavior for some settings, though might be confusing) (4) Global Default Setting - v1, A default setting - v2, B default setting - v3, then A - v2, B - v3, C - v1 (5) Global Default Setting - v1, B default setting - v3, A manually - v4, then A - v4, B - v3 (because v2 !=v3) , C - v4

I guess (5) and (3) this is what causing issue

vshcherb commented 8 years ago

Though I checked this method (most important) and it looks exactly correct https://github.com/osmandapp/Osmand/blob/master/OsmAnd/src/net/osmand/plus/OsmandSettings.java#L327

sonora commented 8 years ago

Victor, I think our logic (1)-(5) above is correct, we have had it successfully for years.

I think the glitch happens in (2): We correctly specify the global and some deviating profile defaults, but upon a fresh install the profile defaults are are not written to the system memory.

So if the first change you make to any setting after a fresh install is for the default profile, the default for the other profiles is also overwritten even if there is a default for that profile. And that is the glitch, it should only set it where there is no separate default for that profile.

sonora commented 8 years ago

PS: Please note that all "settings" affected by this bug are actually solely defined in ApplicationMode.java, not in any of our *Settings Activities, where we normally register settings (and for which there is no bug).

sonora commented 8 years ago

From my testing, progres as follows:

It works ok now, regardless of whether the profile change is caused by starting the follow-navigation mode, or manually during navigation, if there is no day-night change caused by the profile change. If, however, the profile change causes a day->night or night->day transition, the defailt (globe) icon is shown for the new profile..

Not tested on fresh installs, though, only on installs which had previously exhoiited the glitch, so the observation may be coincidental.