feature/settings #33

pablocarmu · 2018-05-02T10:04:11Z

Settings Screen

The way i'm coding this is:

I've created a parent settings screen called SettingsViewController which inherits from StaticTableViewController.
Instead of coding all the logic in this view controller, I've split the settings into three different categories:
- Security: In this category should be coded:
  - frequency: the frequency wich the security methods should be prompted to the user, the options I proposed here are: allways, 1 minute, 5 minutes, 30 minutes, 1 hour .
  - Passcode: custom passcode, a new ViewController should be implemented for this, there is some open source libraries but I prefer to code this instead of using an external dependency.
  - TouchID/FaceID: if the device support any kind of biometric ID prompt the user with this option only if the passcode option is enabled.
- Instant Uploads: In this category should be coded:
  - Photo uploads: This should allow to instant upload photos.
  - Video uploads: This should allow to instant upload videos.
  - Background uploads: Allow background uploads for the selected video or photo options. This option should be presented only if one of the video of photo uploads are selected.
  - Wifi only uploads: Like the above option, this should be presented only if one of the video of photo uploads are selected.
  - Accout to upload: This should present to the user a list of bookmarks you have in the app and select the account you want to be the one in which the instant uploads should be put.
  - Path for instant uploads: Here you can select the folder you want to be the container for the instant uploads.
- Various Things: Here we should code:
  - Open source licenses: Is required to put the license or a link to the license for every third party libreary we use.
  - About ownCloud
  - Feedback
  - More things...

Coding each category into a separate class may look as an overhead, but this allow us to:

Select what settings we want to be shown: Maybe in the future, some users/customers want to ban instant uploads, so this way in a single line of code you avoid the creation and presentation of this settings.
Decouple the settings: Instead of one massive class with all the settings you have 3 classes (each one has a StaticTableViewSection) so you have a more decoupled settings system. This should make us able to have the code isolated and add in the future more settings/categories without make the parent settings view a massive class.

The issue to comment more settings or another way to abord this feature is linked to this pull request so if you have any requirements or any things to discuss please comment there and ping me (@pablocarmu) in the comment so we can get this pull request comment as clean as posible.

felix-schwarz · 2018-05-02T12:05:23Z

@pablocarmu:

Decouple the settings: Instead of one massive class with all the settings you have 3 classes (each one has a StaticTableViewSection) so you have a more decoupled settings system. This should make us able to have the code isolated and add in the future more settings/categories without make the parent settings view a massive class.

Unless there's a reason not to do so, please consider making these classes subclasses of StaticTableViewSection.

felix-schwarz · 2018-05-04T08:05:00Z

ownCloud/Settings/SecuritySettingsSection.swift

+        self.headerTitle = "Security".localized
+        self.identifier = SecuritySectionIdentifier
+
+        updateUI()


Instead of breaking the creation of the rows into methods, and then checking if they exist using their identifier lateron, you could generate all rows here in the init-method and save them in instance variables.

Benefits:

you don't need to keep track of whether or not they were already created (=> they're all just there already, ready for use).

you can use the row objects directly and don't have to find and fetch them through their identifiers (=> you also don't need to generate symbols for the identifiers (like f.ex. SecurityBiometricsRowIdentifier) then, simplifying the implementation further)

to change the interface dynamically, you could either keep using add/remove methods on the StaticTableViewSection - or put together a new rows array and ask the tableView to reload just that section. Ideally, you could extend StaticTableViewSection with a "replaceRows" method that replaces the rows instance var with a passed in array, and triggers a section reload. I also noticed that StaticTableViewSection doesn't support "live" additions/removals and animation yet, so it may be a good idea to:

add animation support to add(rows:) (like add(rows:animated:) where animated defaults to false and only calls insertRows(at indexPaths: [IndexPath], with animation: UITableViewRowAnimation) if the viewController property is set)

add a remove(rows:animated:) method that does the same, just for removal

add add(row:animated:) and remove(row:animated:) convenience methods that wrap the passed row into an array and forward it to the add(rows:animated:) and remove(rows:animated:) methods.

in the future, an insert(rows:indexes:animated:) method could be added, too - and add(rows:animated:) be changed to become just a convenience method to call that. But if it's not needed at the moment, this can surely be saved until we need it.

- Split the settings between Security ones, Instant Uploads ones and other settings. - Used UserDefaults for storing settings but the pincode

@felix-schwarz

- Followed @felix-schwarz recommendations and changed settings sections from NSObject to StaticTableViewSection. - Updated StaticTableViewController and StaticTableViewSection to be able to update and reload a single section.

…nd photo upload options are deactivated

- Implemented insert(row, at) method to StaticTableViewSection. - Implemented new options for the setting, now the wifi only and selected path for each video and photos.

…s that needs to retrieve them. - Remove unused @discardableResults annotations. - Change the settings table view style to .grouped. - Coded UI for the last section of settings called More. - Created ViewController with a WKWebView inside to be able to push it through the navigation controller. - Show in a webview the help and privacy policy webpages.

- Renamed sharedBookmarkManager property on BookmarkManager.swift to shared

…s from methods to instance properties. - Changed SecurityAskfrequency, now it's Int based enum.

- Change the row creation in the MoreSettingsSection to instance properties. - Added new remove method to StaticTableViewSection for removing only one row.

- Fixed som typo errors. - Updated the method calls to insert/remove cells in a "live" way. - Minor bug fixes.

- Remove old unused code.

- Changed the way the frequency options are prompted from an AlertView to a separate View Controller.

- Updated project.

felix-schwarz

Overall this looks really good! 👍

Just one more thing:
If you have a block of code that inserts or removes more than one row, please wrap it in a self.tableView.performBatchUpdates() (=> for an example, check BookmarkViewController.composeSectionsAndRows, which covers both animated and non-animated versions).

felix-schwarz · 2018-05-16T07:34:18Z

ownCloud/Settings/UploadsSettingsSection.swift

+        } else {
+            isPhotoWifiOnlyUploadsEnabled = false
+            remove(rows: [photosWifiOnlyRow!, photosSelectedPathRow!], animated: true)
+            userDefaults.removeObject(forKey: UploadsPhotoSelectedPathKey)


Please move this to the willSet/didSet closures for isPhotoUploadEnabled, too, to keep UI code and defaults manipulation separate.

felix-schwarz · 2018-05-16T07:39:19Z

ownCloud/Bookmarks/BookmarkManager.swift

@@ -22,7 +22,7 @@ import ownCloudSDK
 class BookmarkManager: NSObject {
 	public var bookmarks : NSMutableArray

-	static var sharedBookmarkManager : BookmarkManager = {
+	static var shared : BookmarkManager = {


Ah, so much nicer 😃

felix-schwarz · 2018-05-16T07:40:36Z

ownCloud/Resources/Localizable.strings

+"Security" = "Security";
+"Passcode Lock" = "Passcode Lock";
+"FaceID" = "FaceID";
+"TouchID" = "TouchID";


Please change this to "Face ID" and "Touch ID" (adding a space) to match Apple's naming scheme.

felix-schwarz · 2018-05-16T07:43:08Z

ownCloud/Settings/GlobalSettingsViewController.swift

@@ -25,113 +25,113 @@ class GlobalSettingsViewController: StaticTableViewController {



I think we can remove this file now.

Maybe worth to keep this file in the repo as some kind of "recipes". It really helped me some times to understand how the StaticTableView... structure works. As we are open source maybe itcould be valuable for contributors.

felix-schwarz · 2018-05-16T07:47:11Z

ownCloud/Settings/MoreSettingsSection.swift

+    override init(userDefaults: UserDefaults) {
+        super.init(userDefaults: userDefaults)
+        self.headerTitle = "More".localized
+        self.footerTitle = "owncloud 2018 iOS beta"


You can get the version number from NSBundle.main.object(forInfoDictionaryKey: ) using kCFBundleVersionKey (build number) and "CFBundleShortVersionString" (version number, i.e. "1.0") as keys and display it here.

felix-schwarz · 2018-05-16T08:22:12Z

ownCloud/Settings/SettingsViewController.swift

+
+        self.navigationItem.title = "Settings".localized
+
+        let standardUserDefaults = UserDefaults.standard


IIRC we'll need to grab a different UserDefaults instance via UserDefaults(suiteName: appGroupIdentifier) here. However, since it'll be needed in more places, I'd really like to add that to OCAppIdentity, so SettingsViewController can grab it from there in the future.

For now, please use

// TODO: Use OCAppIdentity-provided user defaults in the future let userDefaults = UserDefaults(suitename: OCAppIdentity.sharedAppIdentity().appGroupIdentifier)

felix-schwarz · 2018-05-16T08:23:36Z

ownCloud/Settings/UploadsSettingsSection.swift

+
+        } else {
+            isVideoWifiOnlyUploadsEnabled = false
+            userDefaults.removeObject(forKey: UploadsVideoSelectedPathKey)


Please move this to the willSet/didSet closures for isVideoUploadEnabled, too, to keep UI code and defaults manipulation separate.

felix-schwarz · 2018-05-16T08:27:22Z

ownCloud/UI Elements/StaticTableViewSection.swift

@@ -182,4 +182,12 @@ class StaticTableViewSection: NSObject {

 		return nil
 	}
+
+    private func selectAnimation(_ animated: Bool) -> UITableViewRowAnimation {


This doesn't seem to be used anywhere. Please remove it.

felix-schwarz · 2018-05-16T08:29:40Z

ownCloud/UIKit Extensions/LAContext+Extension.swift

+        if  canEvaluatePolicy(.deviceOwnerAuthenticationWithBiometrics, error: nil) {
+            switch self.biometryType {
+            case .faceID : return "Face ID"
+            case .touchID: return "Touch ID"


Please add ".localized" to make use of localizations in Localizable.strings.

felix-schwarz · 2018-05-16T08:29:58Z

ownCloud/UIKit Extensions/LAContext+Extension.swift

+
+extension LAContext {
+
+    func supportedBiometricsAuthenticationNAme() -> String? {


Tiny typo here: "NAme" vs "Name" at the end of the method.

- Some minor typo. - Implemented the build number and version number in the footer of the setting header. - Implemented the build number and version number as subject in the feedback email. - Removed section identifier constants. - Fixed a bug in the frequency selector. - Changed the user defaults from .standard to one provided by de SDK. - Removed unused code.

felix-schwarz · 2018-05-18T07:37:16Z

ownCloud/Settings/MoreSettingsSection.swift

+        return ""
+    }
+
+    var appBuildNumber: String {


Great idea to make these "properties". 👍

felix-schwarz · 2018-05-18T07:40:53Z

ownCloud/Settings/SecuritySettingsSection.swift

@@ -164,6 +161,8 @@ class SecuritySettingsSection: SettingsSection {

            var rowsToRemove: [StaticTableViewRow] = []
            frequencyRow?.cell?.detailTextLabel?.text = SecurityAskFrequency.always.toString()
+            frequency = .always


Might want to move this to isPasscodeSecurityEnabled/willSet.

michaelstingl · 2018-05-18T08:33:16Z

Suggested changes:

Show Git hash (on bottom with version?)
Hide/explain unfinished settings
Open links in SFSafariViewController
Make links to Help/Privacy brandable/MDM
Make entries like "Recommend to a friend" brandable/MDM

jesmrec · 2018-05-18T08:59:48Z

A couple of minor details regarding the settings view

In Send feedback option, the subject of the mail has a lack of one )
In the bottom of the view you wrote Owncloud instead of ownCloud . The content of such label has to be brandable as well.

- Fix ownCloud typo. - Replaced the WKWebkit with SFSafariViewController. - Implemented user confirmation when open external url through SFSafariViewController. - Show the git commit next to the app's version and build number.

jesmrec · 2018-05-22T07:21:15Z

Approved

pablocarmu changed the title ~~Settings Screen (Work in progress)~~ feature/settings May 3, 2018

felix-schwarz reviewed May 4, 2018

View reviewed changes

pablocarmu force-pushed the feature/settings branch from c0a3b46 to 354fb4d Compare May 9, 2018 15:38

pablocarmu changed the base branch from master to feature/bookmarks May 10, 2018 07:56

pablocarmu changed the base branch from feature/bookmarks to master May 10, 2018 07:57

Pablo Carrascal and others added 17 commits May 15, 2018 10:24

- Created new settings parent view.

8e8816c

- Split the settings between Security ones, Instant Uploads ones and other settings. - Used UserDefaults for storing settings but the pincode

- Make detail cell themable

70fde22

- Created basic flow for the instant uploads settings.

3994f15

- Updated the background uploads and wifi only uploads if the video a…

b586f92

…nd photo upload options are deactivated

- Changed the reload method in StaticTableViewController.

05a14dd

- Implemented insert(row, at) method to StaticTableViewSection. - Implemented new options for the setting, now the wifi only and selected path for each video and photos.

- Now were able to send email for feedback and for recommend to friends.

d628f2f

- Fixed swiftlint optional unwraping issues.

b62e527

- Renamed sharedBookmarkManager property on BookmarkManager.swift to shared

- Changed creation of the rows in SecuritySettings and UploadsSetting…

28883e0

…s from methods to instance properties. - Changed SecurityAskfrequency, now it's Int based enum.

- Localize SecurityAskfrequency Strings.

c8642cd

- Change the row creation in the MoreSettingsSection to instance properties. - Added new remove method to StaticTableViewSection for removing only one row.

- Implemented live reloading to StaticTableViewSection.

79bc97e

- Rebased with feature/bookmarks.

3464fd5

- Fixed som typo errors. - Updated the method calls to insert/remove cells in a "live" way. - Minor bug fixes.

- Fix bug when the app is restarted.

783e392

- Remove old unused code.

- Fix Allways typo

cc67c3f

- SDK updated.

aa5af5f

- Changed the way the frequency options are prompted from an AlertView to a separate View Controller.

- SDK Update.

b1712fc

- Updated project.

pablocarmu force-pushed the feature/settings branch from bc133a4 to b1712fc Compare May 15, 2018 08:32

felix-schwarz requested changes May 16, 2018

View reviewed changes

felix-schwarz reviewed May 18, 2018

View reviewed changes

felix-schwarz approved these changes May 18, 2018

View reviewed changes

- Fix the Send feedback typo.

1f18c6b

- Fix ownCloud typo. - Replaced the WKWebkit with SFSafariViewController. - Implemented user confirmation when open external url through SFSafariViewController. - Show the git commit next to the app's version and build number.

jesmrec mentioned this pull request May 22, 2018

[Settings] Version and commit in settings view #51

Closed

jesmrec merged commit 1dc1da6 into master May 22, 2018

felix-schwarz deleted the feature/settings branch June 20, 2018 07:52

jesmrec added this to the 0.1.0 milestone Jul 2, 2018

jesmrec mentioned this pull request Aug 1, 2018

Uploads #101

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/settings #33

feature/settings #33

pablocarmu commented May 2, 2018

felix-schwarz commented May 2, 2018

felix-schwarz May 4, 2018

felix-schwarz left a comment

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

pablocarmu May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 16, 2018

felix-schwarz May 18, 2018 •

edited

Loading

felix-schwarz May 18, 2018

michaelstingl commented May 18, 2018 •

edited

Loading

jesmrec commented May 18, 2018 •

edited

Loading

jesmrec commented May 22, 2018

		@@ -25,113 +25,113 @@ class GlobalSettingsViewController: StaticTableViewController {


		self.navigationItem.title = "Settings".localized

		let standardUserDefaults = UserDefaults.standard


		extension LAContext {

		func supportedBiometricsAuthenticationNAme() -> String? {

feature/settings #33

feature/settings #33

Conversation

pablocarmu commented May 2, 2018

Settings Screen

felix-schwarz commented May 2, 2018

Choose a reason for hiding this comment

felix-schwarz 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

felix-schwarz May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelstingl commented May 18, 2018 • edited Loading

jesmrec commented May 18, 2018 • edited Loading

jesmrec commented May 22, 2018

felix-schwarz May 18, 2018 •

edited

Loading

michaelstingl commented May 18, 2018 •

edited

Loading

jesmrec commented May 18, 2018 •

edited

Loading