Conversation
|
Thanks to @nicksloan for the idea of watching |
viktorstrate
left a comment
There was a problem hiding this comment.
Thank you very much @scoates for all your work. I've added some comments on the use of async, if you have any ideas or comments don't hesitate to say so.
| @Binding var height: CGFloat? | ||
| @AppStorage("fontSize") var fontSize: Int = 13 | ||
|
|
||
| @State private var isLoaded: Bool = false |
There was a problem hiding this comment.
Can we rename this to isDraftLoaded I was very confused reading this to begin with, because I though it was the talking about the timeline 😅
|
|
||
| private func saveDraft() { | ||
| if chatInput.isEmpty { | ||
| clearDraft() |
There was a problem hiding this comment.
Can we make clearDraft, saveDraft, and chatInputChanged async functions, and move the Task invocations to the call site of these functions?
| .onChange(of: chatInput) { | ||
| chatInputChanged() | ||
| } | ||
| .onChange(of: replyTo != nil) { | ||
| chatInputChanged() | ||
| } | ||
| .onChange(of: timeline.timeline != nil) { _, timelinePopulated in | ||
| Task { |
There was a problem hiding this comment.
If chatInputChanged became an async function (as mentioned above), it might make sense to use the .task(id:) view modifier instead of onChange(of:) to get cancellation if the value is changed before the async function finishes.
I suppose right now we can end up spawning multiple instances of loadDraft that run concurrently which could make the UI end up showing a wrong draft.
|
Good feedback. Thanks! I'll make some changes (and look into the |
|
Perfect thank you. Yes I know some of my code is also not perfect but I think it's a good idea to slowly improve it throughout the project, now where the general architecture is a little more settled. |
|
(To be clear: I wasn't complaining about your code, and no one expects perfection (-: Just wanted to explain the reason I chose to do it that way, and I appreciate the call-out + permission to do it better.) |
…back to a room (or thread)
- tidy up async/Task - use the `.task(id:)` pattern (instead of `.onChange(of:)`) - rename to isDraftLoaded - better state control
a60aa96 to
bf9f366
Compare
|
I made the requested changes and rebased onto main to pull in the recent changes. It's much cleaner now, I think. Good call on the |
|
Looks great now, thank you! |
Saves and loads a draft of unsent room (and thread) messages.
Had to do a bit of restructuring in
LiveTimelineto capture the focused thread ID (if there's a better way to get this, I'm game, but it wasn't obvious if it's possible).(Also removed a stray old
print.)Replies are attached and restored. This was the most difficult part.
Thanks for pointing out the Matrix SDK facility for this. It's perfect and helped guide the implementation.
Closes #31.