You can test the changes on this Pull Request by downloading the APK here.

Thanks @jd-alexander!

The solution works as expected. However, I like to follow the Fail-Fast principle. Changing the non-null type to nullable prevents the app from crashing, but we'll never know if the users can see their images (and that we have bugs in the app). In an ideal world, everything would be written in Kotlin and we wouldn't have this problem. We are not there yet, but I think it'd be nice to try to keep the interfaces as clean as possible. Wdyt?

ReaderPhotoView.loadImage() is invoked from two places

1. setImageUrl(..) -> which invokes mHiResImageUrl = ReaderUtils.getResizedImageUrl( which always returns non-null string -> this scenario is safe.
2. onLayout(..) -> this is apparently the scenario in which the mHiResImageUrl is null (stack trace in Sentry confirms it).

How would you feel about modifying ReaderPhotoViewerFragment so it's never opened with a null imgUrl -> we could show a toast message and log an exception into Sentry in case someone tries to open it with a null imgUrl. This solution would prevent the app from crashing. Moreover it'd help us investigate how come we are opening the fragment with a null url.
Another solution which is simpler and is ok imo is to check we don't call loadImage from onLayout unless the url is set. We'd still just hide the bug we have, but we wouldn't change the interface of ImageManager which is used all over the app and could potential hide multiple bugs. Wdyt?

Thanks @jd-alexander!

The solution works as expected. However, I like to follow the Fail-Fast principle. Changing the non-null type to nullable prevents the app from crashing, but we'll never know if the users can see their images (and that we have bugs in the app). In an ideal world, everything would be written in Kotlin and we wouldn't have this problem. We are not there yet, but I think it'd be nice to try to keep the interfaces as clean as possible. Wdyt?

Thanks for the review! Let's see what can be done.

ReaderPhotoView.loadImage() is invoked from two places

setImageUrl(..) -> which invokes mHiResImageUrl = ReaderUtils.getResizedImageUrl( which always returns non-null string -> this scenario is safe.
onLayout(..) -> this is apparently the scenario in which the mHiResImageUrl is null (stack trace in Sentry confirms it).

How would you feel about modifying ReaderPhotoViewerFragment so it's never opened with a null imgUrl -> we could show a toast message and log an exception into Sentry in case someone tries to open it with a null imgUrl. This solution would prevent the app from crashing. Moreover it'd help us investigate how come we are opening the fragment with a null url.

Another solution which is simpler and is ok imo is to check we don't call loadImage from onLayout unless the url is set. We'd still just hide the bug we have, but we wouldn't change the interface of ImageManager which is used all over the app and could potential hide multiple bugs. Wdyt?

Yes, I just did some investigation and I think this might be the appropriate solution. We could be having an issue where the onLayout is being called before setImage which is causing the null issue. Wdyt?