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

Configurations in Sources #338

Merged
merged 8 commits into from
Feb 20, 2023
Merged

Configurations in Sources #338

merged 8 commits into from
Feb 20, 2023

Conversation

kuhnchris
Copy link
Contributor

This PR will do the following:

  • Add new parameters to the source to control the yt_dlp flags:
  • Embed metadata (add_metadata)
  • Embed thumbnails (pp: EmbedThumbnail)
  • Enable/disable SponsorSkip implementation (currently always-on)
  • Enable category selection for SponsorSkip

❗ This is still a draft! There are still places that need to be touched up, like the overview
❕ This also includes a custom implementation of a multi-checkbox for django, who thought that this problem still persists after all these years I started working with django...

New "Update"/"Add" source:
image

Old metadata/embed-less OGG (apparently metadata is already included by default...):
image
image

New fancy embed in OGG:
image

Why all this?
Funkwhale really like to check for embeded info and media (covers), so, since most music uploads on youtube are already pretty much a slideshow of the album covers, might aswell use them directly.

@meeb
Copy link
Owner

meeb commented Feb 18, 2023

Thanks, I was going to implement the feature flags but you've beaten me to it so excellent work :) This PR is still marked as draft, are you still working on it?

@meeb
Copy link
Owner

meeb commented Feb 18, 2023

Ah I see it still has debug prints etc. in the code so I assume this is indeed draft. Let me know when you're ready for a proper code check. Thanks!

@meeb
Copy link
Owner

meeb commented Feb 18, 2023

Also note your migration in this PR now needs to follow on from 0015_auto_20230213_0603 (your migration introduced by your other PR) as I've merged that one.

@kuhnchris
Copy link
Contributor Author

Sorry for the late reply, yes, this PR is still a draft as I was still working on figuring out the final deployment - I will re-pull and merge main before sending it in for review, sure. Should be ready by the end of the weekend.

@meeb
Copy link
Owner

meeb commented Feb 18, 2023

No problem, I appreciate the effort. I was asking because I had to merge what I had already pulled in into a release as yt-dlp required an update and an immediate release. Certainly not going to chase someone doing open source contributions!

@kuhnchris
Copy link
Contributor Author

No problems at all, just trying to communicate a timeline. ;-)

@kuhnchris kuhnchris marked this pull request as ready for review February 18, 2023 13:04
@kuhnchris
Copy link
Contributor Author

Ready for review, added the data to the overview aswell.
image
Also, had some weird corner-case where the string was a bit of an issue, added that corner case to the special widget.
Do you want me to split the code for the widget and model/field into a separate python file @meeb ?

Cheers 🥂

@meeb
Copy link
Owner

meeb commented Feb 18, 2023

Nice, thanks!

If you could put the extra fields into a fields.py it would be appreicatd, the models.py is already vast so anything to make it more sensible is probably a good idea.

Also you need to modify your migration, both 0015_auto_20230214_2052 (this PRs migration) and 0015_auto_20230213_0603 (the manual_skip) migration both depend on 0014_alter_media_media_file. This might work now but will make future migrations more difficult.

If you can modify this migration to be 16 and depend on the skip_migration migration it would return migrations to be a single chain which is a lot more sensible for the future.

@kuhnchris
Copy link
Contributor Author

Sure, can fix that, will push a update later today for this. Also, yes, models.py is already pretty big, maybe we should split them into a models/ folder with files for each model, since searching is already pretty difficult in there.
I will at least break out fields.py and let you know. Cheers!

@meeb
Copy link
Owner

meeb commented Feb 19, 2023

I think probably for now just break out the fields and leave the models all in models.py. They're due a larger refactor at some point anyway.

@kuhnchris
Copy link
Contributor Author

kuhnchris commented Feb 19, 2023

As you wish.

  • Moved the Fields (form + db) to fields.py and cleaned up models.py again.
  • Removed the merge migration and re-ordered the auto one to be the next in line.
  • tested it with manage.py migrate on a new database

Hope that works 👍

Is there any kanban board or any list on which features you'd still like to see for a certain release, so we (contributors) can focus on those? (not trying to scope creep, we can open a separate issue for that aswell).

Cheerrs

@meeb
Copy link
Owner

meeb commented Feb 20, 2023

Nice, thanks. I'll merge and have a test! Thanks for your work on this, most appreciated.

@meeb meeb merged commit e4c0f0e into meeb:main Feb 20, 2023
@meeb
Copy link
Owner

meeb commented Feb 20, 2023

There is a (legacy) project board here https://github.com/meeb/tubesync/projects?type=classic but I've not updated it for a while. If you can see that I'll make a "could be suitable for helpful contributors to work on" column and stuff some issues in there if you're interested? Providing you're OK with me reviewing things I've no issue at all with people making good contributions to the project.

I'll find time at some point to review the "new" GitHub projects as well as I've never seen them before until I just clicked on the projects tab :)

meeb added a commit that referenced this pull request Feb 20, 2023
@meeb
Copy link
Owner

meeb commented Feb 20, 2023

As per 2639d91 I've changed the custom field to a textfield as MySQL is fixed to charfields being 255 in length causing an issue when the concatenated sponsors field is >255 chars for people using MySQL as a back-end. I assume this doesn't affect anything else in your PR?

See #345 for discussion.

@kuhnchris
Copy link
Contributor Author

Should not affect anything as long as all the space is sufficient for all the sponsor types (should be fine at the moment).
Alternatively we can also change it to an OneToMany field (which creates another table), just wanted to avoid that, but I leave it up to you which way you prefer to use. 👍

@kuhnchris
Copy link
Contributor Author

There is a (legacy) project board here https://github.com/meeb/tubesync/projects?type=classic but I've not updated it for a while. If you can see that I'll make a "could be suitable for helpful contributors to work on" column and stuff some issues in there if you're interested? Providing you're OK with me reviewing things I've no issue at all with people making good contributions to the project.

I'll find time at some point to review the "new" GitHub projects as well as I've never seen them before until I just clicked on the projects tab :)

Sure, if anything, I'd request you review the things we do. ;-)
And yes, a column like "Open" or "Not yet started" or "Open for contributors" would be fine, and if you want, I could merge those over to the new Github Projects, if you want and do not have the time to do it for now. ;-)
Cheers 🥂

@meeb
Copy link
Owner

meeb commented Feb 20, 2023

Should not affect anything as long as all the space is sufficient for all the sponsor types (should be fine at the moment). Alternatively we can also change it to an OneToMany field (which creates another table), just wanted to avoid that, but I leave it up to you which way you prefer to use. +1

Is there a sensible way to migrate to a OneToMany field now while keeping existing users preferences?

Edit: the usual way to represent variable flags in models is with a bitfield I suppose, but that's not natively supported in Django either. Database tables, whiile overkill, is probably the most "Django-y" way to add this to the project.

@meeb
Copy link
Owner

meeb commented Feb 20, 2023

Can you also take a peak at #345 ? Probably need to do on release testing against Postgres, SQLite and MySQL given they're all supported backends and have slightly annoying variances in features.

@kuhnchris
Copy link
Contributor Author

Should not affect anything as long as all the space is sufficient for all the sponsor types (should be fine at the moment). Alternatively we can also change it to an OneToMany field (which creates another table), just wanted to avoid that, but I leave it up to you which way you prefer to use. +1

Is there a sensible way to migrate to a OneToMany field now while keeping existing users preferences?

Edit: the usual way to represent variable flags in models is with a bitfield I suppose, but that's not natively supported in Django either. Database tables, whiile overkill, is probably the most "Django-y" way to add this to the project.

Exactly, hence why I did it as a comma-separated list.
We can migrate by writing a custom migration (.py), creating the new field, migrate the data over (split by ',' and then assign), and then remove the old field (i.e. 3 migration files).

@kuhnchris
Copy link
Contributor Author

Can you also take a peak at #345 ? Probably need to do on release testing against Postgres, SQLite and MySQL given they're all supported backends and have slightly annoying variances in features.

Hmm, would it help if we add tests for something like that in the future, to avoid things like this breaking?
I wonder, since I would have thought the tests would at least also react to those issues, but I suppose I only had the local sqlite instead of a container with Postgres and MariaDB.

I will test those changes with mysql/mariaDB and postgres in the future, I'll probabbly add some test container CI thing if that is fine by you.

@meeb
Copy link
Owner

meeb commented Feb 20, 2023

Yeah even just running migrate on a blank database then test against all 3 database back-ends will probably catch most of these issues so that would be a definite improvement.

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