Closed Bug 1152100 Opened 5 years ago Closed 5 years ago

Create a proper model for the settings page

Categories

(Firefox for iOS :: General, defect)

All
iOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: wesj, Assigned: nalexander)

Details

Attachments

(1 file, 1 obsolete file)

Right now the settings page is fairly hard coded. i.e.

getNumSections { return 3 }
getNumOfRowsInSection(section) { switch(section) { case 0: return 2; ... }

I'd like to drive this by a better model so that

1.) Methods and data for dealing with a particular pref can be contained, and
2.) We're not doing lots of index/math updating every time we change the position of something.

My initial proposal here is that the SettingsTableViewContoller just holds an array in memory like:

let settings = [
  SettingSection(title: nil, children: [
    StatusSetting(settings: self),
    ConnectSetting(settings: self)
  ]),
  SettingSection(title: "Search Settings", children: [
    SearchSetting(settings: self)
  ]),
  SettingSection("About", children: [
    VersionSetting(settings: self)
  ])
]

The settings objects themselves can implement something fairly simple like:
protocol Setting {
  var title: String?
  var description: String?
  var type: SettingType // Used to set decorations on the row?
  func onClick(viewController) -> Bool
}
Here's what I threw together for this nick. What do you think? I'm sure I'm making some use cases hard. Are they use cases we care about?
Attachment #8589350 - Flags: review?(nalexander)
Comment on attachment 8589350 [details] [review]
PR : https://github.com/mozilla/firefox-ios/pull/322

Sorry for the delayed review.  I think this changed under me, and not for the better.  I feel there are three commits here:

1) rewrite settings.  I was mostly OK with this, just wanted to think over and discuss the hidden stuff.

2) add some clear data stuff.

3) add a pref for clear data.

Please separate.
Attachment #8589350 - Flags: review?(nalexander) → feedback+
wesj: back to you.  There were several small issues with your patch that were best handled by making each setting hidden/not hidden on the basis of account == nil.  Dequeing cells is fine, except when it runs into iOS 8 bugs -- see comments and links in code.
Assignee: nobody → nalexander
Attachment #8589350 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8593677 - Flags: review?(wjohnston)
OS: Mac OS X → iOS
Hardware: x86 → All
Comment on attachment 8593677 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/344

I like this even better! Thanks! I'll rebase the clear private data stuff on it
Attachment #8593677 - Flags: review?(wjohnston) → review+
You need to log in before you can comment on or make changes to this bug.