Skip to content

Fix possible duplicate ids #1354

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 10, 2024
Merged

Fix possible duplicate ids #1354

merged 1 commit into from
Dec 10, 2024

Conversation

LePips
Copy link
Member

@LePips LePips commented Dec 10, 2024 •
edited
Loading

Since we don't check the uniqueness of ids with uniqueElements we need to at least employ the uniqueness consolidation in IdentifiedArray. Just keep the first item in the event of collisions.

@LePips LePips mentioned this pull request Dec 10, 2024
Open
@LePips LePips requested a review from JPKribs December 10, 2024 20:21
JPKribs
JPKribs approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@JPKribs JPKribs left a comment

Choose a reason for hiding this comment

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

Verified all the See Alls and this resolved what I was seeing. Tested deleting which worked. Editing metadata did not work for pagingLibraryViewModel but I realized that isn't actually implemented.

Looks good to me!

@JPKribs
Copy link
Member

JPKribs commented Dec 10, 2024 •
edited
Loading

@LePips for PagingLibraryViewModel, updating a single item on metadata change, using the new Notification payloads would that just look like this:

        Notifications[.itemMetadataDidChange]
            .publisher
            .sink { item in
                guard let newItem = item as? Element else {
                    return
                }
                self.elements[id: newItem.unwrappedIDHashOrZero] = newItem
            }
            .store(in: &cancellables)

Edit: Nevermind, that does work.

The id.hashvalue is what I'm having trouble wrapping my head around. Like, a pure self.elements[index] = newItem wouldn't work, right?

@LePips
Copy link
Member Author

LePips commented Dec 10, 2024 •
edited
Loading

Like, a pure self.elements[index] = newItem wouldn't work, right?

It would work since IdentifiedArray conforms to MutableCollection which requires subscript(position:):

Although, searching to find the positional index and then setting it that way would be less efficient than using subscript(id:):

@LePips LePips merged commit fac0c3e into jellyfin:main Dec 10, 2024
4 checks passed
ddrccw added a commit to ddrccw/Swiftfin that referenced this pull request Jan 26, 2025
@ddrccw
Merge branch 'upstream/main'
53a269e 
* upstream/main: (392 commits)
  [tvOS] Add pin prompt to sign-in screen (jellyfin#1383)
  [iOS] Admin Dashboard - User Access Tags (jellyfin#1377)
  [Meta] 2025 Disclaimer (jellyfin#1381)
  [tvOS] Delete User from User Selection Screen (jellyfin#1359)
  [iOS] Media Item Menu - Identify Media Item (jellyfin#1369)
  [iOS] Admin Dashboard - User Profiles (jellyfin#1328)
  [iOS] Select all Users When Editing (jellyfin#1373)
  [Meta] Automatic String Organization (jellyfin#1372)
  [iOS & tvOS] Unused Localization Cleanup (jellyfin#1362)
  [tvOS] SelectServerView Change to Menu (jellyfin#1363)
  [tvOS] Update ConnectToServerView & UserSignInView (jellyfin#1365)
  Trim Fastlane Options (jellyfin#1367)
  Update Fastlane Runner (jellyfin#1366)
  [iOS & tvOS] Localize Existing Strings (jellyfin#1361)
  [iOS] Admin Dashboard - User Access Schedules (jellyfin#1358)
  [iOS] Admin Dashboard - Parental Ratings (jellyfin#1353)
  [iOS & tvOS] Error Cleanup (jellyfin#1357)
  update (jellyfin#1356)
  Fix possible duplicate ids (jellyfin#1354)
  [tvOS] Media Item Menu - Refresh / Delete Items (jellyfin#1348)
  ...

Signed-off-by: ddrccw <ddrccw@gmail.com>
@JPKribs JPKribs added enhancement New feature or request developer Improves or alters the developer experience labels Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JPKribs JPKribs JPKribs approved these changes

Assignees
No one assigned
Labels
developer Improves or alters the developer experience enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LePips @JPKribs