search

ShokoAnime / Shokofin

Repository for Shokofin, a plugin that brings Shoko to Jellyfin.
https://shokoanime.com
MIT License
140 stars 17 forks source link

Add sortable checkboxes for description source #42

Closed fearnlj01 closed 4 months ago

fearnlj01 commented 9 months ago

Hopefully this should be a somewhat passable implementation of converting the description source option to being driven by orderable checkboxes (at least, in the opening state of the PR, just for further development purposes).

The changes to the plugin settings include one new setting, and one re-purposed setting.

At a minimum, with these (client side only) changes implemented, the plugin settings render as expected, and there should hopefully be no further need for any significant new client side changes (for the sortable checkboxes at least).

By no means is this even slightly near ready to be merged, but all things must start somewhere (not helped by my own lack of ability).

fearnlj01 commented 4 months ago

Rebased the changes on top of master now. I've not double checked that things work as expected, just manually resolved merge conflicts so don't put any faith in this!

I'll review further in the morning and advise if I think I'll be able to handle the C# side of things.

fearnlj01 commented 4 months ago

Further update just to note that it's not fully working as expected... But it is saving as expected.

(I have very quickly tested the changes, and despite issues displaying the config, it's sent to the server as expected, and then used as expected for fetching descriptions)

Will return to this later today!

fearnlj01 commented 4 months ago

I think this is hunky dory now... Will request a review once I test the setting migration (that I completely forgot I had tried to setup...)

fearnlj01 commented 4 months ago

If we don't care for setting migration at all... The PR can be simplified, and the new configuration entry, DescriptionSourceList can return back to being DescriptionSource.

revam commented 4 months ago

Personally I don't care since we don't have a framework in place (yet) to update the xml before it's parsed. And 4.0.0 will be a breaking change, so putting a note in that the users need to go through their plugin settings is probably mandatory at this point, since none of the other new settings have a migration (yet at least).

fearnlj01 commented 4 months ago

Marking back as a draft as the below shouldn't be happening when the settings page is rendered... image

Once fixed will mark as ready for review again.

fearnlj01 commented 4 months ago

Okay, I've tested it a fair bit now and I'm happy that it works as expected.

The capability for Migrations is now removed, probably best not to introduce "hacks" to migrate... At the very least, instead of continuing with DescriptionSource, this PR now uses DescriptionSourceList, so there should be no issues with invalid configurations, and it will simply return to default settings for existing users.