feat(Android): Refactoring configuration #3778

carlpoole · 2020-11-08T22:13:48Z

This branch seeks to improve the config use in Android similar to #3759.

Config can now be initialized from capacitor.config.json and programmatically via ConfigBuilder for embedded use case
The existing config getters like getInt and getString are deprecated.
- For plugins, use getPluginInt and getPluginString.
- For core config values, use the new direct accessor like getServerUrl and getBackgroundColor
Added tests for the Builder and JSON Reading use cases.

… for embedded

… android-config

Conflicts: android/capacitor/src/main/java/com/getcapacitor/Bridge.java android/capacitor/src/main/java/com/getcapacitor/BridgeActivity.java android/capacitor/src/main/java/com/getcapacitor/WebViewLocalServer.java

android/capacitor/src/main/java/com/getcapacitor/CapConfig.java

imhoffd · 2020-12-03T22:32:41Z

android/capacitor/src/main/java/com/getcapacitor/CapConfig.java

+        loadConfig(assetManager);
+        deserializeConfig();


Should all this be happening in the constructor? Maybe we can take this opportunity to practice some separation of concerns. The iOS side now has a few different classes that separate "loading from a JSON file" and "containing the configuration".

Thanks for the feedback. I have removed the public constructor for CapConfig and replaced it with a method fromFile that can be used to create an instance of CapConfig from the JSON config file.

I moved the JSON file loading to the FileUtils class since it involved reading a file and seemed appropriate there (and its also reused elsewhere in the core).

android/capacitor/src/main/java/com/getcapacitor/CapConfig.java

android/capacitor/src/main/java/com/getcapacitor/BridgeActivity.java

jcesarmobile

CapConfig was created to avoid a breaking change as we had the Config class and users were using it on plugins. Used CapConfig as the name to also match iOS name.
Since this PR changes the constructor, that's also a breaking change.
Not sure if instead of changing the constructor, we rename the class to InstanceConfiguration as Ian did on the iOS equivalent #3759

carlpoole · 2020-12-09T14:21:48Z

CapConfig was created to avoid a breaking change as we had the Config class and users were using it on plugins. Used CapConfig as the name to also match iOS name.
Since this PR changes the constructor, that's also a breaking change.
Not sure if instead of changing the constructor, we rename the class to InstanceConfiguration as Ian did on the iOS equivalent #3759

It is a breaking change, but is it expected that 3rd parties would be creating new instances of CapConfig? I updated the code in the core to use the new constructor. If it's not an expected use-case or an embedded/edge case, it might be an acceptable breaking change.

I can see the appeal in naming things similarly to iOS, but I don't think it should be the rule. Java has instances of classes so the name "InstanceConfiguration" feels unnecesarily verbose, but we are likely introducing a new class here to contain the plugin config, so we do need some name work.

jcesarmobile

I think both platforms should be as close as possible, specially when two classes do the exact same thing. If not, when somebody not familiar with one of the two platforms needs to do some change in the other platform will have a hard time finding things.
But if you don't agree I'm ok with it.

imhoffd · 2020-12-10T23:59:05Z

android/capacitor/src/main/java/com/getcapacitor/BridgeActivity.java

@@ -39,7 +39,7 @@ protected void init(Bundle savedInstanceState, List<Class<? extends Plugin>> plu
     * use {@link BridgeActivity#bridgeBuilder}.
     */
    @Deprecated
-    protected void init(Bundle savedInstanceState, List<Class<? extends Plugin>> plugins, JSONObject config) {
+    protected void init(Bundle savedInstanceState, List<Class<? extends Plugin>> plugins, CapConfig config) {


Since this is already a breaking change, we should just remove it. init() is the deprecated way of initializing the Android bridge.

android/capacitor/src/main/java/com/getcapacitor/BridgeFragment.java

android/capacitor/src/main/java/com/getcapacitor/FileUtils.java

android/capacitor/src/main/java/com/getcapacitor/CapConfig.java

android/capacitor/src/main/java/com/getcapacitor/JSONUtils.java

android/capacitor/src/main/java/com/getcapacitor/PluginsConfig.java

…edback

android/capacitor/src/main/java/com/getcapacitor/Plugin.java

carlpoole added 4 commits November 8, 2020 15:59

Deserialize config into object and use object throughout Capacitor

325a225

Use new Config object to initialize for embedded

e2756f5

Config is immutable and can be created programatically with a builder…

b5a055d

… for embedded

Merge branch 'main' into android-config

9cc167b

imhoffd mentioned this pull request Nov 9, 2020

feat(cli): locate plugins by allowlist #3762

Merged

2 tasks

imhoffd added this to the 3.0.0 milestone Nov 19, 2020

carlpoole added the platform: android label Nov 20, 2020

carlpoole and others added 12 commits November 25, 2020 09:21

Merge branch 'main' into android-config

f131dd0

Plugin config separation, refactor, comments + more

2c3548d

deprecated direct config object accessors and added plugin versions

40a7833

Config building tests

76c5bb8

Merge branch 'main' into android-config

7e9b04d

Merge branch 'main' into android-config

f81cda9

Updated tests and minor refactor

fc0f47a

Merge branch 'android-config' of github.com:ionic-team/capacitor into…

5040828

… android-config

Merge branch 'main' into android-config

fca908b

Changed config to use context and get the asset manager itself

15d11f7

Added null check

c6a59f6

Allow fragment users to set config

982b126

carlpoole marked this pull request as ready for review December 1, 2020 18:44

carlpoole requested review from jcesarmobile and imhoffd December 1, 2020 18:47

carlpoole and others added 2 commits December 1, 2020 16:44

Fixed tests

0b1ea33

Merge branch 'main' into android-config

43f127e

Conflicts: android/capacitor/src/main/java/com/getcapacitor/Bridge.java android/capacitor/src/main/java/com/getcapacitor/BridgeActivity.java android/capacitor/src/main/java/com/getcapacitor/WebViewLocalServer.java

imhoffd reviewed Dec 3, 2020

View reviewed changes

ionic-team deleted a comment from imhoffd Dec 8, 2020

carlpoole added 3 commits December 8, 2020 10:47

remove unrelated change

6a697a2

remove unused

55f4299

test import cleanup

1f44a73

jcesarmobile reviewed Dec 8, 2020

View reviewed changes

carlpoole added 3 commits December 8, 2020 17:05

Moved default debug value, renamed some methods

5bb57af

Exception exclusivly json

8457e83

pluginsconfiguration

e589b4a

jcesarmobile approved these changes Dec 10, 2020

View reviewed changes

carlpoole added 6 commits December 10, 2020 11:05

Consolidated plaintext file loading to FileUtils and refactored config

fdc4db0

fmt issue

dd97b37

Split out plugins config into its own object and JSON utils

6b7048a

are -> is for futureproof

21dd356

Javadoc to bring attention to the CapConfig change in BridgeActivity

b119833

Merge branch 'main' into android-config

93804bd

imhoffd reviewed Dec 11, 2020

View reviewed changes

carlpoole added 3 commits December 14, 2020 15:19

Changed PluginConfig to be plugin specific, refactored based on PR fe…

b121eea

…edback

Merge branch 'main' into android-config

7def9c9

missed tag, and comment

ac1e5f7

imhoffd reviewed Dec 14, 2020

View reviewed changes

android/capacitor/src/main/java/com/getcapacitor/Plugin.java Show resolved Hide resolved

android/capacitor/src/main/java/com/getcapacitor/Plugin.java Outdated Show resolved Hide resolved

carlpoole added 2 commits December 14, 2020 17:58

Getconfig returns empty config if does not exist, improves usage

de85778

Moved null check into CapConfig

5932137

imhoffd approved these changes Dec 15, 2020

View reviewed changes

carlpoole merged commit 9820a30 into main Dec 15, 2020

carlpoole deleted the android-config branch December 15, 2020 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Android): Refactoring configuration #3778

feat(Android): Refactoring configuration #3778

carlpoole commented Nov 8, 2020 •

edited

Loading

imhoffd Dec 3, 2020

carlpoole Dec 10, 2020

jcesarmobile left a comment

carlpoole commented Dec 9, 2020

jcesarmobile left a comment

imhoffd Dec 10, 2020

feat(Android): Refactoring configuration #3778

feat(Android): Refactoring configuration #3778

Conversation

carlpoole commented Nov 8, 2020 • edited Loading

imhoffd Dec 3, 2020

Choose a reason for hiding this comment

carlpoole Dec 10, 2020

Choose a reason for hiding this comment

jcesarmobile left a comment

Choose a reason for hiding this comment

carlpoole commented Dec 9, 2020

jcesarmobile left a comment

Choose a reason for hiding this comment

imhoffd Dec 10, 2020

Choose a reason for hiding this comment

carlpoole commented Nov 8, 2020 •

edited

Loading