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

[iOS] Media Item Menu - Edit Arrays (People, Genres, Studios, & Tags) #1336

Merged
merged 20 commits into from
Dec 6, 2024

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Dec 1, 2024

Summary

Creates views for editing / deleting / adding Genres & Tags in the ItemEditorView. Swipe to delete the item. Edit to multi-select/delete items. Add button takes you to the add view, where the input value is checked against the Server to validate whether it's a new Genre / Tag or the Add-Action will create a new item.

Since Genre has a lookup API, it has suggestions based on the current letters entered. Selecting a suggestion populates the input with that option.

Tags doesn't have have a search function. Instead, I am looking up all items where Tags contains the current input. So, I can tell the user if the current tag exists but getting suggestions isn't feasible unless there is a search that I'm not aware of.

These are grouped together since Genres & Tags are both just [String] so it's similar work between them.

Screenshots

New Button New Button
Empty list Empty Item
Tag Input / Validation Item Editing 4
Genre Input / Validation 1 Item Input 2
Genre Input / Validation 2 Item Input 3
Genre Input / Validation 3 Item Input 4
Item Editing 1 Item Editing 1
Item Editing 2 Item Editing 2
Item Editing 3 Item Editing 3

@JPKribs JPKribs changed the title [iOS] Media Item Menu | Edit Genres & Tags [iOS] Media Item Menu - Edit Genres & Tags Dec 2, 2024
@JPKribs JPKribs marked this pull request as ready for review December 2, 2024 16:20
@JPKribs
Copy link
Member Author

JPKribs commented Dec 2, 2024

The validation portion of this is a 'nice to have' but if we want to cut it I'm not opposed. Jellyfin Web there is no validation but IMO, the validation is nice so you don't end up with several variants of tags / genres.

Right now, the way it works is I populate all genres / tags when the viewModel refreshes. Then, I filter the full list to get 'suggestions' / validate if the genre / tag already exists.

@JPKribs JPKribs marked this pull request as draft December 2, 2024 16:34
…n refresh then filterd locally instead of calling the server for changes.
@JPKribs JPKribs marked this pull request as ready for review December 2, 2024 20:59
@JPKribs
Copy link
Member Author

JPKribs commented Dec 3, 2024

I think there is room to make this more reusable. I'm having a bit of trouble figuring it out. So, one view for editXView that gives you existing [Element] list and editing. Then, a view for adding & validating inputs.

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

I like the "suggestions" feature, however these aren't really suggestions for creating a new item because they already exist. We should probably communicate instead that these values already exist.

Also, we shouldn't be performing linear string matching since tags can especially become large and can have very noticeable performance issues. We should use a trie for that, and implementing something similar to the one below is just fine.

https://www.kodeco.com/books/data-structures-algorithms-in-swift/v3.0/chapters/18-tries

@JPKribs
Copy link
Member Author

JPKribs commented Dec 4, 2024

What would be a better/more descriptive word than "Suggestions"? Right now I have "Matches".

I can take a look at the trie as well! I'm traveling for work this week so that might be a weekend project.

I just did a pretty major rewrite so studio, genre, tags, and people are all included and all share the same views using <Element: Hashable>. I've never used one of these before but it looks like it's working as intended! I want to clean up the logic more but I just wanted to make sure this was the right approach opposed to 4 different views?

@JPKribs JPKribs changed the title [iOS] Media Item Menu - Edit Genres & Tags [iOS] Media Item Menu - Edit Arrays (People, Genres, Studios, & Tags) Dec 5, 2024
@JPKribs JPKribs marked this pull request as draft December 5, 2024 18:57
@LePips
Copy link
Member

LePips commented Dec 5, 2024

descriptive word than "Suggestions"?

Maybe Existing Items? Tbh if we don't come up with a better name, Suggestions is good enough.

@JPKribs
Copy link
Member Author

JPKribs commented Dec 5, 2024

At this point in time, I think I am done with the views and most of the data. Everything works how I would like it and I'm able to re-use the same views for all of the Item.[array] editing.

I've added the ability to re-order the array as well so you can move actors around which appears to be working great as well. Otherwise, this is same as this was 8 commits ago but with some additional polish.

I only have 3 items left:

  1. When the text input value changes, it searches against the full [items] list. The issue is, if I type too quickly and the [items] is too large it can start to get laggy. I would like to figure out a system where the search doesn't occur until the text input value has been static for ~1 seconds. This way, it only searches when the typer pauses.

  2. Figure out the Trie? Trei? Make search gooder. Right now it's noticeable rough for people since there are so many.

  3. State Management. I don't want to freeze the whole view while the people population is loading. I want to make sure searching on the population doesn't occur until the population is loaded fully but I want to queue it so it searches as soon as it's available. So handling some actions that should be blocking and some that can run concurrently.

@LePips
Copy link
Member

LePips commented Dec 5, 2024

type too quickly

You will want to take a look at debounce and how the SearchViewModel works with search terms.

I don't want to freeze the whole view while the people population is loading

Instead of preventing the creation by disabling the button, we can still allow item creation even if the check hasn't happened.

@JPKribs
Copy link
Member Author

JPKribs commented Dec 5, 2024

Debounce is working well! I think I have the form working the way I want it to as well. Now I just need to get the trie figured out:
Screenshot 2024-12-05 at 16 45 01

@JPKribs
Copy link
Member Author

JPKribs commented Dec 6, 2024

I think I'm done? It's definitely much better.

Screenshot 2024-12-05 at 19 31 55

Everything I'm looking to accomplish is done but it does feel just a tad off. If there is any feedback let me know!

Here's the People Edit / Input:
https://github.com/user-attachments/assets/7cb102a6-9f19-4e64-bd03-37aaf208bc04

Here's what non-people look like:
Screenshot 2024-12-05 at 19 53 17

@JPKribs
Copy link
Member Author

JPKribs commented Dec 6, 2024

Here are all of the actions now:
Screenshot 2024-12-05 at 19 55 07

@JPKribs JPKribs marked this pull request as ready for review December 6, 2024 02:55
@JPKribs
Copy link
Member Author

JPKribs commented Dec 6, 2024

Alright! Final change on this should be done. I was just wrapping up some permission changes. jellyfin/jellyfin-web#6361, I realized that Collections show up in the 'Allow deletion from' options but that doesn't actually do anything. Being isAdministrator determines you ability to change this.

I've structured item permissions a bit better since we have some user permissions in the userSession but we also have some user item permissions. It starts getting a little complicated some items like deletion are handled at a library level while downloads are handled at the user level. I've scaffolded out the other permissions we will eventually need as well:

struct UserPermissions {

    let isAdministrator: Bool
    let items: UserItemPermissions

    init(_ policy: UserPolicy?) {
        self.isAdministrator = policy?.isAdministrator ?? false
        self.items = UserItemPermissions(policy, isAdministrator: isAdministrator)
    }

    struct UserItemPermissions {

        let canDelete: Bool
        let canDownload: Bool
        let canEditMetadata: Bool
        let canManageSubtitles: Bool
        let canManageCollections: Bool
        let canManageLyrics: Bool

        init(_ policy: UserPolicy?, isAdministrator: Bool) {
            self.canDelete = policy?.enableContentDeletion ?? false || policy?.enableContentDeletionFromFolders != []
            self.canDownload = policy?.enableContentDownloading ?? false
            self.canEditMetadata = isAdministrator
            
            // TODO: SDK 10.9 Enable Comments
            self.canManageSubtitles = isAdministrator // || policy?.enableSubtitleManagement ?? false
            self.canManageCollections = isAdministrator // || policy?.enableCollectionManagement ?? false
            
            // TODO: SDK 10.10 Enable Comments
            self.canManageLyrics = isAdministrator // || policy?.enableSubtitleManagement ?? false
        }
    }
}

This makes it usersession.user.permissions.isAdministrator and usersession.user.permissions.items.canDelete. If this structure isn't a hit I wouldn't be offended but this made sense to me. Here would be the final permission. The idea being there is a server policy setting then we have a Swiftfin setting to toggle that locally. I think we can default all of these to False but Downloads, when it comes, might be worth defaulting to true for items where canDownload is valid.

Action UserState UserPermissions
Edit Collection enableCollectionManagement canManageCollections
Delete Collection enableCollectionManagement canManageCollections
Edit Item Metadata enableItemEditing canEditMetadata
Delete Item enableItemDeletion canDelete
Managing Subtitle enableSubtitleManagement canManageSubtitles
Managing Lyric enableLyricsManagement canManageLyrics
Download Item enableItemDownloads canDownload

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

I noticed we were trying to match search results with types, which used contains to do n^2 searches. I changed the Trie to be able to hold key, element types to be able to search based on the key and get the resulting elements.

The ItemArrayElements uses a lot of casting, there should be a way to use generics instead but I'm okay with this implementation for now.

I think the user permission structure is a good idea! It makes things cleaner with the many permissions and how they work together.

@LePips LePips merged commit a3d84a9 into jellyfin:main Dec 6, 2024
4 checks passed
@JPKribs JPKribs deleted the mediaEditArray branch December 7, 2024 00:20
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.

2 participants