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

Support for a scrollable LocalHero #17

Merged
merged 5 commits into from
Nov 19, 2023

Conversation

Snonky
Copy link
Contributor

@Snonky Snonky commented Oct 2, 2023

Issue #1 has a few requests for a LocalHero that works under a scrollable widget.

I think I found a working solution that should give LocalHeros the ability to skip their animation when scrolling.

It adds the onlyAnimateRemount parameter to the LocalHeroScope constructor.
Set it to true and the LocalHeros in this scope wont animate unless their widget in the widget tree and their position on screen changed.

This will at least work with SingleChildScrollView scrollables because these don't do tree surgery.

Before & after videos

Before:
local_hero_before

After:
local_hero_after

to LocalHeroScope.
It makes the LocalHero animation skip all position changes except when
caused by a remount. A remount is considered a reparent or a slot change
inside a multi-child parent.
The LocalHero now animates due to any change in its Widget.
This still filters out scrolling but enables e.g. resizing.
@mimoislam
Copy link

mimoislam commented Oct 31, 2023

There is error 'package:flutter/src/rendering/object.dart': Failed assertion: line 2597 pos 12: '!_debugDisposed': is not true.
same error but at the same time I have another thing when I use LocalHero without Key it doesn't work is that problem or bug or it is necesseity
'package:flutter/src/rendering/layer.dart': Failed assertion: line 2406 pos 16: '_debugPreviousLeaders!.isEmpty': is not true.
this is the error that is showing

@Snonky
Copy link
Contributor Author

Snonky commented Oct 31, 2023

The first error seems related to issue #5. I also have a PR for that #16.

In my forked branch feature/only-remount I have merged both this and the other PR. See #1 (comment).

when I use LocalHero without Key it doesn't work is that problem or bug or it is necesseity

Yes, you have to use a key on each local hero. Without the key, the change in position cannot be tracked.

@letsar
Copy link
Owner

letsar commented Nov 17, 2023

Thanks for this PR! I tried it by surrounding _DraggableExample with

return SingleChildScrollView(
      child: SizedBox(
        height: 2000,

I saw at least two issues, one is performance : the scroll does not seem smooth. The other one is as we scroll, the widgets are painted above the scaffold and this if we scroll too far, the are painted above the appBar.
With this PR you are maybe in the right direction though, thank you!

@Snonky
Copy link
Contributor Author

Snonky commented Nov 17, 2023

The first issue I did not notice because I only used a Windows PC with a GPU where I get 144 FPS. What device did you use?

For the second issue you can try wrapping the SingleChildScrollView with your LocalHeroOverlay. That should clip the overflowing widgets.

@letsar
Copy link
Owner

letsar commented Nov 18, 2023

My bad, I woke up today thinking about your PR and I remembered that I completely forgot to set onlyAnimateRemount to true 😓 . Sorry for that. It works really great after this 🚀 .
I need to look at the code but this is a really great idea, thanks!
Do you see any reason to not make onlyAnimateRemount true by default?

@Snonky
Copy link
Contributor Author

Snonky commented Nov 18, 2023

I completely forgot to set onlyAnimateRemount to true 😓 . Sorry for that.

Don't worry about it, that also happened to me multiple times during testing! 😅

Do you see any reason to not make onlyAnimateRemount true by default?

Mainly backwards compatibility. But you are right this is unlikely to break any intended functionality. If there will be a short migration guide I agree with you, it can be true by default.

The second smaller reason is ListView. It doesn't work out of the box with LocalHero because the items that are scrolled away are first cached and then disposed. This is good for performance but it breaks the local hero tracking. The cached items (those that are just a few pixels out of the scroll view) are always layed out but not painted. But since the LocalHeroController gets updates about the widget position via the paint call it goes out of sync with the layout position.

I already have a solution for this that I will post as a follow-up PR when we reviewed this one: A CustomScrollView with a SliverLocalHeroList that behaves just like a normal SliverList (this is what ListView uses) but it paints its LocalHero children even when they are in the cache region. Then it works with minimal performance penalty.

@letsar
Copy link
Owner

letsar commented Nov 18, 2023

Yeah we cam bump the version to 0.3.0 to indicate there is a possible breaking change. Can you resolve the conflict so that we can merge this PR?

For the issue with slivers, can you post an example where it does not work? I made a simple test with a ListView but I don't see any issue for the moment if I wrap the item with a StatefulWidget having its State applying the AutomaticKeepAliveClientMixin.

@Snonky
Copy link
Contributor Author

Snonky commented Nov 18, 2023

Can you resolve the conflict ✔

I added a new tab to the example app. To see the issue do the following:

  1. Scroll all the way down and then all the way back up.
  2. Press the 'Add Item' button

You will see a one frame glitch where the items are in the wrong position.

Another problem comes up when pressing 'Add Item'. The last visible item does not get animated off the screen, it just disappears instantly.

If your idea with the keepalive fixes this let me know. I'm not sure how to test it myself-

@letsar
Copy link
Owner

letsar commented Nov 19, 2023

Oh ok I see the issue thanks, and no AutomaticKeepAliveClientMixin won't help with that.
Thanks for this PR by the way, this is a great feature!

@letsar letsar merged commit aff8cfd into letsar:master Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants