Skip to content

Conversation

@shenlong-tanwen
Copy link
Member

No description provided.

@shenlong-tanwen shenlong-tanwen force-pushed the feat/sqlite-native-sync branch from e99f4ff to c0acd86 Compare May 5, 2025 17:12
@github-actions github-actions bot added documentation Improvements or additions to documentation 🖥️web 🗄️server labels May 8, 2025
@alextran1502 alextran1502 force-pushed the feat/sqlite-native-sync branch from 0424d32 to 3360883 Compare May 8, 2025 14:28
@shenlong-tanwen shenlong-tanwen force-pushed the feat/sqlite-native-sync branch from 3360883 to 09e5503 Compare May 8, 2025 14:39
Comment on lines 110 to 111
var updatedArr: Set<WrapperAsset> = []
var deletedArr: Set<String> = []
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if an asset is updated and then later deleted before this runs? Would the changes only show the deletion or would it end up in both lists?

Copy link
Member Author

Choose a reason for hiding this comment

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

It only shows up as deleted from my limited testing. But we need to note these during our full testing of the implementation because the PhotoKit API is extremely quirky

let deleted = details.deletedLocalIdentifiers

let options = PHFetchOptions()
options.includeHiddenAssets = false
Copy link
Member

Choose a reason for hiding this comment

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

If an asset started as not hidden and became hidden later, would this mean that immich would ignore this and later changes? Is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not handle hidden assets yet and ignoring them is okay for the time being. And, I don't think we need a PHFetchOption in the first place. I'll update the method call and pass nil for the options

createdAt: createdAt,
updatedAt: updatedAt,
durationInSeconds: durationInSeconds,
albumIds: self._getAlbumIds(forAsset: asset)
Copy link
Member

Choose a reason for hiding this comment

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

You only need to do this for updated assets, not for inserted, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We would also need this for inserted assets as well. If an asset was inserted after our last sync and also got added to multiple albums before we get to sync the changes, we need the albumIds to link it during our sync

Copy link
Member

Choose a reason for hiding this comment

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

True, I guess I meant assets that were inserted but not updated, because we already know they're not in any albums (or they would be updated too).

}

@RequiresApi(Build.VERSION_CODES.Q)
fun shouldFullSync(callback: (Result<Boolean>) -> Unit) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the callbacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

I marked the methods as asynchronous in the messages template which generates the methods in the native side with a callback expecting the native methods to be asynchronous. However, none of our methods are asynchronous and removing the @async annotation actually makes the native API a lot more simpler. Will remove the async annotation and update the native methods to be synchronous

@shenlong-tanwen shenlong-tanwen force-pushed the feat/sqlite-native-sync branch from d583c07 to 4a9b8c2 Compare May 13, 2025 09:24
@shenlong-tanwen shenlong-tanwen force-pushed the feat/sqlite-native-sync branch from 4a9b8c2 to 30fc632 Compare May 13, 2025 12:53
@shenlong-tanwen
Copy link
Member Author

Superseded by #18428

@alextran1502 alextran1502 deleted the feat/sqlite-native-sync branch May 28, 2025 18:57
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