Conversation
Labels are redundant since they "match" the cell text. They should be translated but they aren't. Identifiers aren't used AFAIK, and they shouldn't be translated.
Since the restoration method called init, it would restore as a plain table view and look all weird
Introduces the ImmuTable table view model
Row is now a protocol. I seem to be hitting some limitations of Swift type system, so there's some duplication. I'll keep trying
We can still use a protocol with type association to define common behavior, even if we don't require that as a type from the outside
Register cells instead of rows. Previously we registered each row's cell class but different rows can use the same cell type. Now we'll map rows to cell classes and identifiers and only register each cell once
Custom cells and rows now live in WPImmuTableCells.swift ImmuTable.swift should be generic
I got it wrong at first and set AccountSettings.account to cascade, but what I wanted was to set Account.settings to cascade. With the previous setting, on sign out the AccountSettings entity would persist with account=nil and would crash on save With the current setting, when the account is removed, its settings are removed as well
Implements account settings change management, and rolls back to previous value if remote request failed
|
Extracted ImmuTable to #4547 |
There was a problem hiding this comment.
I still find a bit weird that the properties that define an object are in an extension, and the helper methods in the class
There was a problem hiding this comment.
I think its sort of a take on Protocol-Based Programming in Swift. We should, ideally, be regenerating those Core Data accessors and not manually adding the properties to the classes. Over time those generated properties will change based upon newer best practices - this forces us to keep those accessors that would normally get overwritten separate.
I like/hate it too. 😄
There was a problem hiding this comment.
I like the separation. I just think the contents of the class/extension should be swapped. My main reason is that none of the code currently in the class would work if you take away the extension, but not the other way around
|
@koke: So I found a scenario I don't know if it is wrong, broken or just okay.
This is the ubiquitous sync / who wins / last synced time issue. The user really isn't aware of the network problem and we don't enqueue any future network request to sync again or the last time we synced those settings. The API side would also need to have this date somewhere to really support that. Thoughts? |
|
The other thing is if the network is slow this screen paints immediately with the data on the device and then suddenly when the remote request succeeds the data is updated. I know UI is coming in another PR - just a thought if we should consider some sort of blocking UI if the data is at least in an initial state (no data synced for my profile). |
|
The failed/loading UI is what comes next, but I felt like this was in a "good enough" state to avoid a huge PR.
I thought of just adding a UIRefreshControl to show something, but I'd rather just implement the final UI when the design is ready. I don't think we should release this before handling that, so maybe feature flag "My Profile" so we don't accidentally push it to the store? |
|
Feature flag sounds like a good idea, @koke! |
Since major comments are addressed in the next PR for UI.|
Feature flag work started in #4582 |
Implements My Profile section in Me, and also includes the backend changes necessary for the new Account Settings (UI coming up in separate PR)
Design decisions
Immutable value-type models. Already started this with People and I'm really liking the pattern. We still use Core Data as the storage layer but the app doesn't have to pass managed objects around anymore.
Network requests detached from data requests. You'll notice that the view controller now calls
refreshSettings(and it doesn't provide data on callback), and thensubscribeSettingsseparately. I've wanted to do this for a long time and the rationale behind this is that we want data updates whether we initiated the network request or not.Data subscription. This one needs a bit of polishing. It works but we're probably doing fetching way more often than needed (every save). I wanted to see if I could build the interface for the subscription system without ReactiveCocoa or RxSwift. I'm not even sure I'd want to use Core Data for this but it was the easiest path for a first implementation as we get a lot of useful features for free.
ImmuTable. I think if you are building a view that's just a table that's really kind of a form, your view controller should only have to specify those cells in a declarative way. The only code there should be how to get a model, turn it into a view model, and handle any changes. It still acts as the data source/delegate now, but I want to take that out as well.
Longer lived service. I'm thinking of a new name for this Service (Store, DataManager, ...), but since the question might come up, I'll answer it. IIRC Services were meant to be lightweight and disposable because keeping them for longer means having to deal with state changes. In this case, the service doesn't really keep any state. It's only properties are
accountIDandremoteand you could easily rewrite the whole thing as a bunch of top level functions that took an accountID and remote as arguments.Some remaining bits
These can go into separate PRs. As this already introduces a few new ideas, let's get this reviewed and I'll handle the issues later
Fixes #4499
I'm pinging three reviewers, but I'd appreciate everyone's opinion on this one
Needs Review: @sendhil @astralbodies @SergioEstevao