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

Jump to bottom

fix: requestlayout improvements #9122

Merged
merged 7 commits into from Aug 11, 2021
Merged

fix: requestlayout improvements #9122

merged 7 commits into from Aug 11, 2021

Conversation

@farfromrefug
Copy link
Collaborator

@farfromrefug farfromrefug commented Dec 30, 2020

This is a first pass at improving requestLayout. The biggest improvements are seen on iOS.

  • during applyAllNativeSetters or applyPendingNativeSetters prevent requestLayout calls on every property change. Instead call requestLayout if needed at the end. This is a big change on iOS as most properties are marked as affecting layout.
  • on top of that dont even call requestLayout at all if within onLoaded. it is not necessary as the parent with layout itself and thus layout all its children.

With that change, the colors fixes and the android background color change simplification, i see a 5-10% shorter loading time on android. Tested it with different apps of different complexity.
@farfromrefug
fix: first pass at improving requestLayout
80e93b7 
It is totally unecessary to call multiple requestLayout inside initNativeView (properties set)
Even worse we dont need to do any requestLayout from “onLoaded” as the parent will layout itself/children
@farfromrefug
missing getter
c36d633
@NathanaelA
Copy link
Member

@NathanaelA NathanaelA commented Dec 30, 2020

This looks awesome. Thanks -- did you run it against the app testing harness?

@farfromrefug
Copy link
Collaborator Author

@farfromrefug farfromrefug commented Dec 30, 2020

Which app are you referring to?

@NathanWalker
Copy link
Contributor

@NathanWalker NathanWalker commented Dec 30, 2020

@farfromrefug npm start > type automated.ios (and you can also run automated.android) to kick them off locally.

@farfromrefug
Copy link
Collaborator Author

@farfromrefug farfromrefug commented Dec 31, 2020

@NathanWalker thanks was not sure @NathanaelA was refering to another one.

NathanWalker
NathanWalker reviewed Jan 29, 2021
View changes
packages/core/ui/core/view-base/index.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view/index.ios.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view/index.android.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view/index.android.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view/index.ios.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view/index.ios.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view-base/index.ts Outdated Show resolved Hide resolved
packages/core/ui/core/view/index.ios.ts Outdated Show resolved Hide resolved
@cla-bot cla-bot bot added the cla: yes label Feb 1, 2021
@farfromrefug
Copy link
Collaborator Author

@farfromrefug farfromrefug commented Feb 1, 2021

@rigor789 should be all good

rigor789
rigor789 reviewed Feb 1, 2021
View changes
Copy link
Member

@rigor789 rigor789 left a comment

LGTM

@rigor789 rigor789 dismissed NathanWalker’s stale review Feb 1, 2021

has been fixed

@NathanWalker
Copy link
Contributor

@NathanWalker NathanWalker commented Jul 12, 2021

@farfromrefug I'm curious if your fork may have a different setting than most? I've noticed that on all your PR's we can not update the PR with base automatically on github however for all other pr's we are able to. Is there a setting on your fork that may be preventing that? We could help get quite a few of your pr's merged if we can update them with latest master automatically here.

@NathanWalker NathanWalker added this to the 8.1 milestone Jul 12, 2021
@farfromrefug
Copy link
Collaborator Author

@farfromrefug farfromrefug commented Jul 12, 2021

@NathanWalker you mean on my PR s you font have the right to update? I always create PRs from github website but I have to say I almost never ensure the checkbox for collaboration is checked or not. though it should be by default

@NathanWalker NathanWalker changed the base branch from master to release/8.1.0 Aug 11, 2021
@NathanWalker NathanWalker merged commit e4ce17e into NativeScript:release/8.1.0 Aug 11, 2021
2 of 3 checks passed
NathanWalker added a commit that referenced this issue Sep 8, 2021
@farfromrefug @NathanWalker
feat: requestLayout performance improvements (#9122)
4f5f0aa 
It was unnecessary to make multiple calls requestLayout inside initNativeView (properties set)
NathanWalker added a commit that referenced this issue Sep 10, 2021
@NathanWalker
revert: feat: requestLayout performance improvements (#9122)
eee2613 
This reverts commit 4f5f0aa.
rigor789 added a commit that referenced this issue Sep 10, 2021
@NathanWalker @rigor789
revert: feat: requestLayout performance improvements (#9122)
eb0d000 
This reverts commit 4f5f0aa.

This commit breaks back-navigation in certain cases, most prominently with Button pseudo classes. We plan to revisit this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanWalker NathanWalker

@rigor789 rigor789

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
8.1
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@farfromrefug @NathanaelA @NathanWalker @rigor789