Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed #21987 -- Add support for custom MEDIA_TYPES to Media objects. #2562

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

Osmose commented Apr 15, 2014

Moved the MEDIA_TYPES list into the Media class and updated the methods
to support subclasses that add to the list. Because some of the methods
rely on knowing that media is stored in _media_name attributes on the
Media object, MEDIA_TYPES had to be updated to store both the type's
name and the constructor for its container.

I'm not super-happy about storing the constructor in MEDIA_TYPES, but I couldn't think of a better way to handle things that also supported __add__ and __getitem__ and didn't make any major API changes.

https://code.djangoproject.com/ticket/21987

@Osmose Osmose Fixed #21987 -- Add support for custom MEDIA_TYPES to Media objects.
Moved the MEDIA_TYPES list into the Media class and updated the methods
to support subclasses that add to the list. Because some of the methods
rely on knowing that media is stored in _media_name attributes on the
Media object, MEDIA_TYPES had to be updated to store both the type's
name and the constructor for its container.
4667ec1
Contributor

hirokiky commented May 3, 2014

Great job.

But If you combine CustomMedias having different MEDIA_TYPES, the MEDIA_TYPES of right side custom media won't affect.
A test for this problem will be like this.
The `

blarght
won't be rendered. :

diff --git a/tests/forms_tests/tests/test_media.py b/tests/forms_tests/tests/test_media.py
index d9fca5f..06c5aaf 100644
--- a/tests/forms_tests/tests/test_media.py
+++ b/tests/forms_tests/tests/test_media.py
@@ -495,6 +495,26 @@ class FormsMediaTestCase(TestCase):
 <div>/bonk</div>
 <div>blargh</div>""")
+        class AnotherCustomMedia(Media):
+            MEDIA_TYPES = Media.MEDIA_TYPES + (('anothertext', list),)
+
+            def add_anothertext(self, data):
+                if data:
+                    for path in data:
+                        if path not in self._text:
+                            self._text.append(path)
+
+            def render_anothertext(self):
+                return ['<div>%s</div>' % text for text in self._text]
+
+        new_m = m + AnotherCustomMedia(text=['blargh'])
+        self.assertEqual(new_m.render(), """<link href="/foo.css" type="text/css" media="all" rel="style
+<link href="/bar.css" type="text/css" media="all" rel="stylesheet" />
+<script type="text/javascript" src="/baz.js"></script>
+<div>biff</div>
+<div>/bonk</div>
+<div>blargh</div>""")
+

This problem might be needed some decision of design.
I suggest not to allow combining different CumtomMedias. because It is complicated to combine MEDIA_TYPES. If you do, you should consider the case when the name of MEDIA_TYPE was conflicted.

@berkerpeksag berkerpeksag commented on the diff Jan 16, 2015

django/forms/widgets.py
@python_2_unicode_compatible
class Media(object):
+ MEDIA_TYPES = (('css', dict), ('js', list))
@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

MEDIA_TYPES is not a module level constant anymore, I'd rename it to media_types.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor
media_types = [('css', dict), ('js', list)]

looks better to me.

@berkerpeksag berkerpeksag commented on the diff Jan 16, 2015

django/forms/widgets.py
@@ -79,8 +77,8 @@ def absolute_path(self, path, prefix=None):
def __getitem__(self, name):
"Returns a Media object that only contains media of the given type"
- if name in MEDIA_TYPES:
- return Media(**{str(name): getattr(self, '_' + name)})
+ if name in [mt[0] for mt in self.MEDIA_TYPES]:
@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

The size of self.MEDIA_TYPES won't be large, but it would be good to pre-compute this.

@berkerpeksag berkerpeksag commented on the diff Jan 16, 2015

django/forms/widgets.py
@@ -79,8 +77,8 @@ def absolute_path(self, path, prefix=None):
def __getitem__(self, name):
"Returns a Media object that only contains media of the given type"
- if name in MEDIA_TYPES:
- return Media(**{str(name): getattr(self, '_' + name)})
+ if name in [mt[0] for mt in self.MEDIA_TYPES]:
+ return self.__class__(**{str(name): getattr(self, '_' + name)})
@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

type(self)(...) looks more Pythonic to me.

@berkerpeksag berkerpeksag commented on the diff Jan 16, 2015

docs/topics/forms/media.txt
@@ -341,3 +341,35 @@ form::
<script type="text/javascript" src="http://static.example.com/animations.js"></script>
<script type="text/javascript" src="http://static.example.com/actions.js"></script>
<script type="text/javascript" src="http://static.example.com/whizbang.js"></script>
+
+Custom Media Types
+------------------
+
@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Needs .. versionadded:: 1.9.

@berkerpeksag berkerpeksag commented on the diff Jan 16, 2015

docs/topics/forms/media.txt
+
+By subclassing ``Media``, you can add support for media types beyond the
+default ``css`` and ``js`` types. One use case for this would be adding support
+for inline JavaScript.
+
+To add a custom media type, you need to add the type to the ``MEDIA_TYPES``
+tuple and add ``add_X`` and ``render_X`` methods for adding and rendering
+your custom type. For example::
+
+ from django.utils.html import format_html
+
+ class InlineJSMedia(Media):
+ # MEDIA_TYPES stores tuples of (media_name, constructor).
+ # __init__ will create an attribute self._media_name using the
+ # constructor.
+ MEDIA_TYPES = Media.MEDIA_TYPES + (('inline_js', list),)
Contributor

Osmose commented Feb 2, 2015

@berkerpeksag Thanks for the feedback! Just as a note, I mentioned in https://code.djangoproject.com/ticket/21987 that I was going to hold off making changes to this PR until https://code.djangoproject.com/ticket/22298 is resolved, as there seems to be little point in landing this work if Media is going to change a bunch or go away anyway.

Owner

timgraham commented Mar 13, 2015

Closing for now based on previous comment.

@timgraham timgraham closed this Mar 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment