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

fix: reduce amount of layout calls and debounce layouts when needed #10164

Merged
merged 11 commits into from
Mar 17, 2023

Conversation

edusperoni
Copy link
Contributor

PR Checklist

What is the current behavior?

We do a layout call on a lot of places where the views might not be fully updated yet

What is the new behavior?

We now schedule a layout call on viewSafeAreaInsetsDidChange so it only triggers if viewDidLayoutSubviews.

We also prevent a layout call if owner.nativeViewProtected.layer.needsLayout() is true, meaning that viewDidLayoutSubviews will be called again and if we run a layout call we will do a layout with the wrong parameters.

cc @CatchABus @farfromrefug

@nx-cloud
Copy link

nx-cloud bot commented Jan 9, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b887399. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@cla-bot cla-bot bot added the cla: yes label Jan 9, 2023
@NathanWalker NathanWalker added this to the 8.5 milestone Jan 9, 2023
@CatchABus
Copy link
Contributor

Perhaps, we could create a view controller base class and add all runningLayout methods and properties there in order to avoid duplicate code. Then, we can make controllers inherit from it and use all common stuff.

@farfromrefug
Copy link
Collaborator

@edusperoni I am a bit worried about that kind of "patch". I fill like we are patching just because we actually dont know the real cause of the inner issue.
If i am not wrong this all thing started with large titles not laying out correctly with listview. If so i feel like we should remove that patch and go back to the initial issue. If that patch was needed i feel like native apps would need this too.
I should mention that your patch point one thing which has always bugged me with N. I dont like the fact that we layout our views from viewDidLayoutSubviews. Not sure it has anything to do with the inner issue, but i want to point out that maybe something is wrong with our way of handling UIViewController lifecycle.

@edusperoni
Copy link
Contributor Author

I believe this issue became more apparent on iOS 16, where the viewDidLayoutSubviews isn't called sometimes when only the safe area changes. The timeout is actually a performance optimization to reduce layout calls and prevent layout changing during animations (which would break some transitions).

Maybe we should review the layout process, but this PR fixes both the issues with side drawer and incorrectly displayed items and large titles/changing safe area sizes.

@farfromrefug
Copy link
Collaborator

Did you find any reference to that ios 16 anywhere ? Like is any one doing native facing this?
As for this fixing in the meantime, it also can bad side effects I many places. It is a dirty hack which a high possibly of side effects.
As for the side drawer issue. Wasn't it a side effect of your first PR?

@edusperoni
Copy link
Contributor Author

My bad, it was 15. I haven't found anything about native apps when I researched it so I figured it was specific to our layout engine

@farfromrefug
Copy link
Collaborator

@edusperoni i understand we need the fix. But wouldn't it be best to revert your original fix and investigate the issue with large titles a bit more? Can you confirm the issue with side drawer was happening or not before your original PR?

@edusperoni
Copy link
Contributor Author

The side drawer issue didn't happen before due to the .layoutIfNeeded() call that I removed in this PR.

The way to reproduce the large title issue is to just have a page with large titles and a list view inside. The large title should bug out (and if you wrap the list view in a grid and put view parallel to the list the views won't change position if the title change sizes)

@CatchABus
Copy link
Contributor

CatchABus commented Jan 18, 2023

@farfromrefug Regarding the drawer issue, Eduardo has created this sample: https://github.com/edusperoni/sidedrawer-rotate-issue

Remove layoutIfNeeded call from your core package and reproduce the issue with the following steps:

  1. Open drawer
  2. Rotate device to landscape
  3. Rotate device back to portrait

Note: Perform rotations while drawer is open.
Result: Drawer children layout is messed up.

To fix this in the current layout engine, we ended up with the following cases after removing layoutIfNeeded call:

  • Nesting existing layout call inside a setTimeout in viewDidLayoutSubviews
  • Calling layoutIfNeeded in viewDidLayoutSubviews
  • Add a needsLayout check in viewDidLayoutSubviews

I left those notes in case they help more on research.

@farfromrefug
Copy link
Collaborator

@edusperoni @CatchABus sorry for not replying sooner. I will look at the repo with the drawer issue.
@edusperoni do we have a repo example of the original issue with big titles?

@farfromrefug
Copy link
Collaborator

farfromrefug commented Feb 27, 2023

@edusperoni @CatchABus actually always thought the drawer issue was with my drawer plugin. But it was with rad drawer?
If that i could test to see if that original bug (without the layoutIfNeeded) is happening with my plugin. If it does not i dont think it is N related!

EDIT: @edusperoni i tested without layoutIfNeeded with my drawer plugin and i see no error with layout on orientation changed. Could this be an issue in the rad drawer plugin?

I maintain we should remove all those hacks and fix issues one by one the right way

NathanWalker
NathanWalker previously approved these changes Mar 17, 2023
@farfromrefug
Copy link
Collaborator

@NathanWalker I see you are about to merge this. I think we should not merge hacks like this (especially when the issue seems to be in a plugin?). The more we add the harder it will become.
At least you should.add comments on what and why those hacks are here.

@NathanWalker NathanWalker merged commit 8b721c1 into main Mar 17, 2023
@NathanWalker NathanWalker deleted the fix/safe-area-layout-fixes branch March 17, 2023 17:07
farfromrefug added a commit to Akylas/NativeScript that referenced this pull request Mar 23, 2023
NathanWalker added a commit that referenced this pull request Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants