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

✨ Ability to disable preview of assets using flag (showPreview) #102

Merged
merged 14 commits into from Apr 30, 2021
Merged

✨ Ability to disable preview of assets using flag (showPreview) #102

merged 14 commits into from Apr 30, 2021

Conversation

yanivshaked
Copy link
Contributor

Hi,

I have added a new flag called showPreview to AssetPicker.pickAssets.

Default behavior (default value of showPreview parameter is true) is identical to the behavior so far: if you click an asset, you will get a preview of it. If you click the small circle, you select the asset.
But if you provide showPreview with false, there is no option to show preview. Instead, click on an assert will either add it to selection (multi assets mode) or select it (single assets more).
That behavior is closer to "mainstream" application which have an image picker integrated (Facebook, Whatsapp, Instgram, Megatok, etc..).

I hope you will find this PR interesting and worthy to integrate soon.

Thank you,
Yaniv

@AlexV525
Copy link
Member

Thanks for the PR. Could you provide some screen records about those apps you mentioned? Also since this package mainly aims to cover the interaction like the WeChat, originally it's allowed to preview. I do think it's worth to integrate, just need some verification.

@yanivshaked
Copy link
Contributor Author

See the following screen recordings:

As you can see, there is no preview available only after the image is selected. Then the user can edit the picture (crop/paint/etc...).

Next screen recording is to demontrate the behavior of wechat asset picker when calling it with showPreview = false (the non default behavior): download

I hope you like it!

Yaniv

@yanivshaked
Copy link
Contributor Author

Please let me know if you need more information/clarifications!

@AlexV525 AlexV525 added await investigate The issue is waiting for further investigation. s: feature There's a feature request in this issue. s: UI This issue is related with UI or layout. labels Apr 29, 2021
@AlexV525
Copy link
Member

Thanks for these videos.

From the video of WhatsApp, I do saw you entered the preview without made the asset selected. Let me know if I didn't get it right.

image

And the same thing also happened in the MegaTok video.

But the recording of this package seems to be clear that what showPreview will do. So yes basically it'll disable the preview for all assets when you tapped them. IMO it will work either in multi select mode or in single mode, but it'll fail at the special wechatMoment type. See the definition at here.

/// The current special picker type for the picker.
/// 当前特殊选择类型
///
/// There're several types which are special:
/// * [SpecialPickerType.wechatMoment] When user selected video, no more images
/// can be selected.
///
/// 这里包含一些特殊选择类型:
/// * [SpecialPickerType.wechatMoment] 微信朋友圈模式。当用户选择了视频,将不能选择图片。
final SpecialPickerType? specialPickerType;

You may want to walk through the special mode to see how it aligns the behavior with WeChat Moment.

@yanivshaked
Copy link
Contributor Author

Hi, See my comments inlined.

From the video of WhatsApp, I do saw you entered the preview without made the asset selected. Let me know if I didn't get it right.

Thats is incorrect. I understand the video can be misleading. WhatsUp opens the gallery in single mode, any click on any image selects it and allows you to crop/paint/.. over it. You may then send it or click the +, to reopen the gallery (this time in multi mode), add more images and click done to have them selected. Same thing happens for MegaTok.

But the recording of this package seems to be clear that what showPreview will do. So yes basically it'll disable the preview for all assets when you tapped them. IMO it will work either in multi select mode or in single mode, but it'll fail at the special wechatMoment type. See the definition at here.

You may want to walk through the special mode to see how it aligns the behavior with WeChat Moment.

You are correct. The new flag showPreview broke the special behavior of the the WeChat Moment.
A few alternatives to deal with it:

  • Ignore showPreview if SpecialPickerType.wechatMoment is used.
  • Instead of using a new parameter called showPreview, I can add a new value to the SpecialPickerType enumeration, for example SpecialPickerType.noPreview. This will be mutually exclusive with wechatMoment by design and will implement the required behavior.

What do you think?

@yanivshaked
Copy link
Contributor Author

Another alternative, is to override (showPreview = true) here, just like you override requestType provided:

if (specialPickerType == SpecialPickerType.wechatMoment) {
requestType = RequestType.common;
}

@AlexV525
Copy link
Member

I'd prefer SpecialPickerType.noPreview, because if we make it a boolean flag, there will be various combined condition if (showPreview && specialPickerType == SpecialPickerType.wechatMoment). Instead, a new enumeration will have no conflict with the old behavior.

@yanivshaked
Copy link
Contributor Author

yanivshaked commented Apr 30, 2021

Done, code refactored (SpecialPickerType.disablePreview).

Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

Didn't finish the review since the rest part is quite a mass, I'll run it locally one I have time.

example/lib/pages/custom_picker_page.dart Outdated Show resolved Hide resolved
example/lib/pages/multi_assets_page.dart Outdated Show resolved Hide resolved
lib/src/constants/enums.dart Outdated Show resolved Hide resolved
lib/src/delegates/asset_picker_builder_delegate.dart Outdated Show resolved Hide resolved
@AlexV525
Copy link
Member

You can apply suggestions on the GitHub without manually update it :)

@yanivshaked
Copy link
Contributor Author

You can apply suggestions on the GitHub without manually update it :)

Yes, I know, but I wanted to change the English text.

wo de yingwen fei chang hao, keshi wo de zhongwen bu hao ;-)

example/lib/pages/multi_assets_page.dart Show resolved Hide resolved
lib/src/constants/enums.dart Show resolved Hide resolved
lib/src/constants/enums.dart Outdated Show resolved Hide resolved
lib/src/constants/enums.dart Outdated Show resolved Hide resolved
lib/src/delegates/asset_picker_builder_delegate.dart Outdated Show resolved Hide resolved
lib/src/delegates/asset_picker_builder_delegate.dart Outdated Show resolved Hide resolved
lib/src/widget/asset_picker.dart Show resolved Hide resolved
@AlexV525
Copy link
Member

AlexV525 commented Apr 30, 2021

I added some changes but unable to push to your branch. So let me paste them here.

  /// Whether the preview of assets is enabled.
  /// 资源的预览是否启用
  bool get isPreviewEnabled => specialPickerType != SpecialPickerType.noPreview;

image

image

image

image

@AlexV525
Copy link
Member

Remember to format the file once you finish your changes.

@AlexV525 AlexV525 removed the await investigate The issue is waiting for further investigation. label Apr 30, 2021
@yanivshaked
Copy link
Contributor Author

Remember to format the file once you finish your changes.

Line length you are using? 80?

@AlexV525
Copy link
Member

Line length you are using? 80?

Yep. Some comments might overflow a bit, just ignore them.

@yanivshaked
Copy link
Contributor Author

I'm unable to copy the Chinese, can you please paste the text characters?

image

@AlexV525
Copy link
Member

  /// Whether the preview of assets is enabled.
  /// 资源的预览是否启用
  bool get isPreviewEnabled => specialPickerType != SpecialPickerType.noPreview;

@yanivshaked
Copy link
Contributor Author

I was referring to those changes:

image

@AlexV525
Copy link
Member

I was referring to those changes:

Those are already suggested at here: #102 (comment)

Co-authored-by: Alex Li <[email protected]>
@yanivshaked
Copy link
Contributor Author

Tough code review, highly appreciated!

@AlexV525
Copy link
Member

That's why we've got an A+ at code factor. 😄

Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

LGTM. And thanks for this solid improvement. 👍

@yanivshaked
Copy link
Contributor Author

My pleasure!

@AlexV525 AlexV525 merged commit 2300382 into fluttercandies:master Apr 30, 2021
@yanivshaked yanivshaked deleted the 20210428_ShowPreview_flag_support branch May 7, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: feature There's a feature request in this issue. s: UI This issue is related with UI or layout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants