New Settings & Typed Preferences #61

kylehickinson · 2018-06-22T21:21:33Z

An attempt at cleaning up brave preferences from being scattered throughout to a single spot with static types which can be observed where their changes are relevant rather than the Settings screen reaching for the AppDelegate/BrowserViewController/etc. and altering app state.

Edit: Done/Deferred

Migration

Moving from Prefs → Preferences.Option<T> using new keys and standard user defaults over the suite FF used previously.
Moving users previous PIN info to shared app container instead of default keychain (or simply reverting back to default keychain over current FF implementation)
Moving non-settings-screen based preferences used (such as DDG popup) into Preferences

For settings that in browser-ios used BoolSetting without the change affecting something else in the app directly in BraveSettingsView, this implementation does the same.

General

Search Engines (Search Engine Equality #65): Deliberately left out before discussing alternatives to previous implementations
Save Login
Block Popups
Tab bar visibility†: Waiting on Brave Home Screens: Top Sites #67 and Toolbar/URL bar UI parity with 1.x #70 (See BrowserViewController.swift:L2713)
Password Managers (GH Issue missing)

Privacy

Clear Private Data
Cookie Control (Cookie Controls #66)
Private Browsing Only†

Security

Passcode (Used FF's implementation over the old implementation)

Shields

Block Ads & Tracking
HTTPS Everywhere
Block Phishing & Malware
Block Scripts
Fingerprinting Protection: See BrowserViewController.swift:L2714
Regional Ad-blocking† (Waiting for AdBlocker/Adblock Rules #71)

Support

Send crash reports and metrics
Report a bug
Privacy Policy
Terms of Use

† - Setting is included however implementation is waiting for other features to be ported

iccub · 2018-06-25T09:32:02Z

Client/Application/Preferences.swift

+    fileprivate init(key: String, default: ValueType, container: UserDefaults = UserDefaults.standard) {
+      self.key = key
+      self.container = container
+      value = (container.value(forKey: key) as? ValueType) ?? `default`


I'm not sure about that fallback to default value.
There are times where we want to see if a preference wasn't set yet.

Example: after you enter private mode there is a popup about switching search engine to duckduckgo.
Our logic goes like this:

no pref set - we show the popup

pref set to false - we don't show the popup but a small info label about ddg private search

pref set to true - don't show popup and callout label

As a workaround we could create an Int enum with not set case

This of course assumes we are going to use Preferences outside of settings. It's good for settings now as each setting option must have a default value.

@iccub My initial thought on that use-case you described was that it's perfect for a Bool? value type which gives you nil, false, and true as options, where the default would be nil (not set).

Since (NS)UserDefualts removes the key-value from the dictionary when you set nil it's basically "unsetting" it if were to ever reset it back to nil.

And since you already were doing checks to see if the pref was set you already were doing some sort of unwrapping.

As for other options: I tried an option where Option.value was an enum Value<ValueType> where it could be .set(T) or .notSet but it made things a bit more complicated than needed around getting/setting the value.

Another option is just to add a computed property to Option like so:

var isSet: Bool { return container.value(forKey: key) != nil }

However then we'd be dealing with a more confusing check when using the value itself (i.e. The bool default would be false, but isSet might return true).

Let me know if this all makes sense. I think we can do most use-cases including ones where you check if its initially set by simply using Optionals and it makes it a bit more clear to use.

That makes sense thanks for the clarification. Optionals with default nils should be enough.

tmancey · 2018-06-25T11:32:21Z

@kylehickinson Great work. Just a few things, such as breaking down viewDidLoad and hard coded values such as "static let TableViewHeaderFooterHeight = CGFloat(44)" but as you say this is work in progress.

kylehickinson · 2018-06-25T13:57:55Z

@tmancey Hah yeah I just copy pasted that over from brave/browser-ios quickly since I was missing some colours. I don't even think TableViewHeaderFooterHeight is used, but I'll remove all those before finalizing the PR

Still needs the removal of the time interval locking, and rather moving onto whether or not to bring it up upon backgrounding the app

Adds an option to other screens to define whether or not the passcode entry should be cancellable

kylehickinson · 2018-07-04T16:17:23Z

This has reached a good point to move on now. There is one outstanding thing that isn't in the task list: User Referral Program (URP #63) has a few prefs saved which aren't migrated or recreated in Preferences at the moment.

When URP is added it should probably move its preferences to a Preferences extension and migrate old values from the old Prefs object.

kylehickinson · 2018-07-04T16:18:05Z

Also another thing missing is test cases to test migration & storing preferences via Preferences

iccub

Looks very nice to me. Found mostly linter stuff and one place where old preferences are used.

I know some of this code was ported from v1 and you didn't change much to it.

iccub · 2018-07-05T14:51:14Z

BraveShared/URLCacheExtensions.swift

+
+  /// Setup default URLCache settings for Brave
+  public func setupBraveDefaults() {
+    memoryCapacity = 6 * 1024 * 1024; // 6 MB


Any reason for semicolons here?

Copy and paste :)

iccub · 2018-07-05T14:53:04Z

Client/Application/Migration.swift

+//  Migration.swift
+//  Client
+//
+//  Created by Kyle Hickinson on 2018-07-03.


Use MPL license.
On a side note, is there a way to set a template to replace default comments on top of a file? I know Android Studio has it.

Missed this one whoops, and hmm quick search: https://stackoverflow.com/a/46345626

Maybe I'll try adding this to this PR

iccub · 2018-07-05T15:10:53Z

Client/Application/Preferences.swift

+                container.set(value, forKey: key)
+                container.synchronize()
+
+                if observers.count() > 0 {


Why check for count > 0 if we are doing forEach anyway?

iccub · 2018-07-05T15:14:25Z

Client/Frontend/AuthenticationManager/BasePasscodeViewController.swift

+
+    var isCancellable: Bool = true {
+        didSet {
+            if isCancellable {


Can be inlined with that ? operator

iccub · 2018-07-05T15:17:41Z

Client/Frontend/AuthenticationManager/BasePasscodeViewController.swift

+        }
+    }
+
+    private lazy var cancelBarButtonItem = UIBarButtonItem(barButtonSystemItem: .cancel, target: self, action: #selector(self.dismissAnimated))


I think we can remove self from selector's method.

iccub · 2018-07-05T15:34:39Z

Client/Frontend/Settings/ClearPrivateDataTableViewController.swift

+
+        let actionSheet = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet)
+        let clearAction = UIAlertAction(title: Strings.ClearPrivateData, style: .destructive) { (_) in
+            self.profile.prefs.setObject(self.toggles, forKey: TogglesPrefKey)


Shouldn't this use the new preferences too?

iccub · 2018-07-05T15:36:19Z

Client/Frontend/Settings/Clearables.swift

    let dir = library.appendingPathComponent(folder)
-    let contents = try manager.contentsOfDirectory(atPath: dir.path)
+    var contents = try manager.contentsOfDirectory(atPath: dir.path)


It can stay as let.

iccub · 2018-07-05T15:39:44Z

Client/Frontend/Settings/OptionSelectionViewController.swift

+//  OptionSelectionViewController.swift
+//  Client
+//
+//  Created by Kyle Hickinson on 2018-06-25.


MPL license.

iccub · 2018-07-05T16:06:39Z

Client/Application/Migration.swift

+            if let value = userDefaults?.object(forKey: profileKey) as? T {
+                option.value = value
+                userDefaults?.removeObject(forKey: profileKey)
+            }


We could add a log here if value was not unwrapped, to catch any typos with preference keys from v1.

Only worry I have is being spammed for keys which aren't set at all yet (i.e. popupForDDG if the user never used private tabs yet will not exist)

iccub · 2018-07-05T16:08:16Z

Client/Application/Preferences.swift

+    static let defaultContainer = UserDefaults(suiteName: AppInfo.sharedContainerIdentifier)!
+}
+
+extension Preferences {


I see two Preferences extensions, I guess one refers to settings only and second is for other things, maybe put a MARK: comment to make it more visible?

jhreis · 2018-07-05T18:43:25Z

Client/Frontend/AuthenticationManager/BasePasscodeViewController.swift

    override func viewDidLoad() {
        super.viewDidLoad()
        view.backgroundColor = SettingsUX.TableViewHeaderBackgroundColor
-        navigationItem.rightBarButtonItem = UIBarButtonItem(barButtonSystemItem: .cancel, target: self, action: #selector(self.dismissAnimated))
+        navigationItem.rightBarButtonItem = isCancellable ? cancelBarButtonItem : nil


Despite the minimal duplication, this probably need to be abstracted. Rather than giving isCancellable a default value, could set it in the init (although inits with VCs are annoying). Or use a separate function that they both call.

jhreis · 2018-07-05T18:45:16Z

Client/Frontend/Browser/BraveWebView.swift

+import Shared
+import JavaScriptCore
+
+class BraveWebView: TabWebView {    


Depends what type of mechanism is in v2 currently. Browser-ios limits number of in-memory tabs, and will start to deallocate them if it reaches a certain threshold. v2 may already have similar "caching" logic in place to properly deallocate webviews while leaving the underlying data intact.

jhreis · 2018-07-05T21:56:24Z

Client/Frontend/Settings/ClearPrivateDataTableViewController.swift

+                // TODO: add API to avoid add/remove
+                self.tabManager.removeTab(self.tabManager.addTab())
+            }
+            //      }


Can this hanging paren be removed?

Its actually part of the // TODO above. When we have Private Browsing back in we need to re-visit this and re-enable the logic commented out around this.

jhreis · 2018-07-05T21:56:51Z

Client/Frontend/Settings/ClearPrivateDataTableViewController.swift

+            Preferences.Privacy.clearPrivateDataToggles.value = self.toggles
+            self.clearButtonEnabled = false
+
+            //      NotificationCenter.default.removeObserver(self)


This looks like a notification reset, if not needed clear it out.

I'm going to remove this for now anyways, but we may end up re-visiting if we use BraveWebView to bring back counting and have to some sort of notification system for letting the controller know when that count updates

kylehickinson · 2018-07-06T14:28:28Z

@jhreis Let me know if you still want BraveWebView removed for the time being

jhreis

There are a couple small things (e.g. lazy vars), but given that most of this code is just moved from current project, I really would prefer to get it in. Related items are so minor and not technically incorrect, so no point in blocking on them rn. Nice work. Huge PR.

jhreis · 2018-07-11T14:47:52Z

Client.xcodeproj/xcshareddata/IDETemplateMacros.plist

+<plist version="1.0">
+<dict>
+	<key>FILEHEADER</key>
+	<string> This Source Code Form is subject to the terms of the Mozilla Public


I know this is needed, it is just painfully dumb... haha.

Adds strongly typed brave preferences and wip refactored settings screen

9a49e2a

iccub reviewed Jun 25, 2018

View reviewed changes

Kyle Hickinson added 9 commits June 27, 2018 16:26

Port most settings screens

39e8329

Remove a majority of FF settings system and FF specific settings

eb60ba6

Fix URLCache extension from using shared regardless of instance

45a1e23

Adds back passcode browser lock feature (WIP)

e42cbee

Still needs the removal of the time interval locking, and rather moving onto whether or not to bring it up upon backgrounding the app

Remove time interval based passcode validation requirements

c3b3db7

Make app passcode entry not cancellable

c2be24c

Adds an option to other screens to define whether or not the passcode entry should be cancellable

Add preference observing to BrowserViewController

d7dfa76

Make app update cookie accept policy at launch based on prefs

ef5c708

Cleanup some old pin related code

f0a105a

kylehickinson force-pushed the preferences branch from e3d36e7 to f0a105a Compare June 29, 2018 21:30

kylehickinson mentioned this pull request Jul 3, 2018

Fix #76: Updated Brave README. #77

Merged

Cleanup, add 4 spaces pref to project & added Xcode 10 compatibility

e930ebb

kylehickinson force-pushed the preferences branch from 86b0249 to e930ebb Compare July 3, 2018 20:16

Kyle Hickinson added 3 commits July 3, 2018 18:40

Add migration logic for main settings

8719dcc

Adds password manager button behaviour selection in settings + cleanup

469c2fd

Remove preferences that aren't used throughout

80f7a40

kylehickinson changed the title ~~[WIP] New Settings & Typed Preferences~~ New Settings & Typed Preferences Jul 4, 2018

kylehickinson requested review from tmancey and jhreis July 4, 2018 16:14

Kyle Hickinson added 5 commits July 4, 2018 12:31

Add migration for DDG popup

36954db

Remove the old key from UserDefaults when migrated over

61c4591

Removed some test code

54bc090

Fix potential build issue in ported Clearables code

461dfa9

Fix an accessibility identifier

f1e24a4

iccub reviewed Jul 5, 2018

View reviewed changes

Kyle Hickinson added 3 commits July 5, 2018 12:57

Fix licenses and code cleanup

f2f6ef1

Add FILEHEADER override so all newly created files use MPL

18cf78a

Add pref migration logging + fix crash when migrating an optional Option

29d7b6c

jhreis reviewed Jul 5, 2018

View reviewed changes

Review cleanup

4e08769

jhreis mentioned this pull request Jul 11, 2018

User Referral Promo (URP) Integration #63

Closed

5 tasks

jhreis approved these changes Jul 11, 2018

View reviewed changes

jhreis merged commit c2236f0 into development Jul 11, 2018

jhreis deleted the preferences branch July 11, 2018 15:22

kylehickinson mentioned this pull request Jul 24, 2018

Cookie Controls #66

Closed

jhreis mentioned this pull request Aug 24, 2018

Daily Active User (DAU) Integration #189

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Settings & Typed Preferences #61

New Settings & Typed Preferences #61

kylehickinson commented Jun 22, 2018 •

edited

iccub Jun 25, 2018

kylehickinson Jun 25, 2018

iccub Jun 25, 2018

tmancey commented Jun 25, 2018

kylehickinson commented Jun 25, 2018

kylehickinson commented Jul 4, 2018

kylehickinson commented Jul 4, 2018

iccub left a comment

iccub Jul 5, 2018

kylehickinson Jul 5, 2018

iccub Jul 5, 2018

kylehickinson Jul 5, 2018

iccub Jul 5, 2018

iccub Jul 5, 2018

iccub Jul 5, 2018

iccub Jul 5, 2018

iccub Jul 5, 2018

iccub Jul 5, 2018

iccub Jul 5, 2018

kylehickinson Jul 5, 2018

iccub Jul 5, 2018

jhreis Jul 5, 2018

jhreis Jul 5, 2018

jhreis Jul 5, 2018

kylehickinson Jul 5, 2018

jhreis Jul 5, 2018

kylehickinson Jul 5, 2018

kylehickinson commented Jul 6, 2018

jhreis left a comment

jhreis Jul 11, 2018

New Settings & Typed Preferences #61

New Settings & Typed Preferences #61

Conversation

kylehickinson commented Jun 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey commented Jun 25, 2018

kylehickinson commented Jun 25, 2018

kylehickinson commented Jul 4, 2018

kylehickinson commented Jul 4, 2018

iccub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson commented Jul 6, 2018

jhreis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylehickinson commented Jun 22, 2018 •

edited