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.

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

Extract Main Menu to Fragment to prevent lifecycle crashes #5829

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Nov 9, 2023

A few crashes we're seeing seem to be due to MainMenuActivity#onResume being called after we've returned from onCreate early (like in the case of there being no project or if there was a crash previously) even though we don't think that should be possible. Example reports can be found here and here.

I wanted to have a quick go at moving the Main Menu view and logic into a Fragment as this would allow us to only load the Fragment when we want to actually display it which prevents problems with lifecycle being called that we want to skip.

Why is this the best possible solution? Were any other approaches considered?

The original solution to this was to have a "launch" Activity with noHistory=true (or that finishes itself) that handles the initial navigation. However, this causes problems for application restarts as that Activity would not be part of the back stack so the logic would be skipped.

Like we've experimented with in EntityBrowserActivity, I think we most likely want the "form management" portion of the app to be hosted in a single Activity and multiple Fragments using the new(ish) Jetpack Navigation component, and this is a good first step towards that. I should also note that I've not made the switch to things like Fragment results or MenuProvider in MainMenuFragment to keep this a short piece of work. That's definitely something we can add on later.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Only the main menu has changed, but it's obviously an important part of the app so playing with it as much as possible would be good.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@seadowg seadowg changed the base branch from master to v2023.3.x November 9, 2023 15:34
@seadowg seadowg added this to the v2023.3.x milestone Nov 9, 2023
@seadowg seadowg changed the title Extract Main Menu to fragment to prevent lifecycle crashes Extract Main Menu to Fragment to prevent lifecycle crashes Nov 9, 2023
@seadowg seadowg requested a review from grzesiek2010 November 9, 2023 15:50
@seadowg seadowg marked this pull request as ready for review November 9, 2023 15:50
@seadowg seadowg requested a review from grzesiek2010 November 28, 2023 09:55
@seadowg seadowg added the high priority Should be looked at before other PRs/issues label Nov 28, 2023
@dbemke
Copy link

dbemke commented Nov 29, 2023

In the CircleCI apk if Google maps aren't available should they by default switch to OSM - is the PR introducing it in the apk?

@grzesiek2010
Copy link
Member

In the CircleCI apk if Google maps aren't available should they by default switch to OSM - is the PR introducing it in the apk?

This branch has been started from v2023.3.x because it will be a point release for that version but Fallback to OSM if other engines aren't available has been merged into master and not to v2023.3.x

@srujner
Copy link

srujner commented Nov 29, 2023

Tested with Success!
Verified on device with Android 13

Verified cases:

  • Regression check on the most important functionalities through the app;
  • All Main Menu Items;
  • Exploratory testing through the app;

@dbemke
Copy link

dbemke commented Nov 29, 2023

Tested with Success!

Verified on device with Android 10

@grzesiek2010 grzesiek2010 merged commit 0c53f9f into getodk:v2023.3.x Nov 29, 2023
6 checks passed
@seadowg seadowg deleted the fix-menu-crash branch November 29, 2023 13:51
seadowg pushed a commit to seadowg/collect that referenced this pull request Nov 30, 2023
Extract Main Menu to Fragment to prevent lifecycle crashes
seadowg pushed a commit to seadowg/collect that referenced this pull request Dec 7, 2023
Extract Main Menu to Fragment to prevent lifecycle crashes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

4 participants