Make sure post has the latest content before uploading. #6036
Conversation
There was a problem hiding this comment.
Hola @SergioEstevao. Just had a couple of thoughts to share.
|
|
||
| //Make sure that we are uploading the latest content on the editor. | ||
| self.post.postTitle = self.titleText; | ||
| self.post.content = self.bodyText; |
There was a problem hiding this comment.
Should we also call [self.post save]; to ensure that the change is persisted to core data, or maybe just call [self autosaveContent] here which does the same work and saves to core data?
|
|
||
| //Make sure that we are uploading the latest content on the editor. | ||
| self.post.postTitle = self.titleText; | ||
| self.post.content = self.bodyText; |
There was a problem hiding this comment.
Also wondering if autosaveContent should instead/also be called from the success block of uploadMedia:trackingId: or from the individual "replace" methods. Just thinking about any other potential ways that race condition might present itself.
There was a problem hiding this comment.
@aerych autoSaveContent is already being called when the image finished upload, and the race condition we see is between the save happening to CoreData and being reflected up to the self.post instance and the upload to the server.
The autosave is done in async way and I think that is the source of our problems. But we cannot change it to sync, because the method is being called for almost every character changed...
There was a problem hiding this comment.
autoSaveContent is already being called when the image finished upload, and the race condition we see is between the save happening to CoreData and being reflected up to the self.post instance and the upload to the server.
That doesn't sound quite right. autosaveContent also updates self.post.postTitle and self.post.content, just like the change in this PR. If the race condition was between the save happening to CoreData and being reflected up to the self.post instance and the upload just updating self.post.content and self.post.postTitle in savePost shouldn't solve the race condition.
The autosave is done in async way
The method appears to be called syncronously on the main thread in respose to WPEditorView webView:shouldStartLoadWithRequest:navigationType:. That guy is fired by an NSInvocation so maybe there is something async happening there. But I think maybe you mean the context is saved asyncronously via [self.post save] (derived contexts all all that). Regardless, I'm not seeing where core data or directly calling autosave is a factor. We should be able to call autosaveContent directly without concern.
I think this patch is right in that we just need to ensure self.post.content and self.post.postTitle are promptly updated. However, the work is already being done in the call to autosaveContent so maybe it would be better to call that method immediately (since it appears to not be), and in more places, rather than duplicate some of the work its already doing in another method.
| @@ -1413,8 +1413,7 @@ - (void)savePost | |||
| [self.view endEditing:YES]; | |||
|
|
|||
| //Make sure that we are uploading the latest content on the editor. | |||
| self.post.postTitle = self.titleText; | |||
| self.post.content = self.bodyText; | |||
| [self autosaveContent]; | |||
There was a problem hiding this comment.
So I dug into this a bit to try and figure out why we were seeing the post title and content cleared.
I believe the reason is that savePostAndDismissVC calls stopEditing before it calls savePost and the call to stopEditing is clearing out the internally stored post title and content.
I think we could move the call to autosaveContent from savePost to savePostAndDismissVC before the call to stopEditing and it would fix this particular issue.
Thoughts?
There was a problem hiding this comment.
@aerych good find I will do a bit more testing on this.
|
@aerych Tested your solution and it looks it work great. Do you want to do a last test before I merged this in? |
aerych
dismissed
their stale review
Its my review and I can dismiss it if I want :P Nyah!
Could we just retarget this one? Github finally let's us do that now. |
when ready as far as I'm concerned :)
|
Close this one in favour of #6082 for the 6.6 release branch. |
Fixes #5923
Found a race condition between the images source being saved to the post, and the possibility of the user save the post. If tapped to quickly sometimes the post was upload to server with the content before the images sources was set.
To test:
Needs review: @aerych