[Feature] Nested between delegates and providers make implementations more complex #237

cyanlink · 2022-01-27T09:01:34Z

cyanlink
Jan 27, 2022

首先感谢作者为我们提供如此强大的文件选择库（Flutter上其他的库都太不好用了吧！）。
First thank you all for building us such a powerful asset-picker library! All other libraries are literally toys compared to this one.

但是，在我尝试避免造轮子、在本插件的基础上写逻辑的时候遇到了困难：Delegate和Provider之间是耦合的（Delegate的构造函数接受且只接受一个Provider，而一个Provider只维护了一个_currentAssets，这限定死了一对一关系）。
However I encountered problems while trying to reuse the codebase and implement a custom file picker: the Delegate and Providers are tightly coupled. (the constructor of a AssetPickerBuilderDelegate takes one and only one provider, and a provider maintains only one _currentAssets, limiting it to a one-on-one relation).

我要造的轮子就是模仿小红书的文件选择页面：
I was trying to imitate the following UI (Switch asset type by Tabs).

这个根据Tab切换所选文件类型的功能，只是通过继承Delegate、Provider类是无法较好实现的：如果通过先全部加载，再在UI逻辑里条件筛选的方式，可能会有数据显示不全，而且影响加载更多的交互等逻辑。
This feature cannot be implemented by extending delegate and provider classes. If I try the hack way which is loading AssetType.common first then filter it in UI logics (I tried), it would cause incomplete file result, and mess up with the interactions/load more logics.

一个可能的实现是，一个Tab对应一个Provider，而Provider在实例化的时候配置不同的类型。但是现有的Delegate模型不允许我这么做。
A possible implementation being, one Tab corresponds to one Provider(or data source, since we logically can't have multiple Provider the Widget of same Type in the hierarchy), and config them with different RequestTypes on instantiating. But the current model of delegate won't allow me to do so.

实际上，父级抽象类 AssetPickerBuilderDelegate 对provider的依赖是完全可以消除的，仅有的两个逻辑，要么可以从AssetPicker那几个静态方法从上层注入Provider，要么可以交给子类实现。
IMO the dependency of a provider in the parent abstract class AssetPickerBuilderDelegate can be eliminated! the only existing 2 uses can be eliminated by injecting it from AssetPicker#pickAssetsWithDelegates or leave it to the child class to implement it.

我们可以先讨论一下这个问题可能的解决，如果有时间我近期会提个PR！（这可能引入Breaking changes，虽然如果不自定义可能不用更改）
We can discuss for a possible solution first, maybe I'll PR in a short time! (Might introduce breaking changes, although no modification is needed if you don't do customization)

cyanlink · 2022-01-27T09:12:33Z

cyanlink
Jan 27, 2022
Author

或者可以这么说，Provider和Delegate本身实现逻辑上就是具有强关联的，因此我们更加不能去限制它们之间的组合关系：因为这完全应当是由继承实现类来决定的，抽象的Delegate基类不该持有Provider。当然这涉及到Flutter Provider库本身的局限（由于动态性，需要“飞线”，无法静态定位）。
In other words, in business logic a Provider (the ChangeNotifier, ViewModel) is strongly related with a Delegate(the UI logic). That's why we should not limit the compositing of said classes: it should be implemented by the extending classes, and an abstract base Delegate should not hold a reference of provider the view model . Of course there will be trouble caused by limitations of Flutter Provider library (search parent widget at runtime, can't statically locate where the parent is)

0 replies

AlexV525 · 2022-01-27T09:18:49Z

AlexV525
Jan 27, 2022
Maintainer

我个人基本认同你的观点，而且这部分因为没有严格的去做分层架构，所以只是提供了点到点的重载。如果可以的话，我也希望能整理一套完善、美观且灵活的架构。

有时间的话，希望你可以帮忙做一份架构模拟设计，我们深入讨论一下怎么把它变得好用(正在回家路上)

Converting this as a discussion according to contents related with this issue.

0 replies

cyanlink · 2022-01-28T08:43:44Z

cyanlink
Jan 28, 2022
Author

今天在我的Fork里做了初步修改，糊出来一个初步实现。发现最关键的还是对AssetPickerProvider这个抽象基类进行关注点分离，里面存在例如isSwitchingPath这种和UI有关的逻辑。目前是靠一个ProviderAggregate类进行一个全局封装。

0 replies

AlexV525 · 2022-02-05T10:17:15Z

AlexV525
Feb 5, 2022
Maintainer

我将在 #241 中进行调整，目前已经将 provider 从抽象 delegate 中拆分，并且优化了一些方法的耦合情况。可以订阅 PR 并且关注实现情况，及时沟通。

0 replies

AlexV525 · 2022-02-10T08:34:25Z

AlexV525
Feb 10, 2022
Maintainer

已合并至 34f133f，并且在 example 中添加了一个示例。

0 replies

cyanlink · 2022-02-18T10:40:17Z

cyanlink
Feb 18, 2022
Author

More on this topic:
I tried out your multi-tab implementation, you used the "triple provider, inject to each tab" method similar to my previous solution in my fork. but there is a serious drawback: initial loading time nearly tripled. (Release build) when single provider exists, it takes approximately 5 sec to load the list, but now it takes 15 sec avg. for the loading indicator to spin.
IMO a more flexible approach would still be using one single provider that loads all the files (maybe RequestType.common), but we now use filters to provide filtered "views of data" to each tab. I'm still working on it tho.

On the UI side, I find the delegate pattern of customizing UI getting bloated and rigid. We should start extracting codes, apply the rule of "Composition over Inheritance" , and we build the whole thing by compositing UI pieces into just a StatefulWidget, not filling/implementing an inert framework delegate. I would really want to reuse the existing UI pieces and loading/thumbnail logics because I don't want to reinvent the wheel.

To make a TabView you will need Vsync, which was not provided (not mixed in) in the top-level State before, so you monkey-patched it. What if the next time someone need some other mixin on State? or I have to introduce an intermediary StatefulWidget mixin SomeMixin then pass the Delegate as param which is god awful coding hell.

All an UI-developer need is the injected-provider and handy building blocks of the UI, functions like gridBuilder, itemBuilder, appBar, bottomAppBar and buttons. We should not be passing a delegate, instead devs should simply pass a Stateful/less Widget Page and we devs guarantee it works with our custom providers.

I will spend some time on revamping it with my own ideas (this weekend), that's going to introduce a lot of big changes so I won't PR early on.

Off-topic: That's why I really hate Java, turned to Flutter and never really learnt Android Native, and started learning Go recently.

2 replies

AlexV525 Feb 18, 2022
Maintainer

(Release build) when single provider exists, it takes approximately 5 sec to load the list, but now it takes 15 sec avg. for the loading indicator to spin. IMO a more flexible approach would still be using one single provider that loads all the files (maybe RequestType.common), but we now use filters to provide filtered "views of data" to each tab. I'm still working on it tho.

This is an example, not a best practise. The point is the ability to integrate. You sure can use assets filter from a single data source, but what about pagination, path filtering and assets counts?

You might be right, things can't be in a right path immediately. But a flexible intergrations probably means you can directly use the photo_manager. Since we have only one major contributor (me) to this package, I would say that the package must keep it's shape when doing things that it designed for.

cyanlink Feb 21, 2022
Author

I fully agree and understand that. So I'm planning to build (maybe completely different and refactored) new things in my own fork.

cyanlink · 2022-02-22T07:24:52Z

cyanlink
Feb 22, 2022
Author

Tried to implement a lazy loading mechanism today in the DefaultAssetPickerProvider class, which brings drastic loading speed-up.
My default strategy is to await load RequestType.common first then image and video.
I find it inefficient to load all path and asset upon provider initialization even if asynchronously, because in this way we cannot control its loading at all. Exposing the loading as a method and let the developer call would be better, I called it in my initState of theAssetGridWidget.

2 replies

cyanlink Feb 22, 2022
Author

And I came to the conclusion that the Delegate pattern is not that bad, tried to refactor it into widget but fruitless, there are encapsulated logics in here. However Delegates are in fact stateless! I can just write a AndroidLayout widget or AppleOSWidget, take the delegate as parameter, override the androidLayout or appleOSLayout method into returning AndroidLayout(delegate)/AppleOSWidget(delegate) , and voila! fully composable (e.g. calling delegate.assetsGridBuilder) and now I get a Widget/State context to work on.
The provider parameter of delegate needs to go tho, it's really not needed since we can get them with context.

AlexV525 Feb 25, 2022
Maintainer

Now the default delegate needs a provider since it holds the provider in its own widget tree, rather than placed right under the page route. Otherwise, we'll break the functionality of keepScrollOffset.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Nested between delegates and providers make implementations more complex #237

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 7 comments 4 replies

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

Select a reply

[Feature] Nested between delegates and providers make implementations more complex #237

cyanlink Jan 27, 2022

Replies: 7 comments · 4 replies

cyanlink Jan 27, 2022 Author

AlexV525 Jan 27, 2022 Maintainer

cyanlink Jan 28, 2022 Author

AlexV525 Feb 5, 2022 Maintainer

AlexV525 Feb 10, 2022 Maintainer

cyanlink Feb 18, 2022 Author

AlexV525 Feb 18, 2022 Maintainer

cyanlink Feb 21, 2022 Author

cyanlink Feb 22, 2022 Author

cyanlink Feb 22, 2022 Author

AlexV525 Feb 25, 2022 Maintainer

cyanlink
Jan 27, 2022

Replies: 7 comments 4 replies

cyanlink
Jan 27, 2022
Author

AlexV525
Jan 27, 2022
Maintainer

cyanlink
Jan 28, 2022
Author

AlexV525
Feb 5, 2022
Maintainer

AlexV525
Feb 10, 2022
Maintainer

cyanlink
Feb 18, 2022
Author

AlexV525 Feb 18, 2022
Maintainer

cyanlink Feb 21, 2022
Author

cyanlink
Feb 22, 2022
Author

cyanlink Feb 22, 2022
Author

AlexV525 Feb 25, 2022
Maintainer