PierreLessard
commented
1 year ago Closing since there is a strange bug with this implementation. When first running the app, after onboarding when clicking the log page and then exiting, either with the save button or the x, the app crashes. Analyzing the issue more it seems that the navigateUp call can not find a anything to navigate to which means that the backstack is somehow lost or emptyed before the navigateUp call. After testing what causes this issue it is only with recomposing the log page on first launch and then calling navigateUp. I think this is an issue with jetpack compose. Since the implementation requires recomposition of the log screen we will have to use a different method. If we get the chance in the future we can try moving the common component.
Bug Fix
Fixed the issue with the save button jumping up. Also fixed the issue where Leo could double click a date and navigate to a log page multiple times, loading the navigation stack with too many entries. Added test support for checking the save button disappears, hence added tests to check this bug does not occur.
Reasons for the issue: The issue occurs because of some odd recomposition issue with compose. What happens is that when we navigate to a new page we not only recompose the inside of the scaffold in the app screen but the app screen itself. This allows for us to use the common component on pages without having to recompose the common component. The only issue is that navigating between pages that do use the common component and those that dont (currently just log screen) there is an issue. What happens is that the common component recomposes before the navigation does, this causes the screen to resize before navigating which in turn moves the save button up. A fix to this, and something that I know was requested earlier on, is to use animated navigation. I spent a few hours trying to port our navigation to use animated navigation; however, the project seems too large to easily port over without very hard to find bugs causing the application to always crash. I still think we should consider animated navigation if we have time as it would make the app a lot prettier and I remember TPP mentioning they were interested in more navigation during the initial meeting with devs. The link is: https://google.github.io/accompanist/navigation-animation/.
Potential other fixes and implemented solution: One option was to move the common component so that it is composed within the route navigated to, instead of outside in the app screen. This would fix the issue of the screen size changing between routes. However, that is a lot of refactoring and since the log screen is the only planned page without the common component I did a simpler fix of recomposing the log page without the save button before exiting. This solution still looks good but if possible considering animation in the future would be better.