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): add album preview images in backup selection #14950

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

Conversation

Tyris
Copy link
Contributor

@Tyris Tyris commented Dec 27, 2024

This PR adds album preview images in the backup selection pages.

For the grid layout (wide-screen, >600px), the only real change is to use the first image in the album.

For the list layout (<=600px), I've removed the icon and made adjustments to make it more similar to the grid view (to facilitate the preview) - however, this work did raise the question: should we just always use a grid view? (since it looks fine on mobile).
Grid (note, this was on mobile since I don't have a tablet handy):
image
List:
image

I also:

  • Fixed an exception when trying to set data after an element was disposed.
  • Removed the album ID from the album preview page (it's always a very large negative number... not sure what value it was supposed to provide?)
  • Ordered the album selection alphabetically (with the "Recent" (isAll) folder at the top).

@Tyris Tyris marked this pull request as ready for review December 27, 2024 10:32
@@ -15,7 +16,9 @@ class AlbumMediaRepository implements IAlbumMediaRepository {
await PhotoManager.getAssetPathList(
hasAll: true,
);
return assetPathEntities.map(_toAlbum).toList();
return assetPathEntities.map(_toAlbum).sortedBy((a) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be sorted from the page that needs the sorted info

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 couldn't think of any examples where you wouldn't want the album's in a "sane/expected order", hence putting it here. There's no real performance impact (since it's a cheap operation).
Let me know if you disagree and I'll move it.

final isDarkTheme = context.isDarkTheme;

final previewAsset = useState<Asset?>(null);
getPreviewAsset() async {
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 extract this logic using a helper method to avoid duplicated code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but wasn't sure where best to extract to. Where do you recommend for these bits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nm, consolidated into a single file (there was enough duplication of business logic as well as display logic to justify this imo).

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.

Thank you for the PR. I left a few comments

Can you help test with an empty album as well? regarding the album thumbnail behavior

@Tyris
Copy link
Contributor Author

Tyris commented Dec 27, 2024

Thank you for the PR. I left a few comments

Can you help test with an empty album as well? regarding the album thumbnail behavior

Will do, although in Android i think an album is literally defined by a folder with at least one image or video in it - so not sure this is possible. If you scroll quickly, you can see the null/empty case before the preview image loads which results in the fallback image.

Will try to test on iOS simulator too.

@alextran1502
Copy link
Contributor

I've just tested on iOS, and it crashes when I access the page, I think it is due to some empty album

@Tyris
Copy link
Contributor Author

Tyris commented Jan 2, 2025

I've just tested on iOS, and it crashes when I access the page, I think it is due to some empty album

Noted. I've been having a lot of trouble getting the app running on either simulator or physical device for iOS; I'll look at making a fix first, then I'll see if one of my iOS guys can help me get it running on device.

I've got it running on simulator (had to replace import path_provider_foundation with import path_provider_ios... not sure why) - but I can't reproduce a crash with empty albums:
image

Can you post crash log?

Separately (but somewhat related) - I was curious about "album" behaviour between Android and iOS since I'm keen to make some changes there; I posted the following on discord:

Tyris: Can someone explain the purpose of being able to "exclude" (double tap) albums in the "select albums" for backup page? (am looking at changes in this area and don't "get" that bit...)
Tyris: I might be misunderstanding things... on Android at least, I thought albums were literally just "a folder that has >=1 image or video and isn't hidden"; and that they were effectively populated by the system. So it shouldn't be possible to for items to exist in multiple "albums".

From speaking to a colleague, it sounds like iOS albums are populated based on the OS's definition of albums (in which a photo can exist in multiple albums), whereas on Android, albums are populated based on the ancient Android Gallery model of grouping based on BUCKET_ID(?), suggesting that photos can only exist in a single album (and also that empty albums can't exist).

Does that sound right?

@JustAdreamerFL
Copy link

Hi, don't know if I should say it here but I think it would be at least a great option for users to be able to switch between list or grid view.

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.

4 participants