switch to using Fragments and ViewModel #72
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
inthewaves
force-pushed
the
migrate-to-fragments-pr
branch
3 times, most recently
from
203ce14 to
b24a605
Compare
|
@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
force-pushed
the
migrate-to-fragments-pr
branch
2 times, most recently
from
d77e98a to
3beda8d
Compare
- 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
force-pushed
the
migrate-to-fragments-pr
branch
from
3beda8d to
4de0b96
Compare
This reverts commit 6260bcb
inthewaves
force-pushed
the
migrate-to-fragments-pr
branch
from
4de0b96 to
0020f2c
Compare
|
Needs to be reworked |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)