Updated ReminderForm to use more standard SwiftUI #13
Conversation
Replaced all of ReminderForm's State(wrappedValue:) with a Binding. This made it possible to remove ReminderForm's custom init. Added ReminderFormConfig for the form's data and logic. Change from .sheet(item:) which is for static content to .sheet(isPresented:) which is for dynamic. Replaced the Environment dismiss with isPresented = false. There are other instances of State(wrappedValue:) in the project which could also be replaced with Binding.
* Fix dynamic queries example * Update Sources/SharingGRDB/Documentation.docc/Articles/DynamicQueries.md * Update Sources/SharingGRDB/Documentation.docc/Articles/DynamicQueries.md --------- Co-authored-by: Stephen Celis <stephen.celis@gmail.com>
|
Hi @malhal! Thanks for taking the time to PR this. We do have some questions though.
This is not our impression. The And this is what you have done with the struct ReminderFormConfig {
// Data for sheet
var isEditing = false
}…but this is not ideal. You are now responsible for setting up all of this state when the mutating func present(
remindersList: RemindersList,
reminder: Reminder = Reminder(listID: 0),
selectedTags: [Tag] = []
) {
isEditing = true
self.reminder = reminder
self.remindersList = remindersList
self.selectedTags = selectedTags
}But in our opinion this is pretty dangerous to use. It is completely possible to invoke this method in the wrong way, for example in your private func showDetails() {
editReminderFormConfig.present(remindersList: remindersList)
}But this now presents a private func showDetails() {
editReminderFormConfig.present(remindersList: remindersList, reminder: reminder)
}…and now we run the risk of losing the tags on this reminder if the user hits “Save”. And further, right now mutating func cancel() {
isEditing = false
}But it would probably be better to reset everything back to their defaults. If you don’t, you run the risk of presenting the form with data from the last time the form was presented. None of these problems are possible when you model the state as a single optional field: @State var editReminder: Reminder?All you have to do is set the value: editReminder = reminder…and that both drives navigation and gives the data to the sheet for it to do its job. And then dismissing is a matter of We also think it’s a little strange that private func showDetails() {
guard let reminderID = reminder.id else { return }
do {
let tags = try _database.wrappedValue.read { db in
try Tag.all()
.joining(optional: Tag.hasMany(ReminderTag.self))
.filter(Column("reminderID").detached == reminderID)
.order(Column("name"))
.fetchAll(db)
}
editReminderFormConfig.present(remindersList: remindersList, reminder: reminder, selectedTags: tags)
} catch {
reportIssue(error)
}
}This is all work that used to be in just the And lastly, this refactor using @State var isDeleteAlertPresented = falseBut now there are two booleans controlling navigation and we only ever expect at most one to be @State var destination: Destination?
enum Destination {
case edit(Reminder)
case delete
}However, this doesn’t really work with
Can you explain more why you have done this? They are functionally equivalent since all All-in-all we are not sure these changes are really “standard SwiftUI”, as you say. If you know of some documentation that backs up some of these claims we’d love to see it. However, the initializer of If you’d like you could rebase your branch onto |
|
The EditorConfig pattern can be seen here: https://developer.apple.com/videos/play/wwdc2020/10040?time=214 |
|
I agree the config's func params could be tightened up. It is quite complicated given the list, reminder and tags are all independent value types rather than objects that have relationships. I took a quick look at the PR and I don't think it is correct because it is still attempting to link States together instead of using Binding but will rebase and test. |
I personally just don’t think this is a technique that should be followed. It has all of the problems I mentioned above:
So that is a lot of downsides to use this config type, and I also just don't see any upsides whatsoever. What are the benefits to using this extra type of imprecisely modeled domain? What is it "fixing" in the current code?
While relationships in SwiftData can be powerful, there is also a cost to them. For one, they force models to be classes instead of structs. We feel it is a great benefit that things like
Can you explain more why you think |
|
The issue with data hanging around is just the new value-type world we live in. Does seem weird to those used to objects. There is no need to nil out values. When cancelling the sheet, it needs to keep the values or there would be a visual glitch, that is why it can't use the item API. |
|
As it was shown in the video there should only be once source of truth. If the State is init with initial reminder that uses supplied remindersList.id then if the View is re-init with different reminderList then the reminder won't be in the correct list. There is an alternative, you pass in the reminderID as a let and fetch the reminder into local state with a query. Also you could optimise the reminder list's query by only fetching the attributes of reminder the list actually shows and not what the editor needs. However going down this path of the Views fetching data usually will run into issues compared to having a model layer like CoreData and SwiftData provides. The reason is with those frameworks it is possible for multiple views to show the unsaved data. |
I’m sorry, but we don’t agree that it’s the world we live in. The main thrust of our open source work is specifically to show that we don’t have to be constrained by limitations in SwiftUI. We build tools that allow using enums to model state, and this repo even shows that SwiftData like tooling can exist in
This is not true. Sheets show the view with the last non- In fact, it is this “config” style of presenting/dismissing from the WWDC video that has the visual glitch. Suppose that you did want to clean up the state when dismissing by updating the mutating func cancel() {
self = Self()
}That seems to solve the problem handily. The boolean is flipped back to However, doing so will cause the view to clear out its state while it dismisses since the sheet has no choice but to show the empty state. Here is what your version of the app looks like if I make the above seemingly innocuous change: Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-17.at.12.12.27.mp4Notice that the state in the view clears out as it is dismissing. And so you either need to hold onto extra state that doesn’t represent anything real, or you need to introduce a visual glitch into the app.
Our current way of doing things also has a single “source of truth”, but you just have to know that only the initial value of The situation you are describing of changing the reminder and that causing the form to show the incorrect data is simply not possible. Sheets are driven by So that means if somewhere in our app we do something like this: editReminder = try await database.read { db in
try Reminder.fetchOne(db, id: 9)
}…while the
That seems unrelated to the conversation at hand, but if you want to PR that change we would happily take it.
I’m not sure I understand this. What are the issues that we will encounter, and are those issues present in this app or any of the case studies and demos in the repo?
That isn’t needed in this demo, and so seems strange to contort our code in strange ways to support it. Also, if we needed multiple views to display an unsaved |
|
The State values should not be cleaned up. The same way you wouldn't set an int back to zero after doing a calculation. That is what I meant by value-type world. Unfortunately Binding can only be passed down. If there was a completely separate part of the UI that needed to see the unsaved data then that is why it would retrieve it from a context like a MOC. |
* Update examples/docs to use `DatabasePool` for non-in-memory * wip
If you do not clean up the state you can easily introduce a bug that shows stale data. For example, if a view can both present the form for editing an existing reminder and creating a new reminder, then you might have a sequence of events like this: // Present form for existing reminder:
config.present(reminderList: reminderList, reminder: reminder)
// Later somewhere else in the view, dismiss form:
config.cancel()
// Later somewhere else in the view, present form for new reminder:
config.present(reminderList: reminderList)Unfortunately this is a bug. The new reminder form will be presented with the data from the existing reminder when
Typically a lexical scope would end the lifetime of an integer not being used anymore: do {
let number = compute()
print(number)
}
number // 🛑 Cannot find 'number' in scopeAnd that is what I am advocating for here. When a view is dismissed its “scope” has ended, and so its state should be inaccessible.
This too seems unrelated to your PR and the broader discussion. This is a theoretical feature that we do not have in our demo and it's not concretely stated how it is meant to work. But for what it is worth, I guarantee that no matter its specification it certainly would be possible to implement without all the drawbacks that this config pattern brings. |
Replaced all of ReminderForm's State(wrappedValue:) with a Binding. This made it possible to remove ReminderForm's custom init. Added ReminderFormConfig for the form's data and logic. Change from .sheet(item:) which is for static content to .sheet(isPresented:) which is for dynamic. Replaced the Environment dismiss with isPresented = false. There are other instances of State(wrappedValue:) in the project which could also be replaced with Binding.
# Conflicts: # Examples/Reminders/ReminderForm.swift
|
Hi @malhal, it doesn't seem like your merge happened cleanly. The diff is showing a lot of things from other commits to the repo. Do you want to proceed with fixing up this PR? |
|
We can just close it. Then perhaps if you change your mind about the config pattern we can revisit making the changes again in the future. |
|
If you are able to address the issues I brought up above we could for sure consider adopting your changes. But as I showed above, this extra config type makes it possible to easily introduce bugs and visual glitches to navigation, and goes against how SwiftUI is intended to work. We really don’t think it’s fair to assume that Apple is demonstrating a pattern in that WWDC video to always follow, but rather showing how to do something in a simple, if flawed, way. |
|
Regarding the risky present func, this:
And this:
Was my fault by having the default params in the func: Should instead be this If the caller wants to create a new reminder instead of supplying an existing one they would init the empty one like this: However, there are likely still ways the config can be misused, especially since all the vars are public. There may be ways to tighten that up. I'm also experimenting with a closure in the config struct, so the save action can be declared where present is called, will report back on how that works out. Regarding when I introduced the issue of a visual glitch: |
|
Edit: it turns out storing closures in State is a bad idea (.e.g the Thought I'd share an example of a variation of the config pattern that tightens up the API by replacing the baked in |
|
Sorry to bombard you with comments but I just finished this version which goes back to the baked in isEditing but it resets the editor state when the sheet dismisses, using the onDismiss sheet action. I think its cleaner than the computed binding but has the weakness Have a nice weekend! |
|
Hi @malhal, thanks for continuing to share your ideas. However, there are still bugs with your code, and in fact now they are even more pernicious than before. If the data source of the To see this, update the Button(item.text) {
editor.beginEditing(initialText: item.text) { result in
if case let .success(text) = result {
item.text = text
}
}
+ Task {
+ try await Task.sleep(for: .seconds(1))
+ items.removeFirst()
+ }
}This emulates the data source changing by removing the first item a second after opening the sheet. If you open the first item, make an edit, then hit “Save”, you will find that the changes were applied to the second item. And so now the data has been corrupted. Further, if you were to open the second item, make an edit, and then hit “Save”, you will get a crash. This is happening because you are escaping the And even beyond those problems, there are still a lot more problems with this code (both versions). You note some of the problems:
Both of these comments are two sides of the same coin. This is what Curtis is referring to in the WWDC video when he says “EditorConfig can maintain invariants on its properties”. That sounds nice on the surface, but a runtime invariant is never as strong as a compile-time invariant. With compile-time invariants you do not need to do extra work to clean up after the state. Whereas maintaining code like this is a pain and prone to bugs: var isEditing = false {
didSet {
if isEditing && action == nil {
print("you must use beginEditing")
isEditing = oldValue
}
}
}Also, that little line being printed to the console will be easy to miss, and so it will be confusing why doing something like This imprecision of the domain also bleeds out in the .sheet(isPresented: $editor.isEditing) {
// onDismiss
editor = Editor()
} content: {One must know to override the Here is how we think this code should be written: struct ContentViewV2: View {
@State var items: [ListItem] = [ListItem(text: "Test 1"), ListItem(text: "Test 2")]
@State var addItem: ListItem?
@State var editItem: ListItem?
var body: some View {
NavigationView {
List($items, editActions: [EditActions.all]) { $item in
Button(item.text) {
editItem = item
}
}
.sheet(item: $addItem) { item in
EditorViewV2(item: item) { item in
items.append(item)
}
}
.sheet(item: $editItem) { item in
EditorViewV2(item: item) { item in
guard let index = items.firstIndex(where: { $0.id == item.id })
else { return }
items[index] = item
}
}
.toolbar {
EditButton()
Button("Add") {
addItem = ListItem()
}
}
}
}
}
struct EditorViewV2: View {
@State var item: ListItem
let onSave: (ListItem) -> Void
@Environment(\.dismiss) var dismiss
var body: some View {
NavigationStack {
Form {
TextField("Text", text: $item.text)
}
.toolbar {
ToolbarItem(placement: .cancellationAction) {
Button("Cancel", role: .cancel) {
dismiss()
}
}
ToolbarItem(placement: .primaryAction) {
Button("Save") {
dismiss()
onSave(item)
}
}
}
}
}
}
And to be honest, even this isn’t how we would like to do things. Having the separate -@State var addItem: ListItem?
-@State var editItem: ListItem?
+@State var destination: Destination?
+enum Destination {
+ case add(ListItem)
+ case edit(ListItem)
+}And then you can do this -.sheet(item: $addItem) { item in
+.sheet(item: $destination.addItem) { item in
EditorViewV2(item: item) { item in
items.append(item)
}
}
-.sheet(item: $editItem) { item in
+.sheet(item: $destination.editItem) { item in
EditorViewV2(item: item) { item in
guard let index = items.firstIndex(where: { $0.id == item.id })
else { return }
items[index] = item
}
} The only catch is that you have to use our SwiftUINavigation library because Swift does not allow dynamic member lookup via enum cases. We provide the tools to do that, and once you do that you now have compile time guarantees that only one single destination can ever be active at a time. We also don't like that one has to search for the index of an item in order to update it. However, this is necessary to avoid the crash that your code currently has. But we have another library that helps with this that allows us to write code like this: -guard let index = items.firstIndex(where: { $0.id == item.id })
-else { return }
-items[index] = item
+items[id: item.id] = itemWith those changes applied our code would actually be even shorter and more correct. And regarding this:
This is also not true. There is no such crash, and the I think at this point it would be very helpful if you could address the various feedback I have given throughout all of my posts in this thread. So far it has all been ignored. I’m just trying to understand what tangible problem you think the current code in the Reminders app has, and how your suggestions are actually fixing those problems. And I personally don’t think “It’s not done the ‘Apple’ way” is a problem. I’d rather see a description of an exact user-facing bug that is possible, or a way of changing the code to making it shorter, more succinct, and more correct. |
|
I addressed some of your feedback earlier. The tangible problem is what I said in my first post - a concern about the misuse of The sheet's closure is called again but the editor does not update with the changed item. I believe we should design for Regarding the |
We just don't consider it a misuse. It is well understood that one can only seed data to
This is true, but if one understands how But also, this problem is exactly why in our SwiftUINavigation library we provide a version of // sheet(item: Binding<Item?>) { (Binding<Item>) … }
.sheet(item: $editItem) { $item in
EditorViewV2(item: $item)
}This is another way to fix the problem, allowing the parent to make mutations to the
The main point I am trying to get across is that no matter what, if you have an imprecisely modeled domain you are open to these kinds of bugs. The only way to squash the bugs is to make the impossible states unrepresentable, which means embracing optionals to drive navigation. Not a boolean plus some additional state. Apple's sample code may use imprecisely modeled domains because it's easy, but I just don't think we should look to those samples as the perfect example of production code. The techniques they show are useful to get a project off the ground, but have a lot of downsides and I don't think it's appropriate to follow it in larger, more complex applications.
That sounds exactly how it is being used now. I have an optional piece of state with a |
|
I found a report by Stephen about a crash with I tried using Below is example I am happy with at the moment but I understand it doesn't model the domain as precisely as you would like. It has My fork is now well behind so I think we should just close this pull request now. |
FWIW, this crash has everything to do with the failable
If this is how you like to do things, then that is great! But we don't personally think it's worth maintaining that much extra code (now 90 lines vs our 60) to have an imprecisely modeled domain. You are still open to the possibility of both We hope in the future you will approach these kinds of conversations by asking questions and trying to get an understanding of why we have done things the way we have rather than assuming we are wrong and that we are using non-"standard" SwiftUI. |
|
It can be allowed because multiple
|
@malhal An error is logged to the console if both flip to
It certainly is feasible. We've demonstrated that.
That's not correct. |
Replaced all of
ReminderForm'sState(wrappedValue:) with aBinding. This made it possible to remove ReminderForm's custom init. AddedReminderFormConfigfor the form's data and logic. Change from.sheet(item:)which is for static content to.sheet(isPresented:)which is for dynamic. Replaced the Environment var dismiss with isPresented = false.There are other instances of
State(wrappedValue:)in the project which could also be replaced with Binding.