ncapdevi / FragNav

An Android library for managing multiple stacks of fragments
1.5k stars 220 forks source link

Feature request #82

Closed predasorinionut closed 7 years ago

predasorinionut commented 7 years ago

There is a method in FragNavController.class with the following signature:

public void pushFragment(@Nullable Fragment fragment, @Nullable FragNavTransactionOptions transactionOptions)

Inside that method is used a FragmentTransaction object and is called the commit() method on it.

I wish we can have another pushFragment() method that uses commitAllowingStateLoss() method call on the FragmentTransaction object instead of simple commit() method call.

The motivation for this request is that I get that annoying error in some cases:

java.lang.IllegalStateException: Can not perform this action after onSaveInstanceState

and I need, on my risk, to use commitAllowingStateLoss() on pushFragment().

Thanks.

ncapdevi commented 7 years ago

Entirely fair. So, there's 3 ways (maybe more, but after my first impression of this) that this could be achieved. My hesitation with this, is that if there's a use-case for "push" then there is a usecase for pop replace and switchTab as well. So all of these need to be considered and implemented.

A) A separate method, something like pushAllowingStateLoss() which would do exactly what it sounds like. Again, we'd want these for every single transaction type (this is the general approach Android team went with) B) Some sort of boolean that's added to the methods to indicate wether we want to commit allowing state loss. This seems reasonable I guess. C) Adding that same boolean to the FragNavTransactionOptions This is the approach I'm most leaning towards. It would mean there's no new methods, or any breaking changes to any of the current methods. We already have the TransactionOptions, so might as well add to them.

Thoughts?

predasorinionut commented 7 years ago

Firstly, thanks for your quick response. I read it and I also think that the C variant is the optimal one. Also, there was a case that crossed my mind and I think there is one more possibility, that I will explain to you in the next few lines:

Let's say that I use FragNavTransactionOptions to configure FragNavController once for the entire app. But let's assume that in my app, there are two or three cases when I want to use the same FragNavController but with different options(eg. different transition animation or different commit option or...).

So, for the above scenario, do you think it's a good idea to also have an overload for push, pop, replace and switchTab methods, that will have a FragNavTransactionOptions parameter and will use that instead of the global FragNavTransactionOptions?

sbfss commented 7 years ago

My hesitation with this, is that if there's a use-case for "push" then there is a usecase for pop replace and switchTab as well.

clearStack also

ncapdevi commented 7 years ago

@sbfss agreed. Forgot to add it to the list, but there's already a public void clearStack(@Nullable FragNavTransactionOptions transactionOptions) so we're good on that front if we add it to the options

ncapdevi commented 7 years ago

@predasorinionut I'm a bit confused, are you on the latest version of the library? I think you're describing what's already in place. For example, both of these methods already exist: public void pushFragment(@Nullable Fragment fragment) and public void pushFragment(@Nullable Fragment fragment, @Nullable FragNavTransactionOptions transactionOptions)

predasorinionut commented 7 years ago

Oh, you're right, my bad. Then, we stick to the C option, where you add a boolean to FragNavTransactionOptions? It looks fine to me. Thanks. :)

sbfss commented 7 years ago

@ncapdevi please look at pull request #84

predasorinionut commented 7 years ago

@ncapdevi can you predict when will the new feature be added? I was hoping that I could close the bug I have opened in my project pretty soon, so please, let me know if any progress is made with this request. Thanks.

ncapdevi commented 7 years ago

@predasorinionut Apologies for the delay. The bug will be fixed and an updated version of the library by the end of this weekend.

predasorinionut commented 7 years ago

@ncapdevi This is great! Thanks.

ncapdevi commented 7 years ago

84 Merged into 2.1.0