-
Notifications
You must be signed in to change notification settings - Fork 2
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
Content type #6
base: master
Are you sure you want to change the base?
Content type #6
Conversation
- REQUIRES MIGRATION WHICH WILL FAIL TO CONVERT PREVIOUS TAGGEDVIDEOS CATEGORY TO A CONTENTTYPE IF THE MODEL's NAME IS NOT THE SAME. - Upload files for any object. - Load TaggedVideo for an object in template. - Get all videos of a TaggedVideo by referencing .videos. - The example app is updated. - Some changes to admin. - Some things don't work yet.
- TaggedVideo.video now gives the lowest video with lowest version value instead of version=0. - When deleting TaggedVideo.video, instead of changing every videos version number by -1, we now upgrade the whole list to prevent gaps.
- Changed videoplayer's video-element's ID from '{{id}}' to 'signbank-video{{id}}' to avoid possible overlaps with video element id's. - Added param 'id' back into templatetag 'videoplayer' - Added try-blocks into TaggedVideo poster_url and get_absolute_url, in case a Video does not exist for a TaggedVideo object. - Made the example create a sample video/taggedvideo object to make it work. - Added a SampleVideo to MEDIA - Showing error messages in red in example app
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
==========================================
- Coverage 75.71% 75.29% -0.43%
==========================================
Files 10 12 +2
Lines 280 340 +60
==========================================
+ Hits 212 256 +44
- Misses 68 84 +16
Continue to review full report at Codecov.
|
Thanks for this. One problem that I'm having right now might cause issues with this change and that is how to deal with homophones - where we want to link one video to many signs. Currently we do this by linking the video to the first in a chain of homophones and look back in the chain to find the sign with the video attached. I would rather have the video linked to each homophone separately. Not sure how to achieve this without storing a reference to the video in the gloss object itself. |
I don't completely understand what you mean with that. |
We have the homophone relationship between glosses via the Relation model. The issue is that to find the video for a given sign, I might need to go through the homophone chain to find the sign that the video is attached to. This means I can't just say {% get_video_for object %} - or at least I need a method on the Gloss model that returns the gloss object that will have the video - that's more or less what we used to do. |
Just realised we already have this method Gloss.get_video_gloss - that might solve the problem. |
Okay good, I took a look on your Relation model and understand that part now, but I am not 100% sure about what exactly are homophones and if it matters is it the source or target. The Gloss.get_video_gloss seems to solve the issue though. |
So I accidentally pushed to wrong remote, so my changes can also be seen in this repositorys branch content_type.
I updated the tests and they seem to pass.
I also made some styling changes, which makes comparing the changes a bit harder, sorry for that.
This change removes
category
andtag
from TaggedVideo, and replaces them withcontent_type
andobject_id
, therefore utilizing the contenttypes framework: https://docs.djangoproject.com/en/1.11/ref/contrib/contenttypes/category
is replaced bycontent_type
, which can be used to identify Models.tag
is replaced byobject_id
, which is the id of the object in the model of itscontent_type
. So it is now possible to connect videos to objects.The unique_together of TaggedVideo is changed to
unique_together = (("content_type", "object_id"),)
There is one problem with migrations: It might be risky to move from the
tag/category
way to this new one. I tried to make migration that attempts to solve the best case scenario wherecategory
is named after a model. I am not sure if it will work, it is not tried and tested. I also noticed now that there might be potential for the migration failing due to uppercase letters in model names https://github.com/Signbank/signbank-video/blob/content_type/video/migrations/0003_auto_20170822_1023.py#L18-L19.I added one new templatetag
get_taggedvideo_for_object
which can be used to get the TaggedVideo object specified for the object you supply. It is used in a template like this:{% get_taggedvideo_for_object object as taggedvideo %}
.There are some other changes too, I added a function videos() to TaggedVideo, it should return all Videos tagged to TaggedVideo, ordered by version.
version 0
, perhaps this works better if for some reasonversion 0
does not exist.tag
andcategory
can be simply replaced withobject
.@stevecassidy
All in all this is not perfect, but if you think that some of these changes could work, we can work on them some more. I think that after these changes we could start preparing to move using signbank-video ourselves.