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

feat(mobile): Added "jump to date" functionality to the memory view #7323

Merged
merged 25 commits into from
Apr 24, 2024

Conversation

arnolicious
Copy link
Contributor

@arnolicious arnolicious commented Feb 21, 2024

Added a "Jump to date" Button in the bottom Info bar of the memory view.

It was somewhat mentioned here, but it is mainly a feature I've found myself wanting quite often.

As to the implementation, I don't have much Flutter experience, so my approach to communicate between the two components (the button in the memory view, and the Timeline list that needs to scroll) was with events, for which I've added the event package

If there's a better/more "fluttery" way to do this, let me know!

And for the UI, at first I wanted to add an IconButton, however I couldn't really find a fitting Icon for this action (the closest I could come up with, was the open_in_new Icon), so I settled on a TextButton, and added the text to the languages I speak.

Here's a demo of what it looks like, on a very choppy emulator :D
https://github.com/immich-app/immich/assets/46051866/289750d1-d2b5-4619-8441-2c77295dd57b

Copy link
Member

@shenlong-tanwen shenlong-tanwen left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this PR. I'm sure a lot of users would find this useful. Can you please address the review comments. Also, Can we change the button to an IconButton instead? Choose any icon you find appropriate for now and we can change it later. open_in_new,move_down or timeline seems appropriate to me

Copy link
Contributor

@martyfuhry martyfuhry left a comment

Choose a reason for hiding this comment

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

This looks great and is super useful!

alextran1502 and others added 4 commits February 27, 2024 10:47
- Added index bound checks
- Handled edge cases when scrolling to the very bottom of the grid-view
- removing the listener on dispose
@alextran1502
Copy link
Contributor

I tested the PR and when clicking on the button to jump to timeline, it doesn't do anything 🤔

@arnolicious
Copy link
Contributor Author

Well that's kind of embarassing, i had left an offset in the scroll index from when i was testing...
I hope that was the issue, but I added an Error toast for when the scrolling fails due to the index being out of bounds (for debug purposes, I'd remove that before merging)

Did the memory view at least close when you clicked the button, @alextran1502 ?

@alextran1502
Copy link
Contributor

Well that's kind of embarassing, i had left an offset in the scroll index from when i was testing... I hope that was the issue, but I added an Error toast for when the scrolling fails due to the index being out of bounds (for debug purposes, I'd remove that before merging)

Did the memory view at least close when you clicked the button, @alextran1502 ?

Hello, yes, when clicking on the button, it dismisses the memory view

@arnolicious
Copy link
Contributor Author

I'm kinda stumped, i added another toast, in case the date that the listener receives is somehow null...

Are you testing this on the demo instance by any chance?

@DawidPietrykowski
Copy link
Contributor

I tested it on my instance and it works, hides the memory view and jumps to the correct date

@alextran1502
Copy link
Contributor

Hey @arnolicious, I just tested again on my prod instance. When I clicked on the link, I got this error

image

@arnolicious
Copy link
Contributor Author

Hm, an index of -1 would indicate no item in the list matching the date was found, which shouldn't happen, since if that memory exists, it should also exist in the timeline.

The check looks right to me, at first i suspected timezones to possibly be the cause of this, but I couldn't replicate it.
(I assume your server and your phone are in the same timezone, right?)

final index = widget.renderList.elements.indexWhere(
      (e) =>
          e.date.year == date.year &&
          e.date.month == date.month &&
          e.date.day == date.day,
);

Is the date in the error message correct, is that the date of the memory you clicked on?
I assume that somehow the date must be incorrect, maybe it's shifted by a day, and so I'd assume either the day before or after that memory does not have any assets... Is that the case? @alextran1502

@martyfuhry
Copy link
Contributor

I'm curious, what if you group the assets on the timeline by month instead of day? Does that change the expected index in the render list?

I'm thinking about this setting, in particular.

Screenshot_20240322_164141.jpg

@arnolicious
Copy link
Contributor Author

arnolicious commented Mar 22, 2024

Oh, didn't know that was a thing lol

Gonna look into it, thanks for pointing that out!

(Edit: Yeah, that causes the error, lets hope Alex had the timeline setting grouped to month, so it is the same issue :D )

@martyfuhry
Copy link
Contributor

You should also gracefully handle the index not found case. In the ideal case, we'd hide the button entirely. But we can also show a nicely formatted error that we can't find the day in the timeline. Maybe we can do something even better?

@arnolicious
Copy link
Contributor Author

I'm not sure how feasible or necessary hiding the button would be, since it really shouldn't happen, once this bug is fixed. I will make the toast error more user friendly tho.

The question now is rather, what should happen when the timeline is grouped by month?
Scrolling to the month is fine, but I'm not sure how/if it's possible to then find the specific day in the month and scroll to it... 😅

@alextran1502 alextran1502 merged commit dc9b51a into immich-app:main Apr 24, 2024
22 checks passed
@arnolicious arnolicious deleted the feat(mobile)-jump-to-asset branch April 24, 2024 20:49
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.

None yet

6 participants