search

InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.63k stars 902 forks source link

ApplicationList: Reset app menu screen when loading watch face #2009

Closed vkareh closed 3 months ago

vkareh commented 4 months ago

This prevents the application list from loading in the last used screen and instead goes back to the first screen whenever the watch face is loaded.

Fixes #2006

github-actions[bot] commented 4 months ago
Build size and comparison to main: Section Size Difference
text 377096B 16B
data 940B 0B
bss 63516B 0B
JF002 commented 4 months ago

Have you checked why the page number of the launcher is actually stored? I wouldn't want to apply a change that conflicts with the original intend of the code.

I think it would also be a good idea to check that the behavior reported in #2006 does not happen for other apps as well, like settings for example (that you can open with a double press on the button iirc)? Maybe the fix should be more generic than it currently is?

vkareh commented 4 months ago

Have you checked why the page number of the launcher is actually stored? I wouldn't want to apply a change that conflicts with the original intend of the code.

Yes. It was added as part of #217 with the description:

  • Go back to the same menu page when we leave an app

This behaviour is preserved, as we only reset the page when we're not in an app.

I think it would also be a good idea to check that the behavior reported in https://github.com/InfiniTimeOrg/InfiniTime/issues/2006 does not happen for other apps as well, like settings for example (that you can open with a double press on the button iirc)? Maybe the fix should be more generic than it currently is?

This issue doesn't happen in the settings, since we don't store the current screen there.

FintasticMan commented 4 months ago

Git bisect says the bug was introduced in 39bc166e549e8ccae75468aa2dd3613d51f54e27. There was no call to SetAppMenu removed in that commit... Why didn't the bug occur before?

JF002 commented 3 months ago

Git bisect says the bug was introduced in https://github.com/InfiniTimeOrg/InfiniTime/commit/39bc166e549e8ccae75468aa2dd3613d51f54e27. There was no call to SetAppMenu removed in that commit... Why didn't the bug occur before?

The call to Settings::SetAppMenu() was done in the ctor of the Clock class, which was removed in this https://github.com/InfiniTimeOrg/InfiniTime/commit/39bc166e549e8ccae75468aa2dd3613d51f54e27 commit!

And I think the @vkareh found a better place for this in DisplayApp so that watch faces do not have to manage this state.