-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Replaces bundle usage in the editor with a local database #19747
Conversation
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
Thank you for putting the |
Thank you for testing and your feedback @justtwago 🙇 I also experimented with removing views from the Bundle. Since you are able to reproduce this consistently could you give the patch below a try? 🙇
|
Yeah, totally makes sense. I think that it's too extreme case that even crashes the app when I'm trying to put everything to a clipboard. 😄 I think that we can proceed with putting a revision to a local DB and monitoring the results on Sentry.
Thank you for the idea! I tried it out, and it looks like the app doesn't crash 😄 Anyway, I'm not sure that it's something that we want to have in the app since when the app destroys the activity in the background we can't retrieve the views when are back to the app so it causes a crash (it happens for me once but I couldn't for this and reproduce it later to catch a stack trace.) |
One more question is about putting/extracting a revision to/from DB:
GitHub doesn't allow me to comment this block of code 😄 |
Great suggestion @justtwago 👍 I added the rest of the revision instances in the db with 3b61f82 |
This makes sense to me too @justtwago 👍 I prepared this PR for review to at least cover the data we add to the Bundle. |
Generated by 🚫 dangerJS |
Generated by 🚫 Danger |
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.
Antonis, thank you for the amazing work on the fix!
I smoke-tested the editor and also used the Don't keep activities
flag, and the DB seems to work perfectly. Fingers crossed for seeing some progress in Sentry after releasing it.
I left some minor comments for cosmetics stuff, but I'd merge it today to include the fix in the nearest release.
In addition, we could consider extracting the persistence logic to a separate gradle module in the future.
libs/editor/src/main/java/org/wordpress/android/editor/savedinstance/SavedInstanceDatabase.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/history/HistoryDetailFragment.kt
Show resolved
Hide resolved
Thank you for testing extensively and your feedback @justtwago 🙇
That indeed a good idea. I was on the edge of adding this but thought that it would extend the scope of the change and preferred to use the |
Yeah, totally agree that it's out of this PR's scope. 💯 |
Partially #9685 and #19157
Description
This PR tries to mitigate the
TransactionTooLargeException
error in the editor by replacingBundle
forParcelable
objects with a custom database implementation.To Test:
Regression Notes
Potential unintended areas of impact
Editor
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
Added tests for the added local db
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.UI Changes Testing Checklist: