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

[tvOS] Media Item Menu - Refresh / Delete Items #1348

Merged
merged 10 commits into from
Dec 10, 2024

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Dec 8, 2024

Summary

This should be the full extent of tvOS item management. I can't imagine that anyone want to manually write metadata or replace images from their TV. Instead, this should allow refreshing metadata if data is missing or some information is wrong. Additionally, if enabled, this should allow for deleting items which was a popular ask on the Android TV side: jellyfin/jellyfin-androidtv#2368

This should almost 1 to 1 mirror iOS with some small differences:

  1. Since this is on the ActionHStack, I don't have router access to dismiss the coordinator on item Deletion. The result is the user needs to manually go back.
  2. iOS has a loading ProgressView() on the Refresh Metadata button. Since tvOS's button is hidden behind the menu, I removed the loading circle since it was giving some weird flashing (at least on the emulator)

Screenshots

Buttons - No Access vs Access Screenshot 2024-12-08 at 15 31 37
Menu Buttons Screenshot 2024-12-08 at 15 32 46
Refresh Options Screenshot 2024-12-08 at 15 33 15
Delete Warning Screenshot 2024-12-08 at 15 33 04

@LePips
Copy link
Member

LePips commented Dec 10, 2024

Since this is on the ActionHStack, I don't have router access to dismiss the coordinator on item Deletion. The result is the user needs to manually go back.

That isn't necessarily true as routers are retrieved via EnvironmentObject. For example, the following works as all items are presented with an ItemCoordinator. I did test this, however I ran into the same issue as #1090 (comment), where I had to delete the .receive(on: RunLoop.main) in DeleteItemViewModel.events. I'm still unsure why this is an issue. I also did not test iOS.

@EnvironmentObject
private var router: ItemCoordinator.Router

//...

.onReceive(deleteViewModel.events) { event in
    switch event {
    case let .error(eventError):
        error = eventError
        isPresentingEventAlert = true
    case .deleted:
        router.dismissCoordinator()
    }
}

@JPKribs
Copy link
Member Author

JPKribs commented Dec 10, 2024

That isn't necessarily true as routers are retrieved via EnvironmentObject.

Ah! My bad, yes this worked. I did have to delete the .receive(on: RunLoop.main) in DeleteItemViewModel.events While I was in there, I cleaned up some of the states / backgroundStates since this really only does item deletion and I over-engineered some non-existent scenarios when I made this.

I validated that this does work! Additionally, with the new Notifications, I no longer need the onNotification to refresh the data source. Instead, it's appropriately handled by the ViewModel which is perfect.

IMO, this is all ready to go! Only item that may need an adjustment is:

iOS has a loading ProgressView() on the Refresh Metadata button. Since tvOS's button is hidden behind the menu, I removed the loading circle since it was giving some weird flashing (at least on the emulator)

I going over some of what I've done, there are like 3 differing formats for errors. Just varying versions of the same thing that are all roughly the same but have some weird interactions. The issue I ran into working on this is one of error that I had on iOS didn't have a way to dismiss it. I wanted to just knock that out since copying and pasting clearing isn't working for me lol.

import SwiftUI

struct ErrorMessageModifier: ViewModifier {

    @Binding
    var error: Error?

    let dismissActions: (() -> Void)?

    // MARK: - Body

    func body(content: Content) -> some View {
        content
            .alert(
                L10n.error.text,
                isPresented: .constant(error != nil),
                presenting: error
            ) { _ in
                Button(L10n.dismiss, role: .cancel) {
                    error = nil
                    dismissActions?()
                }
            } message: { error in
                Text(error.localizedDescription)
            }
    }
}

So, we should just be able to call .errorMessage(error: error) and have a consistent error message across the board. Let me know if you think this is a good call! I just made it present on error != nil and dismiss resets the error back to nil. There is an optional dismiss action if we need anything else.

Looks like this:
Simulator Screenshot - iPhone 16 Pro - 2024-12-10 at 10 42 38

…data fails to load, and add errorMessage on failed events.

MARK sections: Var/Func always unless only Body and Var/Lets only if there are several of varying types / functions.
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.

Looks great!

@LePips LePips merged commit 548d35b into jellyfin:main Dec 10, 2024
4 checks passed
@JPKribs JPKribs deleted the tvOSItemManagement branch December 10, 2024 21:52
ddrccw added a commit to ddrccw/Swiftfin that referenced this pull request Jan 26, 2025
* 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 <[email protected]>
@JPKribs JPKribs added the enhancement New feature or request label Feb 7, 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