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(mobile): the page for adding photos to the album cannot be navigated back using gestures #16449

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ExceptionsOccur
Copy link
Contributor

@ExceptionsOccur ExceptionsOccur commented Mar 1, 2025

fix(mobile): the page for adding photos to the album cannot be navigated back using gestures #16449

@ExceptionsOccur ExceptionsOccur force-pushed the fix/gestures-invalid-in-album branch 2 times, most recently from df04152 to cd89e5c Compare March 3, 2025 01:08
@shenlong-tanwen
Copy link
Member

@ExceptionsOccur Thanks a lot for the fix. Can you rebase the branch over main and remove the changes in other files. The fix is only in the file immich_asset_grid_view.dart, but your diff contains other changes as well.

@shenlong-tanwen shenlong-tanwen self-requested a review March 3, 2025 11:39
@ExceptionsOccur ExceptionsOccur force-pushed the fix/gestures-invalid-in-album branch from cd89e5c to 3667156 Compare March 3, 2025 13:24
@ExceptionsOccur
Copy link
Contributor Author

@ExceptionsOccur Thanks a lot for the fix. Can you rebase the branch over main and remove the changes in other files. The fix is only in the file immich_asset_grid_view.dart, but your diff contains other changes as well.

Alright, the code for the other part will be submitted in a new PR.

@shenlong-tanwen
Copy link
Member

Alright, the code for the other part will be submitted in a new PR.

A small remark though, your change makes the back gesture deselect all assets and immediately close the selection page. However, it is a much better experience if the first gesture deselects the assets and the next one goes back. The actual change to handle this is rather simple, you just need to find the difference between the selectedAssets and the preselectedAssets and prevent pop when the difference is empty

@ExceptionsOccur
Copy link
Contributor Author

Alright, the code for the other part will be submitted in a new PR.

A small remark though, your change makes the back gesture deselect all assets and immediately close the selection page. However, it is a much better experience if the first gesture deselects the assets and the next one goes back. The actual change to handle this is rather simple, you just need to find the difference between the selectedAssets and the preselectedAssets and prevent pop when the difference is empty

Indeed, it can be done this way. I will optimize it later. Mainly, I remember that before it was returned directly, so when I fixed it, I also did it according to the previous idea.

@ExceptionsOccur
Copy link
Contributor Author

First-time return gesture adds the feature to cancel all current selections
The first time the selection is canceled, the second time it will return; if there are no new selections, it will return directly

if (didPop) {
return;
} else {
if (_selectedAssets.length != widget.preselectedAssets!.length &&
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't always assume the widget.preselectedAssets value will be available and should handle it being null more gracefully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants