-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for editing comments #24093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Generated by 🚫 Danger |
46e049b to
f129705
Compare
|
| App Name | WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24093-db430bb | |
| Version | 25.7.1 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | db430bb | |
| App Center Build | WPiOS - One-Offs #11528 |
|
| App Name | Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24093-db430bb | |
| Version | 25.7.1 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | db430bb | |
| App Center Build | jetpack-installable-builds #10560 |
f129705 to
dceffdf
Compare
| let commentID = comment.commentID as NSNumber | ||
| let service = CommentService(coreDataStack: ContextManager.shared) | ||
|
|
||
| let remoteComment = try await withUnsafeThrowingContinuation { continuation in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the benefits of using withUnsafeThrowingContinuation vs withCheckedThrowingContinuation in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I've used withCheckedThrowingContinuation before because AFAIU it throws a fatalError if there you don't adhere to one of its rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On that aspect, I guess the "checked" version fails early, but who knows what it means if we are not "being extra careful".
Unlike the “checked” continuation variant, the UnsafeContinuation does not detect or diagnose any kind of misuse, so you need to be extra careful to avoid calling resume twice or forgetting to call resume before letting go of the continuation object.


This PR adds support for editing existing comments from Reader, including Gutenberg comments.
Instead of a single
CommentComposerViewController, I created separate classes to make sure it's easy to follow what's going on. There were too many small differences to keep it in a single file.CommentCreateViewController,CommentCreateViewModelfor creating new commentsCommentEditViewController,CommentEditViewModelfor editing existing commentsCommentEditorViewControllerfor hiding the complexity of dealing with plain text and Gutenberg commentsRelated PR: wordpress-mobile/WordPressKit-iOS#830
Changes
CommentEditViewController(recording: plain text, blocks)Fixes
textis now anasyncpropertyKnown Issues
Testing
Note: enable "Gutenberg Comment Composer" for testing
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: