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] Admin Dashboard - Task Triggers, Device Management, & Device Posters #1255

Closed
wants to merge 14 commits into from

Conversation

JPKribs
Copy link
Member

@JPKribs JPKribs commented Oct 5, 2024

Issue

This is an extension of #1230

Summary

This PR is kind of a mix of low hanging Admin Dashboard changes. The primary changes are:

Task Triggers

  1. Adds the ability to see Task Triggers in the Edit view
  2. Adds the ability to delete Task Triggers
  3. Add the ability to create new Task Triggers

Device Management

  1. Adds "All Devices" view to the Admin Dashboard
  2. Adds the ability to see all Devices with active session connected to the Server
  3. Adds the ability to delete Devices
  4. Adds the ability to delete all Devices
  • Mirroring Jellyfin-Web, users cannot delete their own session from the same device
  1. Adds the ability to change the custom name for a Device
  • The current production version of the Swift-SDK does not have CustomNames. So we can set it using this but we can't see it in client. When we update, I want to change this so it uses the Custom Name if available then uses the Device Name as fallback.

Device Posters

  1. I cleaned up some of the spacing for Active Devices. The issue was that the Albums were Squares and expanded to fill the height available so it was a little too wide.
  2. I pulled all of the Device Images from Jellyfin-Web: https://github.com/jellyfin/jellyfin-web/tree/c190b1a1df0ad2755ad7114e66ce74529eff9212/src/assets/img/devices
  3. I also included a background color by pulling the color from the logos. See screenshots to see how this looks.

Screenshots - Task Triggers

Task View Task View
Delete Task Delete Task Confirm Delete
No Tasks No Tasks
Add Task Add Task
All Task Types w/ Icons Screenshot 2024-10-09 at 23 38 59

Screenshots - Device Management

Delete Single Device Swipe Confirm Same Session
Delete All Devices Same Session
Custom Name Device Tap

Screenshots - Device Posters

Active Device Poster Changes Task View
Various Poster Examples Task View

@JPKribs
Copy link
Member Author

JPKribs commented Oct 6, 2024

This is basically done but I have 2 outstanding items:

  1. I made an Int.HourLabel that goes from second to hours text. Great for what I am doing but might not be good overall? Feedback is welcome for this!
  2. I don't love how my Pickers look on the AddTaskTriggerView. It's all totally functional right now but I can't seem to find a good way to handle pickers. Especially since I want to default everything to nil.

Outside of those 2 items, I think this is good to go.

@JPKribs JPKribs reopened this Oct 7, 2024
@JPKribs
Copy link
Member Author

JPKribs commented Oct 8, 2024

I've re-based on Main the only way I know how: Poorly. But this should be up to date now with the new changes included. I also wanted to emphasize setting defaults. So, when the TriggerType is changed, values are set to a default value. I am also no longer using anything other than formatStyles for getting formatting done. There is still plenty I want to cleanup but I think this is a much better second attempt having come off of #1258.

Outstanding items, I want to:

  • Revisit this with fresh eyes later in the week to make sure there isn't anything weird I need to fix.
  • Fix the Observer so it updates the observer.task.triggers when they are updated. I was able to get this work by running the stop() then start() back to back but I need to find a better solution to this.

@JPKribs JPKribs marked this pull request as ready for review October 8, 2024 16:05
@JPKribs JPKribs requested a review from LePips October 8, 2024 16:10
@LePips
Copy link
Member

LePips commented Oct 8, 2024

I've started my changes but have some more to go. Some notable ones:

  • While referencing the web is good, we should adhere to iOS/mobile styles and have consistent design throughout the application. For the trigger deletion, instead of a button with the trash icon, we should use a swipeAction or onDelete. In our case, we want to use the former so that SwiftUI doesn't apply its default behavior for list row deletion.
  • When we are editing/adding values, we should present as a modal instead of pushing and editing the back button. In fact, the user security settings should be changed to a modal as well.
  • Since we know the server types, we don't need to consider unknown server trigger types.

I haven't gotten to everything but here are some bigger things we should change:

  • We aren't editing trigger information, nor do I really care to, so we should remove values/logic for editing a trigger in AddTaskTriggerView
  • For intervals, instead of introducing a new type I may just want us to use the HourMinutePicker. This doesn't require us to make a new type and strings and would ultimately match the Next Up Days picker we just talked about, which should be moved to a wheeled picker in the future. As an additional note, we shouldn't make new strings for minutes/hours and instead use SwiftUI's provided formatters.

@JPKribs
Copy link
Member Author

JPKribs commented Oct 9, 2024

* We aren't editing trigger information, nor do I really care to, so we should remove values/logic for editing a trigger in `AddTaskTriggerView`

* For intervals, instead of introducing a new type I may just want us to use the `HourMinutePicker`. This doesn't require us to make a new type and strings and would ultimately match the `Next Up Days` picker we just talked about, which should be moved to a wheeled picker in the future. As an additional note, we shouldn't make new strings for `minutes/hours` and instead use SwiftUI's provided formatters.

I can knock this out from my end! My only concern is that Jellyfin-Web has a pretty locked down interval which I tried to mirror. Technically, it's all ticks so we can enter anything variation we want but I'm not positive what happens if the interval doesn't match one on Jellyfin-Web? Does it actually register as something or does it output a undefined? I would argue fitting Jellyfin-Web here matters a little more since that's how most interaction with Scheduled Tasks will occur.

EDIT: I can now confirm that Jellyfin-Web handles non-standard times:
Screenshot 2024-10-09 at 18 12 54

The time formatting is still a bit weird to me so I'll take a crack at it but feel free to let me know if this isn't correct!

…. Remove leftover EditTaskTriggerView logic. Add Results/Errors to the EditScheduledTaskView. Track Progress on EditScheduledTaskView.
… struct. Creation of a ChevronInputButton object that probably needs some love...
@JPKribs
Copy link
Member Author

JPKribs commented Oct 10, 2024

I think I addressed most of the items. I removed the TaskTriggerIntervals enum. I tried out a few routes. Using the DatePicker works but limits the user to 23:59 being the longest interval they can schedule to. My current implementation probably needs to go but it's just every 15 minute segment...

I created a ServerTicks struct for going from seconds/minutes/hours/days to ticks and vice versa. In my mind, it's just easier to do that once than having random " / 10000000" when needed. Let me know if this isn't the way to approach this.

I created a ChevronInputButton. The goal of this was to extend the existing Chevron Button but instead, when clicked, it brings up an Alert with a TextField input. It works, can take multiple input types (Int, Double, String, etc) but I also think there is room to do this better. The reason I made a new button for this is this input format now exists for the TaskTriggers > RunTimeLimit and the Customize > MaxNextUpLimit. If this exists in multiple places I think it would be good to get all the button logic done there (I think that's all there now) so it's easier to roll out changes if something comes up.

Lastly, I went through some of the TODO in the TaskTriggers and added a section for In-Progress information, last run history, and errors. Those are pretty standard but let me know if there are any changes needed there.

Everything should be localized and all minutes/hours/days should be using built-in functionality now.

I think that's all of the more out-there changes! Let me know if there are any questions or changes I can assist with!

@JPKribs JPKribs marked this pull request as draft October 11, 2024 00:19

import Foundation

struct ServerTicks {
Copy link
Member

Choose a reason for hiding this comment

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

I have thought previously about making ticks more strongly typed, but as either probably something loose like typealias Ticks = Int64 or making a stronger type like this conforming as well to ExpressibleByIntegerLiteral with necessary functions and values like you've done below.

However, I would want this in the SDK so we can set the type for all tick properties during generation.

@JPKribs JPKribs changed the title [iOS] Admin Dashboard - View/Edit Task Triggers [iOS] Admin Dashboard - Task Triggers, Device Management, & Device Posters Oct 11, 2024
@JPKribs
Copy link
Member Author

JPKribs commented Oct 11, 2024

This is starting to creep up in scope. I just keep seeing things that would be quick to add. Feel free let me know if this needs to be broken out in other PRs. There are couple items that I wasn't sure about that I'd be happy to change but I might need some direction:

  1. [iOS] Admin Dashboard - Task Triggers, Device Management, & Device Posters #1255 (comment) - I'd be happy to go this route but I have some ServerTick vars I'm using now to go from Hours to Ticks, etc. Are those actually valuable or should I just be doing that as a one-off? Or, is there a way to store a binding as ticks and edit it in hours?
  2. My AddTaskTrigger > Intervals is kind of hard to visualize a good method for it. I moved away from a more static enum but I'm not sure if there is a better way to manage this. I was able to create an IntervalPicker that looks like the HourPicker but goes up to 100 hours but I'm not sure how valuable that is either.
  3. I made a ChevronInputButton with the goal of capturing the 3 places where I am using a Chevron Button to open an Alert where you can input values to a binding. I'm sure there is room to do it better plus, if we're doing this in multiple places do we want a different layout than ChevronButton for displaying numbers like this? Also, making sure this accounts for all situations I'm sure there is room to expand/improve there too.
  4. I made a DeviceType Enum that inits from a string and can output the systemImage (SVGs), clientColor, or clientTitle. I am mirroring how this is done from here. Unfortunately, this is done by parsing strings from the client / device name. I've built this out and expanded on some missing Swiftfin/Infuse naming that Jellyfin-Web was missing. That being said, is there a better way to do this than just throwing 100 strings in there? Can/Should I be doing a find or REGEX? Apple Devices have about 20 strings when parsing "iOS, tvOS, iPadOS, Infuse" would get use the same result? I also worry that creating this opens a maintain nightmare since the current web maintenance looks pretty manual just saving .svg and manually parsing app names. If App Names update, this would need to be adjusted.
  5. For DevicesViewModel, I parse it so you can't delete your own session. Should I include a state for 'You just tried to delete your own session' instead of handling that check on the View?
  6. Finally, the All Devices View has 4 popups. 2 Deletion Confirmations (Single Device & All Devices) and 2 Alerts (Failed to Delete & Custom Name Editor). Should any of these be moved to their own View? Originally, I had the name on it's own view but I moved it to an alert since it was kind of wonky passing a viewModel.

Sorry, I know this is a lot of items! Please let me know and I can split these out into other PRs. The Devices & Posters additions were just related because I was using some of the ServerTicks items at the same time.

@JPKribs JPKribs marked this pull request as ready for review October 11, 2024 05:59
Shared/Extensions/JellyfinAPI/DayOfWeek.swift Outdated Show resolved Hide resolved

deviceTask = Task { [weak self] in
await MainActor.run {
let _ = self?.backgroundStates.append(.gettingDevices)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be simplified a little by adding it to the states before creating the task?


// MARK: - Body

// TODO: Likely want to redo this but better. Needed in
Copy link
Contributor

@chickdan chickdan Oct 12, 2024

Choose a reason for hiding this comment

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

Needed in? Also how can it be done better in your eyes? A lot of TODOs in this project don't provide enough direction for contributors to help out (and I'd like to be a more active contributor).

Copy link
Member Author

@JPKribs JPKribs Oct 12, 2024

Choose a reason for hiding this comment

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

TODO: Finish TODO...

Sorry about that, I meant to pull the locations that call this button.

This is called from the TimeIntervalSection of the AddScheduledTaskTriggersView and the HomeSection of the CustomizeViewSettingsView. So, it's "Needed in..." those locations.

I detail my potential issues with this and another comment on this PR. My goal with this button is we now have two locations where we are using an alert to host a TextField used to populate a binding. My hope was to consolidate that work into a single view so we're not redoing the alert logic in multiple places. My thinking is that if we can create one good instance on a button and then use that button everywhere it's easier to maintain.

Some of what I think might be issues or something I could do better:

  • I think I need more ways to Init this? That is, do I include the text field in the alert or should I make them so if you can be passed in trailing so you could pass in a textField or a picker or... whatever is needed. Frankly, a lot of what I've done in Swift has been single function so creating something like this with a bit more of a broad potential usage, I want to make sure it's flexible.

  • UI wise, do we want to use the Chevron button to display the value? Is there a better way to display this information?

  • Lastly, I like the idea of consolidating all of the alert inputs into a single button, but I want to make sure that it's not too broad to begin with. That is, are the inputs on this button going to require code work than recreating the alert system?

Let me know if any of that doesn't make sense! I'm still newer to Swift/SwiftUI so advice is always appreciated.

@JPKribs JPKribs marked this pull request as draft October 14, 2024 03:24
@JPKribs
Copy link
Member Author

JPKribs commented Oct 14, 2024

I've got a couple items in my list of things I want to work on for this. I'm going to take another look at this later in the week with some fresh eyes. For now, I am setting this back to draft until it's fully ready.

… BEFORE attempting to delete. Creation of an Admin Session Indicator for showing if there are active sessions from the HomeView. Fix Day of Week localization based on jellyfin#1255 (comment) - Confirmed working! Created an activeSessions published value for usage with the Active Sessions Indicator. Migration of the ChevronInputButton to allow more input types than just TextFields. Creation of a Users section. Grid vs Row User Section. Allow for User Password Resets. Make sure the DeviceType backgrounds never clash with the SVG. General cleanup of previous work mostly labels and formatting based on changes mentioned above.

Finally, migrated ALL of the UserDashboard -> AdminDashboard (Since it's admin only now) and created it's own coordinator. Now available from the ActiveSessionsIndicator.
@JPKribs JPKribs closed this Oct 14, 2024
@JPKribs JPKribs deleted the EditTaskTriggers branch October 14, 2024 04:19
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.

3 participants