Replace BaseActivity.sharedPreferences with app preferences #4797

ezaquarii · 2019-11-04T15:57:28Z

eliminate access to shared preferences when setting dark theme.
extend AppPreferences interface to allow subscription for preference changes
improve preferences interface documentation

@AndyScherzinger @tobiasKaminsky @dan0xii

This is my initial proposal to deal with AppPreferences interface bypass.
I tried to address access to AppPreferencesImpl but it looks like we still have
many points where we do cross-cutting calls to private implementation for various
reasons (dodgy workarounds, circular dependencies, etc). Despite my tries, I conclude
that closing AppPreferencesImpl is a bit too much work for a single change and I exhausted
my bandwidth for today.

This PR will address only theme related concerns. I'll follow up with improvements around
*Impl access as my time allows.

Fixes #4791

Signed-off-by: Chris Narkiewicz hello@ezaquarii.com

src/main/res/xml/preferences.xml

codecov · 2019-11-04T21:03:28Z

Codecov Report

Merging #4797 into master will increase coverage by 0.1%.
The diff coverage is 70.83%.

@@             Coverage Diff             @@
##             master    #4797     +/-   ##
===========================================
+ Coverage     17.48%   17.58%   +0.1%     
  Complexity        3        3             
===========================================
  Files           376      377      +1     
  Lines         32385    32413     +28     
  Branches       4569     4573      +4     
===========================================
+ Hits           5661     5699     +38     
+ Misses        25813    25798     -15     
- Partials        911      916      +5

Impacted Files	Coverage Δ	Complexity Δ
...m/nextcloud/client/preferences/AppPreferences.java	`0% <0%> (ø)`	`0 <0> (?)`
.../android/ui/activity/ThemedPreferenceActivity.java	`66.66% <44.44%> (+1.66%)`	`0 <0> (ø)`	⬇️
...owncloud/android/ui/activity/SettingsActivity.java	`38.7% <50%> (+0.04%)`	`0 <0> (ø)`	⬇️
...com/owncloud/android/ui/activity/BaseActivity.java	`35.55% <57.14%> (ø)`	`0 <0> (ø)`	⬇️
...xtcloud/client/preferences/AppPreferencesImpl.java	`58.6% <92%> (+4.89%)`	`0 <0> (ø)`	⬇️
...ncloud/android/ui/fragment/OCFileListFragment.java	`25.57% <0%> (-0.41%)`	`0% <0%> (ø)`
...cloud/android/ui/activity/FileDisplayActivity.java	`19.93% <0%> (-0.25%)`	`0% <0%> (ø)`
...in/java/com/owncloud/android/utils/ThemeUtils.java	`51.57% <0%> (+1.96%)`	`0% <0%> (ø)`	⬇️
...m/owncloud/android/ui/activity/DrawerActivity.java	`45.19% <0%> (+2.29%)`	`0% <0%> (ø)`	⬇️

AndyScherzinger · 2019-11-04T21:04:03Z

In general it looks good while there are now two keys for the theme which should imho be just one.

nextcloud-android-bot · 2019-11-04T21:19:30Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11535

ezaquarii · 2019-11-04T23:23:56Z

In general it looks good while there are now two keys for the theme which should imho be just one.

Yeah, I just checked the settings XML and it looks like Android is putting the UI key into the store,
which is not good. I'll roll this back as I have no good solution for this atm.

The rationale behind this experiment is that conceptually we have 1 key somewhere behind AppPreferences and nobody should touch it in any way other than calling strongly typed API. This is on purpose:

people should not liberally write into the SharedPreferences bypassing the interface we provide, with all sort of potential sanitation and safeguards; we'd like to discoruage people from fiddling with the store on their own, so we can safely evolve this area in the future.
UI should not be concerned how or where we store app settings - we might find that JSON stored on Nextcloud server is the way to go at some time and I'd argue that this should be the goal eventually - I have some vested interest here, as I'm kinda tired of reconfiguring my autoupload each time I reinstall the app (;
tracking preferences access via interface is as easy as tracking a method call; there is is one entry gate and no surprises

Currently keys must be maintained in sync in 3 distinct places:

AppPreferencesImpl - to access the store
settings.xml - so the UI has backdoor into the store layer
SettingsActivity - so we can access UI controls

BTW, those sets are not 100% overlapping.

Now, you could argue that we can just move those keys to AppPreferences interface to unify 1. and 2., but my fear is that we again start seeing stuff like this:

void outOfTheBlue() {
    SharedPreferences p = MainApp.getInstance().getSharedPreferences(); // circular deps again
    p.edit().setString(AppPrefernces.PREFS__DODGY_KEY, "magic value").apply(); // out of band calls

    MainApp.getInstance().getSharedPreferences().setBoolean(AppPrefernces.PREFS__WORKAROUND_FLAG, !MainApp.getInstance().getSharedPreferences().setBoolean(AppPrefernces.PREFS__WORKAROUND_FLAG, false)).commit();
}

and we just got rid of it recently. Those will slip through code reviews even if we stay alert.

Enough ranting. 🙃 Having said that, I'm reverting my experiment as I have no good and quick workaround for antipattern of the native Android API and the solution we have right now seems work well enough. Not great - not terrible. ¯\_(ツ)_/¯

- Eliminate access to shared preferences when setting dark theme. - Extend AppPreferences interface to allow subscription for preference changes - Improve preferences interface documentation Fixes #4791 Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>

nextcloud-android-bot · 2019-11-05T06:07:15Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11544.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

nextcloud-android-bot · 2019-11-05T06:08:43Z

Codacy

304

Lint

Type	master	PR
Warnings	59	59
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	69
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	119
Security Warnings	44
Dodgy code Warnings	138
Total	423

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	69
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	119
Security Warnings	44
Dodgy code Warnings	138
Total	423

AndyScherzinger · 2019-11-05T20:56:15Z

src/main/java/com/nextcloud/client/preferences/AppPreferencesImpl.java

    private static final String PREF__FOLDER_SORT_ORDER = "folder_sort_order";
    private static final String PREF__FOLDER_LAYOUT = "folder_layout";
-    public static final String PREF__THEME = "darkTheme";
+    static final String PREF__DARK_THEME_ENABLED = "dark_theme_enabled";


Will likely chance when @tobiasKaminsky pushes his PR to the repo adding a 3rd option for the theme values (dark, light, system)

This will be interesting. I wild guess that the change will be based on the SharedPreferences code that I just removed and the preference will become some kind of enum instead of boolean?

I'm fine with resolving conflicts on my side, let's just make sure we don't have to do a complete rewrite. (-:

Is there any public branch I could potentially try?

Is there any public branch I could potentially try?

Don't know, I just saw this earlier this evening, which is why I mentioned @tobiasKaminsky here --> #4757 (comment)

AndyScherzinger · 2019-11-05T21:00:11Z

Fine with me. Just one minor comment that this PR and a soon to be openend PR by @tobiasKaminsky will have conflicts while both PRs will change the type of the preference and thus (fine since master is in alpha state for v3.10) both PRs will break the activated dark theme (since the key changed twice then)

tobiasKaminsky · 2019-11-06T12:50:37Z

We merge this one, and then I change it within my upcoming PR.
Sometimes it will just happen that we create conflicts…

890789f Merge pull request #4797 from nextcloud/ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity 180bdee Merge pull request #4801 from Infomaniak/feature/resolve-set-as-wallpaper-issue 039adc0 [tx-robot] updated from transifex b59938d daily dev 20191106

ezaquarii requested review from AndyScherzinger, dan0xii and tobiasKaminsky November 4, 2019 15:57

ezaquarii added the 3. to review label Nov 4, 2019

ezaquarii force-pushed the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch from 9de6bc1 to dc32b64 Compare November 4, 2019 20:30

nextcloud deleted a comment Nov 4, 2019

ezaquarii force-pushed the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch from dc32b64 to 6a5928c Compare November 4, 2019 20:34

nextcloud deleted a comment Nov 4, 2019

ezaquarii force-pushed the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch from 6a5928c to fa4f166 Compare November 4, 2019 20:49

nextcloud deleted a comment Nov 4, 2019

AndyScherzinger reviewed Nov 4, 2019

View reviewed changes

src/main/res/xml/preferences.xml Outdated Show resolved Hide resolved

ezaquarii force-pushed the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch from fa4f166 to 3961fd6 Compare November 4, 2019 21:12

nextcloud deleted a comment Nov 4, 2019

ezaquarii force-pushed the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch from 3961fd6 to f8e1de0 Compare November 4, 2019 23:33

ezaquarii force-pushed the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch from f8e1de0 to 6b01382 Compare November 5, 2019 06:01

ezaquarii requested a review from AndyScherzinger November 5, 2019 06:03

AndyScherzinger reviewed Nov 5, 2019

View reviewed changes

AndyScherzinger approved these changes Nov 5, 2019

View reviewed changes

AndyScherzinger mentioned this pull request Nov 5, 2019

Obey dark/light setting by OS #4757

Closed

tobiasKaminsky approved these changes Nov 6, 2019

View reviewed changes

tobiasKaminsky merged commit 890789f into master Nov 6, 2019

delete-merged-branch bot deleted the ezaquarii/replace-shared-preferences-with-app-preferences-in-base-activity branch November 6, 2019 12:52

AndyScherzinger added this to the Nextcloud App 3.10.0 milestone Nov 6, 2019

Uh oh!

Replace BaseActivity.sharedPreferences with app preferences #4797

Replace BaseActivity.sharedPreferences with app preferences #4797

Uh oh!

Conversation

ezaquarii commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AndyScherzinger commented Nov 4, 2019

Uh oh!

nextcloud-android-bot commented Nov 4, 2019

Uh oh!

ezaquarii commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nextcloud-android-bot commented Nov 5, 2019

Uh oh!

nextcloud-android-bot commented Nov 5, 2019

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

Uh oh!

AndyScherzinger Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

ezaquarii Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger commented Nov 5, 2019

Uh oh!

tobiasKaminsky commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ezaquarii commented Nov 4, 2019 •

edited

Loading

codecov bot commented Nov 4, 2019 •

edited

Loading

ezaquarii commented Nov 4, 2019 •

edited

Loading

ezaquarii Nov 5, 2019 •

edited

Loading