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

Enable swiping between assets #381

Merged
merged 32 commits into from
Aug 3, 2022
Merged

Enable swiping between assets #381

merged 32 commits into from
Aug 3, 2022

Conversation

TCVinNYC
Copy link
Contributor

Adding left/right swiping between assets using PageViewer

This is Pr V2 because I'm a little dumb lol

Copy link
Contributor

@alextran1502 alextran1502 left a comment

Choose a reason for hiding this comment

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

Hey man, thanks for this. Everything looks great I left a few comments. Besides that AMAZING!

scaleStateChangedCallback: _scaleStateChanged,
onScaleEnd: _onScaleListener,
return SizedBox(
height: MediaQuery.of(context).size.height,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to wrap the SizedBox here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flutter throws an error for having undefined/infinite height and width.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, I guess I accidentally fixed it after I encountered the error haha

},
[],
);
@override
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you have used useEffect above, I think we don't need initState function here

https://pub.dev/documentation/flutter_hooks/latest/flutter_hooks/useEffect.html

@alextran1502
Copy link
Contributor

So I tested on an instance with 13000 photos/videos. There is no noticeable performance issue. The only issue is that when you double tap on an image to zoom, you cannot pinch with two fingers to zoom in/out. If you swipe left or right in zoom mode, it will navigate forward or backward. I think we will need a flag to handle the zoom or normal mode and which interaction can be done in each mode

@TCVinNYC
Copy link
Contributor Author

Awesome!
Still going to move things around and only run things once for peace of mind haha
Once I figure out that, I'll fix the zooming issue.

@alextran1502
Copy link
Contributor

I think let's make fixing this PR a priority to merge it in. The dark mode PR might need a lot of additional works

@alextran1502
Copy link
Contributor

alextran1502 commented Aug 1, 2022

  1. If I click on the first asset in the list, the scrolling is disabled
  2. I still cannot use two fingers to zoom in and out when pinching

Tested on Samsung S9

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 1, 2022

  1. It throws an exception error, uploading a fix in a few minutes.
  2. That's interesting, I'll test on Android emulator. I don't have an android atm

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 1, 2022

Did some more cleanup, and I added more files to attempt to save the sorted list in a Hive box, but I'm having issues getting it to save.

All swipes & gestures for the main page should work.

Seems fine on iPhone, and Android emulator except Double Taps don't register as "zoomed in" until you move the image.

@@ -31,66 +25,17 @@ class VideoViewerPage extends HookConsumerWidget {

String jwtToken = Hive.box(userInfoBox).get(accessTokenKey);

void showInfo() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we remove these functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GalleryView already does the same function.

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

That's so interesting.
How many assets are in this album?

Maybe this if (list.isEmpty) is causing this problem.

@alextran1502
Copy link
Contributor

That's so interesting. How many assets are in this album?

Maybe this if (list.isEmpty) is causing this problem.

There is 1 asset in the album. list.isEmpty is not a problem since it is specific for getting the list of albums for backing up. This is the sharing album feature

@alextran1502
Copy link
Contributor

alextran1502 commented Aug 3, 2022

image
This is the problem :)

album_viewer_page.dart line 186

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

Ohh haha, that's okay. I'll remove it after I'm done figuring out the EXIF issue.

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

Try selecting an album with more than 3 items haha.

@alextran1502
Copy link
Contributor

Another behavior I just discovered is that you can only swipe in one date group. See the video below.

Android.Emulator.-.PixelXLAPI30_5554.2022-08-02.21-38-15.mp4

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

Yeah don't worry, that was only an issue from last commit because I was fixing the albums situation first lol


@override
Widget build(BuildContext context, WidgetRef ref) {
//get the list of whats in the assets
Copy link
Contributor

Choose a reason for hiding this comment

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

If this works, I am OK leaving this here. I don't want to use Hive if we don't really need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to move it if we could mainly because the album list is already sorted, so it's just reduntant. I can keep it here if you'd really like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured out that I needed to at least move this method somewhere else so it didn't conflict with the Albums swipe.
The latest commit should work perfectly for swipes in all areas now.

@alextran1502
Copy link
Contributor

Ok I think that is all I have for this route. I am pretty happy with the current behavior, very slick. Thanks again!

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

Ok I think that is all I have for this route. I am pretty happy with the current behavior, very slick. Thanks again!

Appreciate the feedback!

TCVinNYC and others added 4 commits August 2, 2022 23:41
* Refactor sharing to album

* Added library page in the bottom navigation bar

* Refactor SharedAlbumService to album service

* Refactor apiProvider to its file

* Added image grid

* render album thumbnail

* Using the wrap to render thumbnail and album info better

* Navigate to album viewer

* After deletion, navigate to the respective page of the shared and non-shared album

* Correctly remove album in local state

* Refactor create album page

* Implemented create non-shared album
@alextran1502
Copy link
Contributor

Please fixed the merge conflict, I will have this merged in as soon as tomorrow :)

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

@alextran1502 I don't have write access to fix things here. I could try doing it manually though if you'd rather me do that haha

@alextran1502 alextran1502 linked an issue Aug 3, 2022 that may be closed by this pull request
@alextran1502
Copy link
Contributor

@alextran1502 I don't have write access to fix things here. I could try doing it manually though if you'd rather me do that haha

What you would like to do is to merge upstream and then fix the merge conflict :) That is how Git solves these conflict issue

@TCVinNYC
Copy link
Contributor Author

TCVinNYC commented Aug 3, 2022

@alextran1502 I don't have write access to fix things here. I could try doing it manually though if you'd rather me do that haha

What you would like to do is to merge upstream and then fix the merge conflict :) That is how Git solves these conflict issue

Ohh I see, thanks for the heads up!!

@alextran1502
Copy link
Contributor

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork Here is a relevant guide

@@ -16,3 +16,7 @@ const String backupInfoKey = "immichBackupAlbumInfoKey"; // Key 1
// Github Release Info
const String hiveGithubReleaseInfoBox = "immichGithubReleaseInfoBox"; // Box
const String githubReleaseInfoKey = "immichGithubReleaseInfoKey"; // Key 1

// Image Sorted List Info
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also remove these since we don't use them?

@@ -361,7 +361,7 @@
CODE_SIGN_IDENTITY = "Apple Development";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please exclude this file from the PR

@alextran1502 alextran1502 merged commit 8c184dc into immich-app:main Aug 3, 2022
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.

[Feature] Swipe between photos
4 participants