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

Yaru layout index controller #414

Merged
merged 12 commits into from Nov 29, 2022

Conversation

d-loose
Copy link
Member

@d-loose d-loose commented Nov 24, 2022

WIP
Adds YaruLayoutIndexController YaruPageController that controls

  • YaruMasterDetailPage
    • YaruLandscapeLayout
    • YaruPortraitLayout
  • YaruCompactLayout

Pull request checklist

  • This PR does not introduce visual changes, or
    • I ran flutter test --update-goldens and committed the changes if there were any, or
    • I added before/after/light/dark screenshots if the visual changes I made were not covered by golden tests.
      Before After
      Light
      Dark

Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming idea: YaruPageController 🙂

@d-loose
Copy link
Member Author

d-loose commented Nov 24, 2022

Naming things is always the hardest part :D

@jpnurmi
Copy link
Member

jpnurmi commented Nov 24, 2022

I wish YaruCompactLayout would be named something like YaruNavigationPage to be consistent with YaruMasterDetailPage and YaruNavigationRail but it would break the software app once again... 🤔

@Feichtmeier
Copy link
Member

Breaking software is not the problem
I just thought about a future where we might want to readd the layout builder and the bottom bar again
But then this could also be done inside the app so feel free to rename it ;P

@jpnurmi
Copy link
Member

jpnurmi commented Nov 25, 2022

Sorry for repeatedly hijacking random PRs. 🙂 Let's continue the discussion here:

@d-loose d-loose marked this pull request as ready for review November 25, 2022 09:14
lib/src/layouts/yaru_compact_layout.dart Outdated Show resolved Hide resolved
lib/src/layouts/yaru_master_detail_page.dart Show resolved Hide resolved
lib/src/layouts/yaru_compact_layout.dart Outdated Show resolved Hide resolved
lib/src/layouts/yaru_compact_layout.dart Show resolved Hide resolved
lib/src/layouts/yaru_master_detail_page.dart Show resolved Hide resolved
Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my head hearts with all these layouts, indexes, controllers, and listeners but i think it looks good now 👍

lib/src/layouts/yaru_compact_layout.dart Show resolved Hide resolved
lib/src/layouts/yaru_compact_layout.dart Outdated Show resolved Hide resolved
lib/src/layouts/yaru_compact_layout.dart Outdated Show resolved Hide resolved
lib/src/layouts/yaru_master_detail_page.dart Outdated Show resolved Hide resolved
Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that we always have a controller instance, we could let AnimatedBuilder take care of rebuilds:

AnimatedBuilder(
  animation: _pageController,
  builder: (context, child) => YaruFooBarPageOrLayout(
    index: _pageController.index
  ),
),

this way, we could get rid of the manual add/removeListener() and setState() calls.

Despite the name, AnimatedBuilder is not limited to Animations. Any subtype of Listenable (such as ChangeNotifier and ValueNotifier) can be used with an AnimatedBuilder to rebuild only certain parts of a widget when the Listenable notifies its listeners. This technique is a performance improvement that allows rebuilding only specific widgets leaving others untouched.

@d-loose
Copy link
Member Author

d-loose commented Nov 28, 2022

Do we want to keep YaruPortraitLayout and YaruLandscapeLayout available as stand-alone layout widgets with their own controllers? Using the AnimatedBuilder like this in YaruMasterDetailPage would make YaruPortraitLayout and YaruLandscapeLayout stateless and dependent on a parent widget that handles interactions.

@jpnurmi
Copy link
Member

jpnurmi commented Nov 28, 2022

We should be free to change YaruPortraitLayout and YaruLandscapeLayout as we wish. They are not exported in lib/yaru_widgets.dart because they are supposed to be internal implementation details. Do you think they would be valuable as public classes?

@d-loose
Copy link
Member Author

d-loose commented Nov 28, 2022

Maybe there are situations where you wouldn't want the layout to change, but I guess this can be controlled by setting the breakpoint to zero or infinity.
But I like the idea of moving all control to the MD page, it simplifies things a lot.

@jpnurmi
Copy link
Member

jpnurmi commented Nov 28, 2022

Is this good to go as it is now? We could leave using AnimatedBuilder and the consequent changes as a separate step.

@d-loose
Copy link
Member Author

d-loose commented Nov 28, 2022

Yes, should be good to go - let's do it in a separate PR. There are still some things to consider with the AnimatedBuilder (navigator in YaruPortraitLayout, YaruLandscapeLayout needs to be stateful to manage pane resizing)

Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good! LGTM 👍

Thanks for working on this and sorry it got more laborious than anticipated... :)

@d-loose
Copy link
Member Author

d-loose commented Nov 28, 2022

Better to do it right than deal with annoying issues later - thanks again for the feedback!

@Feichtmeier Feichtmeier merged commit 61496f4 into ubuntu:main Nov 29, 2022
@jpnurmi jpnurmi mentioned this pull request Jan 11, 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.

None yet

3 participants