Current settings should be saved on killing app too (not only on closing app)
ClosedPublic

Authored by sanjibanb on Sep 26 2016, 8:43 PM.

Details

Reviewers
nienhueser
rahn

Diff Detail

Repository
R34 Marble
Lint
Lint Skipped
Unit
Unit Tests Skipped
sanjibanb updated this revision to Diff 6932.Sep 26 2016, 8:43 PM
sanjibanb retitled this revision from to Current settings should be saved on killing app too (not only on closing app).
sanjibanb updated this object.
sanjibanb edited the test plan for this revision. (Show Details)
sanjibanb added reviewers: nienhueser, rahn, kossebau.
sanjibanb set the repository for this revision to R34 Marble.
sanjibanb added a project: Marble.
sanjibanb added a subscriber: Marble.
sanjibanb updated this revision to Diff 6934.Sep 26 2016, 8:54 PM

Add braces

sanjibanb updated this revision to Diff 14850.May 26 2017, 3:31 AM

Updating diff to latest code.

nienhueser requested changes to this revision.May 26 2017, 7:33 PM

From what I remember the problem goes like this: On Android, change some setting, then open the application list and swipe Marble Maps to the right to kill it. The settings were not saved, so not restored upon the next start.

From what I understand from the patch, it works around it by saving settings when you open the application list and Marble Maps becomes not active. This seems ok, especially for the suspended state. What worries me is to load settings when it becomes active. This seems unneeded to me, and also dangerous in case a state change event would not get caught for some reason. Can you check if the patch works as well if you only save settings in suspended and inactive state, but do not load them here?

This revision now requires changes to proceed.May 26 2017, 7:33 PM
kossebau resigned from this revision.Jun 7 2017, 3:48 PM
sanjibanb updated this revision to Diff 15439.Jun 13 2017, 8:39 PM
sanjibanb edited edge metadata.
sanjibanb removed a reviewer: kossebau.

Makes sense. I modified the patch so that it only saves settings in suspended and inactive states, but not load them during any state change, and verified that it still resolves the issue.

nienhueser accepted this revision.Jun 14 2017, 3:45 AM

Looks good, please push.

This revision is now accepted and ready to land.Jun 14 2017, 3:45 AM