Skip to content
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

MultiEditorViewController: Reload the current editor during export process #141

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

twstokes
Copy link
Collaborator

@twstokes twstokes commented Jun 30, 2022

Description

This PR reloads the current editor when exporting from a MultiEditorViewController so that layers don't disappear if the user cancels the process. This currently affects WPiOS and appears to have been a problem in the past fixed by this PR. However, that fix doesn't seem to have the same effect anymore.

What this PR aims to do:

  • Only reload the currently selected (and visible, for WPiOS) editor rather than all exported editors
  • Reduce memory consumption and retention during the export process

⚠️ I'm unsure how this may affect the Tumblr or any other clients. Since the original fix was for WPiOS I'm inclined to think Tumblr won't be negatively affected, but I'm OK with implementing a more backwards-compatible fix if we think it's a risk.

Related issues:

Testing

  1. Run WPiOS from this PR
  2. Create a new Story post
  3. Add media from the device
  4. Add a text layer
  5. Tap the arrow at the top right to start the export / posting process
  6. Cancel the posting process by swiping the publishing sheet down
  7. Observe that the text from step 4 is retained
Before After
Before.MP4
After.mp4

@twstokes twstokes changed the base branch from main to fix/multi-export-crash June 30, 2022 14:52
@twstokes twstokes self-assigned this Jun 30, 2022
@twstokes twstokes added bug Something isn't working stories Related to the Stories project labels Jun 30, 2022
@twstokes twstokes added this to the 1.4.3 milestone Jun 30, 2022
@twstokes twstokes marked this pull request as ready for review June 30, 2022 17:01
@twstokes
Copy link
Collaborator Author

@afterxleep, @bjtitus here's the second change related to wordpress-mobile/WordPress-iOS#18967. If you could give this a look I'd appreciate it! 🙇

@bjtitus bjtitus self-requested a review June 30, 2022 19:38
Copy link
Collaborator

@bjtitus bjtitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and I think it is fine for the Tumblr app as well. I was hoping we might get the Buildkite integration in before merging this, but it should be good to go!

@twstokes
Copy link
Collaborator Author

twstokes commented Jul 2, 2022

This looks good to me and I think it is fine for the Tumblr app as well.

Thanks @bjtitus for checking it out! 🙇

I was hoping we might get the Buildkite integration in before merging this, but it should be good to go!

If tagging and pushing 1.4.3 (after #140 is reviewed) to CocoaPods complicates anything, I understand if you all want to get the Buildkite integration going first.

@twstokes twstokes force-pushed the fix/multi-export-crash branch from 4d715e0 to dd7b104 Compare July 6, 2022 13:32
@twstokes twstokes force-pushed the fix/reload-current-editor branch from c6c78b7 to b699ec6 Compare July 6, 2022 13:34
@twstokes twstokes merged commit 9dbb644 into fix/multi-export-crash Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stories Related to the Stories project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants