Skip to content

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Nov 21, 2024

Summary

Adds a section to the ItemEditorView to allow for manually changing most metadata. This does not include free-form content such as:

  • Studios
  • Genres
  • People
  • Tags

These will be added in a later PR to keep this from getting too large. These will require an editView & addView to allow deleting existing items or adding a new item to them. To simplify review, these can be seen here but I'll add these as a separate component when ready.

Screenshots

New Button New Button
View pt. 1 View pt. 1
View pt. 2 View pt. 2
View pt. 3 View pt. 3

@JPKribs
Copy link
Member Author

JPKribs commented Nov 21, 2024

This is still coming in a little bigger than I would like it to but a lot of the line count is coming from the enums/extensions that are more volume than substance. I'm leaving this in draft for a sec because I'm under the weather this week and want to review this more in depth with a clear head. Primarily I want to look at:

  1. Should Metadata & Lock Metadata Sections be two separate views?
  2. Does splitting the Pickers into their own files make sense? It makes sense for Language since I can see use reusing that for something like 'Default subtitles' or a feature like that later but I doubt Country will get much use. Finally, I would be amazed if 3DVideoFormatPicker ever gets usages outside this PR.
  3. Does the order of the sections/buttons make sense?
  4. Is the current switch case for EditMetadataView the best way to do this between ItemTypes or is there a better way to be doing this?

@JPKribs JPKribs changed the title [WIP] [iOS] Media Item Menu | Edit Metadata Pt. 1 [WIP] [iOS] Media Item Menu | Edit Metadata Nov 22, 2024
@JPKribs JPKribs marked this pull request as ready for review November 24, 2024 03:01
@JPKribs JPKribs changed the title [WIP] [iOS] Media Item Menu | Edit Metadata [iOS] Media Item Menu | Edit Metadata Nov 24, 2024
@JPKribs JPKribs added enhancement New feature or request and removed enhancement New feature or request labels Nov 24, 2024
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 moved to use the binding extensions like in #1313, so I'm sure we may get a merge conflict based on whatever is merged first. I had to add the NilIfEmptyStringFormatStyle so that if a field was nil, then just tapping on the field didn't "change" the value if I used coalesce. We still have the issue of nil values for dates, since DatePicker can't take an optional binding, but that's okay. Also, if the description is nil and the user taps on the field, it will coalesce to an empty string and indicate a change but I'm fine with that.

I think the order looks good and I'm not worried about the file structure.

Copy link
Member Author

@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.

Leaving myself some notes on mobile to look at when I pick this back up on the computer.

- Fix the notification when metadata was updated to work with 100% consistency
- Flip the locking to be true -> lock like server
- Redo the whole itemEditorViewModel to be more in-line with other viewModels | also fixes iPad weirdness
- Use itemViewModel for the edit view so I can just reuse those existing notifications instead of recreating the wheel
- More human dates for people - Date of death instead of "End date" (yikes)
#Conflicts:
#	Swiftfin/Views/ItemEditorView/EditMetadataView/Components/Sections/DateSection.swift
#	Swiftfin/Views/ItemEditorView/EditMetadataView/Components/Sections/LockMetadataSection.swift
#	Swiftfin/Views/ItemEditorView/EditMetadataView/Components/Sections/OverviewSection.swift
@JPKribs
Copy link
Member Author

JPKribs commented Nov 27, 2024

I think this mostly looks good! I've merged this to main and stomped out some of the build issues. I just went through a cleanup an I think there are only items I don't feel super confident in:

  1. I found that the notification itemMetadataDidChange was focused around userData. So I made a new one for updating metadata that's just itemId: itemMetadataWasEdited. Since I made this change, this is working really well now!

  2. Where I used to pass in a BaseItemDto for the ItemEditorView I am instead passing in the ItemViewModel since this can only be called from ItemView which already has it and this let's me re-use the updating from the ItemView without recreating it unnecessarily.

  3. I cleaned up a lot of strings. I've been using Copilot to localize everything and there was a stint where all of my localizations has a redundant 3rd comment. I've just trimmed all my old localizations to match.

  4. I made a languagePicker. I'm hoping we can reuse it later for something like "Set default audio language" or "Set default subtitle langauge" so please let me know if there's anything we want to adjust on that to make it more universal.

  5. LockMetadata I think it would be good to add an animation for when the other lock options disappear. I just wasn't sure which one to use.

  6. ParentalRating works. I'm getting the server's localized ratings which is good but the pickers for it is kind of clunky. Now sure how to make it better and it works so I don't want to blow it up but I'm open to revisions!

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.

Everything looks great just need the consolidation of the notification.

2 - passing in the view model is fine
4 - the picker looks good for now, I don't know what changes we'll need in the future but we'll deal with them then
5 - I agree an animation would look nice but that's difficult when the rows are the bottom and the scroll offset changes, I'm fine with a snap here

…le" menu object in the toolbar. Makes it easier to ensure that this format looks the same throughout.
@JPKribs
Copy link
Member Author

JPKribs commented Nov 30, 2024

@LePips What do you think of something like this as a viewModifier:

//
// Swiftfin is subject to the terms of the Mozilla Public
// License, v2.0. If a copy of the MPL was not distributed with this
// file, you can obtain one at https://mozilla.org/MPL/2.0/.
//
// Copyright (c) 2024 Jellyfin & Jellyfin Contributors
//

import Defaults
import SwiftUI

struct NavigationBarMenuButtonModifier<Content: View>: ViewModifier {

    @Default(.accentColor)
    private var accentColor

    let isLoading: Bool
    let isHidden: Bool
    let items: () -> Content

    func body(content: Self.Content) -> some View {
        content.toolbar {
            ToolbarItemGroup(placement: .topBarTrailing) {

                if isLoading {
                    ProgressView()
                }

                if !isHidden {
                    Menu {
                        items()
                    } label: {
                        Label(L10n.options, systemImage: "ellipsis.circle")
                    }
                    .labelStyle(.iconOnly)
                    .backport
                    .fontWeight(.semibold)
                    .foregroundStyle(accentColor)
                }
            }
        }
    }
}

Since we'll now have this Menu in 3 spots: ItemView, ServerUsersView, and PagingLibraryView. I've included this in the latest commit. The isHidden is specifically so it can work for ServerUsersView where the ... gets replaced by a regular button in editMode. Otherwise, the isLoading is just to avoid the headache of making sure that is consistent throughout usage.

Let me know if you have any changes or if this isn't a good route for this!

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.

A wrapper for the similar toolbar loading+menu is good. I'll be taking a look at the notification system to allow for keys to statically define the objects that they send.

@LePips LePips merged commit da40f6a into jellyfin:main Dec 1, 2024
4 checks passed
@JPKribs JPKribs deleted the itemMetadata branch December 1, 2024 18:49
@JPKribs JPKribs changed the title [iOS] Media Item Menu | Edit Metadata [iOS] Media Item Menu - Edit Metadata Dec 2, 2024
@JPKribs JPKribs added the enhancement New feature or request label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants