fossasia / open-event-attendee-android

Open Event Attendee Android General App https://github.com/fossasia/open-event-android/blob/apk/open-event-dev-app-playStore-debug.apk
Apache License 2.0
1.95k stars 554 forks source link

Open saved state for events #1078

Open liveHarshit opened 5 years ago

liveHarshit commented 5 years ago

Describe the feature you'd like Open saved state for events while navigating using navigation drawer.

Would you like to work on the issue?

Parent Issue: #1139

AnEnigmaticBug commented 5 years ago

@liveHarshit can you please elaborate this a bit more?

liveHarshit commented 5 years ago

While navigation to other section (using bottom navigation) from events fragment, after returning back, events are loading again. It should not happen.

AnEnigmaticBug commented 5 years ago

Can I work on this?

liveHarshit commented 5 years ago

Please, go ahead.

AnEnigmaticBug commented 5 years ago

Ok. Thank you :)

iamareebjamal commented 5 years ago

@AnEnigmaticBug Please take this up on priority

AnEnigmaticBug commented 5 years ago

Ok @iamareebjamal

AnEnigmaticBug commented 5 years ago

I need to discuss what caching strategy should I use for this. I've the following strategies in mind:

AnEnigmaticBug commented 5 years ago

I'll begin working on this ASAP once I get to know which one of these strategies is preferred by you.

liveHarshit commented 5 years ago

@AnEnigmaticBug What if we load fragment from the back stack?

AnEnigmaticBug commented 5 years ago

@liveHarshit that came to my mind too. But I've a (possibly misplaced) concern regarding that solution. If we solve this problem using back stack:

  1. We may have to 'fight' with the default way Navigation component works which will make it difficult to follow the code and increase chances of future errors.
  2. This problem is not isolated to the events screen. As referenced by #1139 , it also occurs in at least the tickets screen. To solve this problem for both events and tickets screen, we'll have to keep both EventsFragment and TicketsFragment alive/in the back stack at all times which doesn't make sense. As more and more screens need their contents to be saved, this issue will only increase.
  3. If we use a single way consistently throughout the app, it will be easier to understand and modify in future.
AnEnigmaticBug commented 5 years ago

@iamareebjamal which caching strategy do you prefer?

iamareebjamal commented 5 years ago

I don't think that there needs to be such a contrived strategy for this simple task, bottom nav view should have several fragments open simultaneously

When navigating across the nav view, the fragments should remain in memory and not be replaced, and hence, onCreateView should not be called on each navigation

This is a standard behaviour of bottom nav view naive implementation without any hacks or tricks

AnEnigmaticBug commented 5 years ago

@iamareebjamal The documentation suggests that the default behavior is to pop the back stack till the start destination and then navigate to the destination with launchSingleTop flag enabled. So, except the startDestination(EventsFragment), all other Fragments will be popped every time we select a bottom-navigation tab.

This means that while we can solve the issue for EventsFragment, whenever we select a bottom-navigation tab all other Fragments will be removed. This means that the issue will still be there for TicketsFragment and any other Fragments we may add in the future.

iamareebjamal commented 5 years ago

Not good then. Can you think of an alternative way we can retain the standard behaviour?

AnEnigmaticBug commented 5 years ago

Just to avoid any confusion, can you tell me what you mean by standard behavior?

iamareebjamal commented 5 years ago

When navigating across the nav view, the fragments should remain in memory and not be replaced, and hence, onCreateView should not be called on each navigation

AnEnigmaticBug commented 5 years ago

IMO there are 3 issues with keeping them in memory:

  1. If we keep all Fragments in memory, we'll not only be keeping their associated ViewModels, but also their associated Services in memory. This can potentially increase the memory usage of our app.

  2. At some point we need to remove the Fragments from the back-stack. Suppose that the EventsFragment is on the back-stack and we've opened TicketsFragment. The tickets load and get displayed. Now, it we press the back-button, the TicketsFragment will be removed. So now, if we immediately go back to the TicketsFragment, it will load all the data again.

  3. There always should be some data to display whenever possible. If the user exits the app and turns it on after a few minutes to see events in the same location, he should immediately be shown the relevant data. Right now, the data is loaded every time the app starts and the app requires internet access on startup. IMHO, the internet access should be made as optional for the user as possible.

iamareebjamal commented 5 years ago

All these problems are much deeper and separate issues. The current problem is just this -> all fragments in the bottom nav should not be replaced, recreated or removed on navigation. Just as they remain in a normal bottom nav view.

Login and Profile are different. But other fragments should behave un-weirdly

iamareebjamal commented 5 years ago

About first point, yes all the fragments in bottom nav will be in flat hierarchy, meaning they are expected by the user to be in memory all times, so it's no problem that they will keep using memory

For reference to what I want, please take a look at Twitter bottom navigation

AnEnigmaticBug commented 5 years ago

I see. Please correct me if I'm wrong, what I think is that you want multiple back-stacks: one back-stack for each tab.

Implementing this will be difficult. Ian Lake(one of the creators of the navigation component), himself said that he could only try to implement it in the navigation component in future. If we go that route, then we'll have to write a lot of custom code.

IMHO trying to go that route will be a significant effort. Even material design guidelines recommend that switching between tabs should reset the screens. So a normal bottom navigation view doesn't have multiple back-stacks. This behaviour is only for Android and not for iOS supposedly because of the difficulty in implementing a complete and bug-free solution in Android.

My guess is that the Twitter android app is not using the navigation component but rather are using either a different library or their own system.

iamareebjamal commented 5 years ago

They're not using any library. The behaviour I am mentioning happens by default. If you want, I can create a sample APK with this behaviour with custom multi back stack handling logic using only appcompat and nothing else

liveHarshit commented 5 years ago

They're not using any library. The behaviour I am mentioning happens by default. If you want, I can create a sample APK with this behaviour with custom multi back stack handling logic using only appcompat and nothing else

I agree with that. Hare problem is with navigation architecture component. It loads fragment from the back stack when we navigate to some fragment with navController and go back. But here the case is different. For bottom navigation, we set up the navigation menu with the navController and start destination is event fragment. So, no fragment goes into back stack if we navigate using the bottom nav menu then event fragment is set by default on back pressed.

AnEnigmaticBug commented 5 years ago

I get your point. I'm thinking of going with the StackOverflow answer given here. What do you say about this approach @iamareebjamal @liveHarshit?

liveHarshit commented 5 years ago

I get your point. I'm thinking of going with the StackOverflow answer given here. What do you say about this approach @iamareebjamal @liveHarshit?

Try it if works fine.

haroldadmin commented 5 years ago

Here's Google's sample on Advanced usage of the Nav AAC: https://github.com/googlesamples/android-architecture-components/tree/master/NavigationAdvancedSample

Maybe this can help you. They created this sample to show how to maintain multiple backstacks

GitHub
googlesamples/android-architecture-components
Samples for Android Architecture Components. . Contribute to googlesamples/android-architecture-components development by creating an account on GitHub.
AnEnigmaticBug commented 5 years ago

I was also looking at the sample. This seems the way to go. Thanks for your help @haroldadmin :)

AnEnigmaticBug commented 5 years ago

@iamareebjamal @liveHarshit. I've not been able to work on this issue because my mid-semester exams are going on right now. They'll be over in a few days. I'll begin working on this issue the day I get free.

liveHarshit commented 5 years ago

@iamareebjamal @liveHarshit. I've not been able to work on this issue because my mid-semester exams are going on right now. They'll be over in a few days. I'll begin working on this issue the day I get free.

Take your time. :)

AnEnigmaticBug commented 5 years ago

My exams are now over. I've resumed working on this issue.

VinamraGuptaa commented 3 years ago

Hello. Is this issue still open?