-
Notifications
You must be signed in to change notification settings - Fork 4.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
[RN Mobile] RichText: Return early when content has changed in onSelectionChangeFromAztec #41682
[RN Mobile] RichText: Return early when content has changed in onSelectionChangeFromAztec #41682
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @kbrown9! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
b8a59b8
to
5bb4a1b
Compare
…ctionChangeFromAztec
5bb4a1b
to
2c30dd9
Compare
Thank you for creating those builds! I had no idea where to start with that. I'll plan to install the Android build on my devices for testing. Unfortunately, I don't have any iOS devices to use for testing. As you can see from the code changes, I added a check for the last event type. The mobile unit tests caught a problem when changing the color of a text selection, and checking the last event type solved that problem. |
No worries! Just in case you're interested in playing more with Mobile Gutenberg in the future, we have some documentation (that's a bit hidden away) to help with creating installable builds for Android here and iOS here.
Perfect, if you're able to go through the checklist for Android then I'll go through it for iOS. 🟢 Writing Flow checklist (iOS)Build: pr18873-f758713 General
Rich Text Format
Splitting and merging
Undo / Redo - Test Cases |
Everything seemed to work well for me. I encountered a few of the known issues, but I did not notice any new ones. 🟢 Writing Flow checklistBuild: wordpress-mobile/WordPress-Android#16742 (comment) General
Rich Text Format
Splitting and merging
Undo / Redo - Test Cases |
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.
@kbrown9, these changes look great to me!
To measure this, I used Android's dumpys tool for measuring performance and followed these steps in an emulator:
- Copied/pasted the content from Automattic's How We Work page to a new post. This was in order to get a fair sized post for testing.
- Opened the post and ran
adb shell dumpsys gfxinfo org.wordpress.android.prealpha reset
from Android Studio's terminal, in order to reset any existing data. - Added three new paragraph blocks to the post, making sure to add multiple lines of content to one.
- I ran
adb shell dumpsys gfxinfo org.wordpress.android.prealpha
to gather data from the terminal. - I repeated the above against
trunk
and then with the changes from this PR applied. I was mindful to add similar types of content when testing both the "before" and "after".
Here's an overview of the time it took to render frames at the 50th, 90th, 95th, and 99th percentiles (keeping in mind the desired end goal is to be ≤ 16.67ms in order to get 60 frames per second):
-- | Before Fix | After Fix |
---|---|---|
Time to render 50th percentile | 6ms | 5ms |
Time to render 90th percentile | 44ms | 26ms |
Time to render 95th percentile | 93ms | 42ms |
Time to render 99th percentile | 350ms | 121ms |
The above indicates improvements of 50%+ for the higher percentiles. Anecdotally, I also felt that things seemed faster when experimenting with adding various blocks and writing content in posts. 😍
I plan to ask for another set of eyes on this PR in order to double-check that the methodology I used for testing seems sound, so will hold off on approving for now, but I think this seems like it will be a really great improvement!
It'd be a good idea to add this change to the changelog in order to make testers aware of it for beta testing too.
Hey there 👋 these changes look good to me ✅, I manually tested using a Redmi Note 8T and I can see a nice improvement in performance, especially when typing fast, comparing it with the production version. I did several tests with formatting, switching between blocks, and autocorrecting words. I think we should add a comment with the code we are adding, just to have a reference of the problem is solving. What do you think @kbrown9? |
Thanks for the tests and review! I agree that a comment with the code makes sense, and I'll also add this change to the changelog. |
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.
Congratulations on your first merged pull request, @kbrown9! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
What?
If the component's content has changed, return early in the
RichText::onSelectionChangedFromAztec
method.Why?
There have been reports of slow performance in the RichText component on older and lower-end devices. One potential cause of the slow performance is the amount of work that happens with each keypress in the
RichText::onSelectionChangeFromAztec
method.Issue: wordpress-mobile/gutenberg-mobile#2594
How?
It seems unnecessary to do anything in the
RichText::onSelectionChangeFromAztec
method while the user is currently typing quickly. It appears that theRichText:onChangeFromAztec
method is only called at the beginning and end of a typing spree. Therefore, a potential improvement is to only update the selection whenRichText::onChangeFromAztec
has been called. We can determine whetherRichText::onChangeFromAztec
has been called by comparing the event's content to the component's current content and checking the last event type. If they are different and the last event type was 'selection change',RichText::onChangeFromAztec
was not called. (If it had been called, it would have set the component's content to the event's content here).I expect this change to significantly decrease the lag experienced when creating a new block after typing. I also expect this change to decrease the lag experienced when a block using the RichText component expands to create a new line after a significant amount of typing.
Testing Instructions
The writing flows will need to be tested thoroughly to verify that this change doesn't cause any regressions.
Screenshots or screencast
n/a