Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch to using Fragments and ViewModel #72

Closed

Conversation

inthewaves
Copy link
Member

@inthewaves inthewaves commented Aug 28, 2020

Closes #69, closes #70, closes #71

This PR makes the app use Fragments hosted by an Activity instead of an app with just one Activity. Functionally, it should be the same as before (besides how the document properties will now load in if the dialog is opened before the properties is loaded). Fragments can give us more flexibility in the future on how we can display the WebView, how we could manage some bottom menu via a BottomNavigationView or similar, etc.

The PR also moves state information about the viewer into a ViewModel, which dialogs can use to get information they need. This removes the use of Loaders, which are deprecated. (The document properties parsing was rewritten in Kotlin for coroutines.)

See commit messages for more details. If using Fragments is too much, some parts of this PR such as the ViewModel architecture can be integrated back into the original way the app was built.

Tested on a GrapheneOS Android 10 device and an Android 7 device (tested to see that the app performs like what it was before)

@thestinger
Copy link
Member

@inthewaves This needs to have conflicts resolved.

I'm not that experienced with Android app UI development and I'm not up-to-date on the APIs and tools that are used for modern apps. That makes it difficult for me to review this.

I have my hands full with other things right now so I also can't commit to promptly reviewing this, particularly since it's primarily just an architectural and tooling change. For example, the migration of Vanadium to Chromium 85 has already been delayed for several days with no progress on it and no one helping with it.

@inthewaves inthewaves force-pushed the migrate-to-fragments-pr branch 2 times, most recently from d77e98a to 3beda8d Compare August 29, 2020 05:14
- Host the WebView and all of its related code inside of a Fragment
instead of an Activity. Fragments can give us more flexibility in the
future on how we can display the WebView, how we could manage some
bottom menu via a BottomNavigationView or similar, etc.

- Use a ViewModel and LiveData architecture in order to move away from
Loaders, which are now deprecated. Almost all state information for the
viewer (like current page, zoom ratio) is now stored in the ViewModel,
which survives configuration changes.
  - Rewrite the document property parsing in DocumentPropertiesLoader in
    Kotlin, taking advantage of Kotlin coroutines to do asynchronous
    parsing of the document properties.

- Dynamically update the properties dialog after PDF loads.
If viewing the PDF properties while the PDF loads, before, the dialog
would show an error message. Now, the error message will be swapped out
with the parsed info as soon the document properties have been parsed.
This is done using an Observer on LiveData.

- Use an alpha version of AndroidX Fragment so that we can use a simpler
way to pass data between two Fragments:
https://developer.android.com/training/basics/fragments/pass-data-between

- Alpha version of Fragments also has ActivityResultLauncher, which
simplifies the SAF launch
https://developer.android.com/training/basics/intents/result
"While the underlying startActivityForResult() and onActivityResult()
APIs are available on the Activity class on all API levels, it is
strongly recommended to use the Activity Result APIs introduced in
AndroidX Activity 1.2.0-alpha02 and Fragment 1.3.0-alpha02."

Closes GrapheneOS#69, closes GrapheneOS#70
- Save state using the ViewModel Saved State module, since the ViewModel
alone doesn't survive system-initiated process death. The ViewModel is
good for configuration changes.
https://developer.android.com/topic/libraries/architecture/viewmodel-savedstate

- Save the document properties state to prevent reparsing and to show
the properties if the Activity gets killed while the properties dialog
is open.

- Don't load null URIs from open document action.
Now, we just reconstruct the properties if the app goes through some
system-initiated process death. For configuration changes, the app
still retains the properties in the ViewModel.

Also,
- Move the WebView into a container so that it can be cleared without
giving errors.
- Refactor document parsing even more (+ move into separate file) and
  conform to Kotlin style guide
At times, the WebView would execute methods via the Channel even when
the Fragment was in a destroyed state. This crashed the app, since the
Fragment wasn't tied to an Activity. This fixes that by explicitly
stopping the WebView when the Fragment's view is destroyed.

Fixes GrapheneOS#71
This means we won't be able to use ActivityResultLauncher and
FragmentResult owners and listeners. This can be reverted later when
it's more stable.
@inthewaves
Copy link
Member Author

Needs to be reworked

@inthewaves inthewaves closed this Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants