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

Open saved state for events #1078

Open
liveHarshit opened this issue Feb 11, 2019 · 31 comments
Open

Open saved state for events #1078

liveHarshit opened this issue Feb 11, 2019 · 31 comments

Comments

@liveHarshit
Copy link
Member

liveHarshit commented Feb 11, 2019

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

Would you like to work on the issue?

  • Other: Open for all

Parent Issue: #1139

@AnEnigmaticBug
Copy link
Contributor

@liveHarshit can you please elaborate this a bit more?

@liveHarshit
Copy link
Member Author

liveHarshit commented Feb 11, 2019

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

@AnEnigmaticBug
Copy link
Contributor

Can I work on this?

@liveHarshit
Copy link
Member Author

Please, go ahead.

@AnEnigmaticBug
Copy link
Contributor

Ok. Thank you :)

@iamareebjamal
Copy link
Member

@AnEnigmaticBug Please take this up on priority

@AnEnigmaticBug
Copy link
Contributor

Ok @iamareebjamal

@AnEnigmaticBug
Copy link
Contributor

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

  • Always use the database. Get new data only if either:

    1. No data exists.
    2. User has pulled to refresh.
  • Always use the database but send a request in background every time it is accessed.

  • Keep a singleton which holds whether the events data has been retrieved in this session or not. If it has been retrieved, use the database. Otherwise, use the database and send a request in background.

@AnEnigmaticBug
Copy link
Contributor

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

@liveHarshit
Copy link
Member Author

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

@AnEnigmaticBug
Copy link
Contributor

@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 Bottom Navigation issues #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
Copy link
Contributor

@iamareebjamal which caching strategy do you prefer?

@iamareebjamal
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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

@AnEnigmaticBug
Copy link
Contributor

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

@iamareebjamal
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

haroldadmin commented Mar 10, 2019

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
Samples for Android Architecture Components. . Contribute to googlesamples/android-architecture-components development by creating an account on GitHub.

@AnEnigmaticBug
Copy link
Contributor

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

@AnEnigmaticBug
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Contributor

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

@VinamraGuptaa
Copy link

Hello. Is this issue still open?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants