rollback separating intialization on construction

liquidninja24 commented 7 years ago

By separating initialize from construction we run into the situation where mSelectedTabIndex is set to 0, but none of the internals have been set up. This causes a NPE when calling getCurrentStack() and other functions that use mSelectedTabIndex. I think it makes more sense to default to TAB1 and initialize things on construction, but allow for delayed initialization by setting the defaultSelectedIndex to NO_TAB.

You also run into a weird situation where the controller is restored from savedInstanceState, but then you call initialize which overrides the restored state.

liquidninja24 commented 7 years ago

@ncapdevi: any thoughts on this?

ncapdevi commented 7 years ago

So sorry for the delay. Was traveling a touch. Looking into this now.

ncapdevi commented 7 years ago

Yup. This looks right. Also, so many tests! Thanks, those were desperately needed.

ncapdevi commented 7 years ago

I'm still not entirely sure on this. By doing it this way, without the public initialize() function, then there's initial NPE when you have something like this

    @Override
    public void onTabTransaction(Fragment fragment, int index) {
        // If we have a backstack, show the back button
        if (getSupportActionBar() != null) {
            getSupportActionBar().setDisplayHomeAsUpEnabled(!mNavController.isRootFragment());
        }
    }

Which is a fairly common use-case. This is because it gets called during initialization but the initial construction hasn't been finished yet.

liquidninja24 commented 7 years ago

There's two points here. 1) the onTabTransaction should never be called when NO_TAB is selected. 2) Why not have isRootFragment() return true if NO_TAB is selected? I know it's not 100% correct, but more likely than not if no tab is selected the user is not going to be able to interact with any controls anyway.

On Thu, Mar 9, 2017 at 12:10 AM, Nic Capdevila notifications@github.com wrote:

I'm still not entirely sure on this. By doing it this way, without the public initialize() function, then there's initial NPE when you have something like this
@Override
public void onTabTransaction(Fragment fragment, int index) {
    // If we have a backstack, show the back button
    if (getSupportActionBar() != null) {
        getSupportActionBar().setDisplayHomeAsUpEnabled(!mNavController.isRootFragment());
    }
}
Which is a fairly common use-case. This is because it gets called during initialization but the initial construction hasn't been finished yet.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ncapdevi/FragNav/pull/50#issuecomment-285256290, or mute the thread https://github.com/notifications/unsubscribe-auth/ABlKPALFH-IJc0xjf74LE9NyzG8nosqQks5rj4nOgaJpZM4MRdIP .

ncapdevi commented 7 years ago

1) That is definitely true, but there is a property within the FragNavBuilder to set the selected (i.e., starting) tab index, which will cause a tab transaction. I guess we could take this out of the builder (and therefore leave it at NO_TAB during creation) and not call any of the callbacks. This would require a function call afterwards to set the initial tab index. 2) The problem will still come up regardless of this because the issue isn't what isRootFragment, but rather that mNavController is null during the .build() process, which may or may not call onTabTransaction.

I'm thinking that pulling out the selectedTabIndex portion of the builder, and only allowing NO_TAB during building/construction of the controller is the best solution here (only negative is the additional function call after construction to set whatever index you want to be on, but I don't think that's necessarily a bad thing). Thoughts?

Thanks for the feedback.

liquidninja24 commented 7 years ago

Ah, I see what you mean now when you say, "mNavController is null doing the .build() process". If the initial index is set then it causes a fragment transaction, but the constructor hasn't returned at that point so mNavController is null. If I remember correctly one wasn't able to set the onTabTransactionListener until after the constructor was called, so it was never invoked in the initialize function.

The other thing to think about is when restoring from savedInstanceState. After building the navController using savedInstanceState you then have to check if it's null or not so as not to override the restored state with the "default" tab index. This is what I do in my code, but I just wanted to point it out.

With that said- yea, maybe it's best to initialize with NO_TAB, and then have the developer specifically set the the starting index (if not restoring from instance state).

ncapdevi commented 7 years ago

I keep running around in circles with this problem. I am definitely leaning towards separating the construction and the initialization again. The solution that I'm thinking of with regards to your original concern about the NPE is throw an error when any function is called before the initialization. A message that specifically indicates that you're trying to getCurrentStack() without having initialized. I'm not sure if that would cause you issues. If it causes you issues, could you lay out the exact situation that is causing the issue? I've added to the test file (currently failing) to show the issue with mFragNavController being null and calling the onTabTransaction

liquidninja24 commented 7 years ago

Sorry I haven't gotten back to you. I'm out of the country on vacation. I'm back next week and will try to think through this some more. Honestly, it might be worth jumping on a hangout and talking through the issue rather than continuously going back and forth on github.

On Sun, Mar 12, 2017, 4:28 AM Nic Capdevila notifications@github.com wrote:

I keep running around in circles with this problem. I am definitely leaning towards separating the construction and the initialization again. The solution that I'm thinking of with regards to your original concern about the NPE is throw an error when any function is called before the initialization. A message that specifically indicates that you're trying to getCurrentStack() without having initialized. I'm not sure if that would cause you issues. If it causes you issues, could you lay out the exact situation that is causing the issue? I've added to the test file (currently failing) to show the issue with mFragNavController being null and calling the onTabTransaction

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ncapdevi/FragNav/pull/50#issuecomment-285901631, or mute the thread https://github.com/notifications/unsubscribe-auth/ABlKPKKUoaQMNsvkVveHZ9wND9kW8TDLks5rkxIDgaJpZM4MRdIP .

ncapdevi commented 7 years ago

Not a problem at all. Yeah, that sounds great and would definitely be helpful. I want to hammer this stuff out and finalize 2.0 sooner rather than later, but something about it still seems a bit unsettling. Enjoy the time off.

ncapdevi commented 7 years ago

@liquidninja24 I'm looking at things again, specifically your first message, and my current thinking is thus:

Pull the initialize(selectedIndex) function from the builder, and make it a required public function
Leave the mSelectedIndex on a NO_TAB (this state my be useful for some weird use case that I'm not using (and honestly can't specifically think of, but let's imagine the navController is detached or something for a moment))
getCurrentStack() can return nullable. I'll annotate the function, it requires you check it to be null before using it (not my favorite, but it's Java so we're all used to it) (other option here would be to have it throw a runtime exception here, but having to use a try/catch is way less performant than a simple null check)
handle the "re-initialization" when pulling from a restored state. I'm not 1000% sure what you mean by this, but I'm assuming I'll run into it while I'm coding it up

I really think this is the best path moving forward and creates the least chances for errors. We could easily take it steps further and throw exceptions for certain methods like getCurrentFrag() if we haven't call initialize() yet or we could simply return null. I may hold off on implementing this to make sure it all sounds reasonable to you but I'm hoping to knock it out quickly this week and push 2.0.0 out. Thanks so much for all of your insight and help.

ncapdevi commented 7 years ago

Sometimes I'm impatient, check out https://github.com/ncapdevi/FragNav/tree/public_initialize nad see if that works for your use-case and makes sense to you.

liquidninja24 commented 7 years ago

I haven't had a chance to test, but my concern around restoring from savedInstanceState is as follows:

 mFragNavController = FragNavController.newBuilder(savedInstanceState, mFragmentManager, 1)
                .rootFragments(rootFragments)
                .build();

mFragNavController.initialize(TAB1);

If you're restoring from savedInstanceState then it should restore the tab that you were on. However, if you're forced to call initialize(tabIndex) after creating the controller then you'd be overwriting the restored tab index.

By setting the initial tab index in the builder it allowed us to conditionally use the passed in value, or the one stored in savedInstanceState if provided.

ncapdevi commented 7 years ago

Ah, yes, yes, good point, your brain is good. Yes, it seems the full way around this would be public void initialize(@Nullable Bundle savedInstanceState, @TabIndex int index)
and also public void initialize(@Nullable Bundle savedInstanceState, @TabIndex int index, List<Fragment> rootFragments)

of course could easily keep this as a single method and pass in null, but this is probably better.

That being said, it's not my favorite thing in the world to have to do, but I think it accomplishes all of the goals. Thoughts?

liquidninja24 commented 7 years ago

This is starting to look like the old constructors you had before switching to the builder model.

I'm still in favor of keeping the initialize function internal, and doing all of the initialization through the builder.

With regards to the NPE you're getting in the initial onTabTransaction, I think that we should just not call that function in the initialize function. If you look at v1.4 of the code you'll notice that onTabTransaction never gets called despite the code existing in initialize. This is because one was not able to set the onTabTransactionListener via the constructor, but instead after the object had been initialized. If you'd like to preserve this functionality then I think it's totally fine to pass in a boolean to the builder setOnTabTransactionListener specifying whether it should be called initially or not. The BottomBar library does something like this: https://github.com/roughike/BottomBar/blob/master/bottom-bar/src/main/java/com/roughike/bottombar/BottomBar.java#L396

What do you think?

On Tue, Mar 28, 2017 at 12:43 AM, Nic Capdevila notifications@github.com wrote:

Ah, yes, yes, good point, your brain is good. Yes, it seems the full way around this would be public void initialize(@Nullable Bundle savedInstanceState, @TabIndex int index) and also public void initialize(@Nullable Bundle savedInstanceState, @TabIndex int index, List rootFragments)

of course could easily keep this as a single method and pass in null, but this is probably better.

That being said, it's not my favorite thing in the world to have to do, but I think it accomplishes all of the goals. Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ncapdevi/FragNav/pull/50#issuecomment-289661331, or mute the thread https://github.com/notifications/unsubscribe-auth/ABlKPJVgSivxH1jI8j2D1-nkQVouA9mnks5rqI_VgaJpZM4MRdIP .

ncapdevi commented 7 years ago

You're not wrong.

My concern in wanting the object to exist in that a lot of repeated functionality can/should be done via ontabtransaction/onfragmenttransaction. Let's use a super simple example and say that all of your fragments implement an interface like this

public interface TitleInterface{
      public String getTitle();
}

now, in your 'ontabtransaction` you're checking to see if the fragment implemented that interface, and if it did, you're calling that method and setting the title Activity's toolbar via that. If that is your setup, it won't be called on the initial creation(because either the object is null, or we've decided not to call it) so you never get alerted of that first transaction.

So that's the general use case I'm trying to design around. Obvious solution would be to say that the activity should know which tab it's setting first, and set the title during creation, and then let the ontab/fragmenttransaction take care of it from there. I don't know, maybe I'm overly worrying about it, but at this point I just want it out there so that all of the other positives (transactionOptions) can be used.

I'm not against keeping the initialize private, that's just my current concern. Am I being overly worried? Any other ideas? Not a big enough concern. Would love to hear your thoughts.

ncapdevi commented 7 years ago

just realized that another solution would be to pass the FragNavController itself via those two functions. Essentially something like this.


    public interface TransactionListener {
        void onTabTransaction(FragNavController fragNavController, Fragment fragment, int index);
        void onFragmentTransaction(FragNavController fragNavController, Fragment fragment, TransactionType transactionType);
    }

I realize it's not the most efficient thing memory wise, but it's negligible and not in a loop, so it should be a non-issue. That being said, I don't actually like this. It's weird to have a member variable that you're using (required since you'll be doing pop/push/etc.) and then have essentially which is 99.999%(every time except before the object has actually returned) the same variable coming through the function Thoughts?

ncapdevi commented 7 years ago

I left everything I wrote so you could see how dumb I was being. I've thought it over. I'm dumb. The fragment is coming through in both of those interface functions, so you have that. The only thing that you CAN'T do, is to get the fragment stack. The reason that you might want that, is if you want to know weather you are deep in the stack and want to put a back button in the toolbar. Now, if it's the first call and you just have a null-check, this will get passed over and that's fine, because you're always at the bottom of the stack. So I agree, I think that the initialization can stay private within the function. Thanks for back and forthing with me on this. I'm going to finalize it and hopefully push out the version this week. If there's an issue we haven't thought of, it will be a minimal change and revolve around this issue. (maybe a fix as simple as another interface that provides the whole stack on transactions)

liquidninja24 commented 7 years ago

Hi buddy,

I'm sorry I never got a chance to reply to your comments! I've been traveling for work and hadn't had a chance to sit down and reply.

Your concerns are totally valid wrt the mNavController being null in the onFragmentTransaction handler. Your proposed solutions should be fine for now while things are fleshed out some more.

Congratulations on publishing 2.0, and I look forward to using it and contributing in the future!

Cheers, Nick

On Thu, Mar 30, 2017 at 1:56 AM, Nic Capdevila notifications@github.com wrote:

I left everything I wrote so you could see how dumb I was being. I've thought it over. I'm dumb. The fragment is coming through in both of those interface functions, so you have that. The only thing that you CAN'T do, is to get the fragment stack. The reason that you might want that, is if you want to know weather you are deep in the stack and want to put a back button in the toolbar. Now, if it's the first call and you just have a null-check, this will get passed over and that's fine, because you're always at the bottom of the stack. So I agree, I think that the initialization can stay private within the function. Thanks for back and forthing with me on this. I'm going to finalize it and hopefully push out the version this week. If there's an issue we haven't thought of, it will be a minimal change and revolve around this issue. (maybe a fix as simple as another interface that provides the whole stack on transactions)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ncapdevi/FragNav/pull/50#issuecomment-290310028, or mute the thread https://github.com/notifications/unsubscribe-auth/ABlKPGeiCewMM_hDiriJL77Wc10QeSnCks5rq0QpgaJpZM4MRdIP .

ncapdevi / FragNav

rollback separating intialization on construction #50