refactor: improve logs related to deckPicker in StudyOptionsFragment

[criticalAY]

Purpose / Description

Improvise the logs in StudyOptionsFragment to find the exact cause of NPE in study option fragment

Fixes

• Related [BUG]: Null DeckPicker reference in StudyOptionsFragment on tablets #17284

How Has This Been Tested?

Local build

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

One blocker on the comment, rest are preferences

[mikehardy]

this will convert a crash that is the reason we saw this problem into a bunch of log warnings that we will never see

[mikehardy]

I think we should send an error report at least

I think it should still be non-null, but, the getter is just returning activity, perhaps it should return requireActivity then it will still crash, but it will have an obvious error

All of the activity results should have an isAdded check from Fragment, no need to do the work if the fragment has been detached and disposed?

and configureToolbar shouldn't be run in onCreateView, it should probably run onAttach ?

In those cases I don't think it's ever possible for Activity to be null (onAttach it will exist, and if !Added and you avoid, then you will avoid when it is not null)

And if we somehow still attempt to fetch the deckPicker / activity then it will still crash so we'll see it

[criticalAY]

config toolbar should not be in onAttach since,

• onAttach(): This is too early in the fragment lifecycle to configure UI elements like the toolbar since the view hasn't been created yet.
• onCreateView(): The view hierarchy is created here, but it's still not fully initialized. This method is mainly intended for inflating the layout.
• onViewCreated(): The view is fully initialized at this point, making it the ideal place to set up the toolbar.

[mikehardy]

Based on that description of the lifecycle, is it possible then that we are in a race with construction of view / configure of toolbar, and a teardown cycle with destroy of view / detach of fragment?

If not, then I don't know how the activity could be null. Perhaps we need some logging on the teardown direction of the view/fragment lifecycle to see if we are still trying to configure a toolbar when the user has already moved on to something else and the fragment detached so there is no activity ?

[lukstbit]

I'm fine with the changes, just left two minor style issues.

To be honest, this bug would probably be fixed by using the FragmentResult apis in the configureToolbarInternal() method to ping DeckPicker and let it handle the menu update itself. It's not the job of StudyOptionsFragment to handle the menu of its parent, directly accessing DeckPicker also makes it very hard to test the fragment separately from DeckPicker because any test needs to make sure that the DeckPicker is available or try weird mocking.

I have the same complaint about pushing the handling of those menu items that access directly DeckPicker(deck rename/delete/export) into StudyOptionsFragment.

[david-allison]

@mikehardy awaiting your review

[david-allison]

this bug would probably be fixed by using the FragmentResult apis in the configureToolbarInternal() method to ping DeckPicker ...

@lukstbit just to note: the following issue/PR starts to move in this direction, and it'd be useful to transfer the additional thoughts from your comment here so they can be worked into the solution

• DeckPicker should handle menu responsibility instead of StudyOptionsFramgment (Tablet) #17077
• Menu handled by DeckPicker instead of StudyOptionsFragment #17099

[mikehardy]

looks pretty thorough! There is a small chance acracrium will be bombed with this but somehow I don't think so - hopefully just enough to learn stuff
Thanks @criticalAY