Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

refactor: improve logs related to deckPicker in StudyOptionsFragment

Open criticalAY opened this issue 1 year ago • 1 comments

Purpose / Description

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

Fixes

  • Related https://github.com/ankidroid/Anki-Android/issues/17284

How Has This Been Tested?

Local build

Checklist

Please, go through these checks before submitting the PR.

  • [x] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [x] You have commented your code, particularly in hard-to-understand areas
  • [x] 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

criticalAY avatar Oct 20 '24 18:10 criticalAY

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

mikehardy avatar Oct 20 '24 21:10 mikehardy

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.

criticalAY avatar Oct 21 '24 06:10 criticalAY

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 ?

mikehardy avatar Oct 21 '24 20:10 mikehardy

@mikehardy awaiting your review

david-allison avatar Oct 23 '24 16:10 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

  • https://github.com/ankidroid/Anki-Android/issues/17077
  • https://github.com/ankidroid/Anki-Android/pull/17099

david-allison avatar Oct 23 '24 22:10 david-allison