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 Metadata #1323

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

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

@@ -81,6 +81,8 @@ extension Notifications.Key {
static let didFailMigration = NotificationKey("didFailMigration")

static let itemMetadataDidChange = NotificationKey("itemMetadataDidChange")
Copy link
Member

Choose a reason for hiding this comment

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

You're correct that this notification was mainly used for the user info since that would change after playback. Now these two notifications mean the same thing and do the same thing. I don't think we should have both, but we can change this notification to send the item instead in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

For ItemViewModel, is this okay to consolidate [String: String] or Item.Id?

    Notifications[.itemMetadataDidChange].publisher
        .sink { [weak self] notification in
            if let userInfo = notification.object as? [String: String] {
                if let itemID = userInfo["itemID"], itemID == item.id {
                    Task { [weak self] in
                        await self?.send(.backgroundRefresh)
                    }
                } else if let seriesID = userInfo["seriesID"], seriesID == item.id {
                    Task { [weak self] in
                        await self?.send(.backgroundRefresh)
                    }
                }
            } else if let itemId = notification.object as? String, itemId == self?.item.id {
                Task { [weak self] in
                    await self?.send(.backgroundRefresh)
                }
            }
        }
        .store(in: &cancellables)

Copy link
Member Author

@JPKribs JPKribs Nov 28, 2024

Choose a reason for hiding this comment

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

I made the switch. It all seems to work both for userData updates and with metadata updates. The only interaction that's kind of off is if you edit an episode, going back to the season it's incorrect at the EpisodePicker level:

Simulator.Screen.Recording.-.iPad.Pro.13-inch.M4.-.2024-11-27.at.20.26.23.mp4

The only part of this that I'm not positive on is, I had to create another action on the itemViewModel for "replace" that takes the full baseItemDto and replaces it from a notification.

…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!

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