#4418 - Newforms Media, ready for commit?
Russell Keith-Magee
Attached to #4418 is the latest version (v3) of the newforms Media
patch. It incorporates some suggestions made before I disappeared on
my grand adventure last month.
For those not keeping track, the idea of this patch is to enable a
newforms widget to define the javascript/css that it requires; then,
when a form is generated that uses that widget, it is possible to
determine the minimum set of javascript/css that is required to render
the form. More details on the ticket; example usage in the regression
test.
Other than needing some documentation, I think this is pretty close to
being ready for committal. At last count, Jacob was +1 to the idea,
Malcolm said the idea was growing on him but he hadn't looked at the
patch. Any feedback (especially objections to committing) would be
appreciated.
Changes in this version of the patch:
- It handles https, as well as https absolute paths.
- <script> tags are closed with a </script> rather than being <script /> tags.
- CSS definitions are now dictionaries, rather than lists. This allows
for CSS for different media types. See the regression tests for
examples of usage.
- Inherited widgets now inherit the media of their parents, unless overridden.
- MultiWidgets inherit the media definitions of their component widgets.
- Fixed a problem with the addition operator on Media objects.
I have also attached the same patch with some extra bits for the
newforms-admin branch. With this patch applied:
- FormSets, FieldSets and AdminForms can be interrogated for media,
just like Forms
- Any application can reference an 'AdminDateWidget' or
'AdminTimeWidget' and they will get all the javascript definitions
required to render a pretty calendar/time widget
- Date/Time widgets now always appear on inline forms when required.
Previously, they wouldn't be displayed unless the base form also
required the widget.
Yours,
Russ Magee %-)
Jacob Kaplan-Moss
> Attached to #4418 is the latest version (v3) of the newforms Media
> patch.
I like -- +1!
Jacob
Brian Rosner
<freakb...@gmail.com> said:
This is an excellent patch and does solve many good issues as well as
add some good new functionality. One thing I want to point it is
ticket #4398. You might want to make sure that is also incorporated.
--
Brian Rosner
http://www.brosner.com/blog
Adrian Holovaty
> patch. It incorporates some suggestions made before I disappeared on
> my grand adventure last month.
I see there are some not-insignificant changes to the admin site; it'd
be much easier if this were a patch against the newforms-admin branch,
as that branch has changed much of that admin code. What do you say?
Also, what happens when a form has defined a field called "media"?
Other than that, this patch needs some documentation to
docs/newforms.txt before it can be checked in.
Adrian
--
Adrian Holovaty
holovaty.com | djangoproject.com
Russell Keith-Magee
>
> On 7/9/07, Russell Keith-Magee <freakb...@gmail.com> wrote:
> > Attached to #4418 is the latest version (v3) of the newforms Media
> > patch. It incorporates some suggestions made before I disappeared on
> > my grand adventure last month.
>
> I see there are some not-insignificant changes to the admin site; it'd
> be much easier if this were a patch against the newforms-admin branch,
> as that branch has changed much of that admin code. What do you say?
That was going to be my next question - whether to push this into
trunk or newforms-admin. In playing with the code last night, I was
thinking that newforms-admin might be a better idea, so we can work
out the little kinks before we push the API into trunk.
Should I take that as clearance to push this into newforms-admin?
> Also, what happens when a form has defined a field called "media"?
I haven't tried this explicitly, but the behavior should be no
different to a form that has a field called 'clean', 'reset', or uses
any other method name that is on the BaseForm class.
> Other than that, this patch needs some documentation to
> docs/newforms.txt before it can be checked in.
Granted. I just wanted to make sure that the interface was solid
before I started writing.
Russ %-)
Russell Keith-Magee
>
> This is an excellent patch and does solve many good issues as well as
> add some good new functionality. One thing I want to point it is
> ticket #4398. You might want to make sure that is also incorporated.
Thanks for the heads up on that. I'm pretty sure I have that covered,
but I'll make certain.
Yours,
Russ Magee %-)
Gary Wilson
> Any feedback (especially objections to committing) would be
> appreciated.
First, great work! I think all the kiddies will like it. Won't be long
before Django has a Scriptaculous widget set, I bet :)
Are we fine with having two ways to add media?
The two methods are as a property, using a getter method:
class MyWidget(Widget):
...
def _get_media(self):
return Media({
'css' = {'all': ('/path/to/css',)},
'js' = ('/path/to/js',),
})
media = property(_get_media)
and as an inner class:
class MyWidget(Widget):
...
class Media:
css = {'all': ('/path/to/css',)}
js = ('/path/to/js',)
While the inner class version looks a little nicer, it has been brought
up [1] that defining media as an inner class is less flexible (mainly
because of the lack of inheritance capabilities).
Just throwing out an idea here, maybe we could accommodate this problem
with something like:
class MyWidget(Widget):
...
class Media:
css_extend = {'all': ('/path/to/css2',)}
js = ('/path/to/js',)
Where, "<media_type> =" would replace media defined in base class(es)
and "<media_type>_extend =" would extend media defined in base class(es).
Of course, this would be exchanging the complexity of supporting two
ways to define media with the complexity of looking at base class(es) if
there is a <media_type>_extend being used. Although, this is already
being done when no media is defined so maybe it wouldn't take too much
code to determine this. Thing is, we would be doing all this to get
inheritance, which we get for free if using a property instead.
-----
On another note, in line 114 of django/newforms/widgets.py there is:
media_property and media_property or Media(media_definition)
is this, perhaps, supposed to be:
media_property or media_definition and Media(media_definition)
Gary
[1]
http://groups.google.com/group/django-developers/browse_frm/thread/58fc9cab819c08b9/
Russell Keith-Magee
>
> Russell Keith-Magee wrote:
> > Any feedback (especially objections to committing) would be
> > appreciated.
>
> First, great work! I think all the kiddies will like it. Won't be long
> before Django has a Scriptaculous widget set, I bet :)
That was the general idea (although I wasn't looking specifically at
Scriptaculous...) :-)
> Just throwing out an idea here, maybe we could accommodate this problem
> with something like:
>
> class MyWidget(Widget):
> ...
>
> class Media:
> css_extend = {'all': ('/path/to/css2',)}
> js = ('/path/to/js',)
>
> Where, "<media_type> =" would replace media defined in base class(es)
> and "<media_type>_extend =" would extend media defined in base class(es).
Hrm. In my original work, I was looking specifically at widgets, and
inheritance wasn't such a big deal. However, in tooling around with
newforms-admin, it occurred to me if we're going to be consistent, we
should using a the same media definition in the ModelAdmin as we do
for a widget. However, this will require inheritance of media - the
existing implementation for the js keyword adds to the base media.
As for fixing this - I was thinking along similar lines to you, but
introducing a new keyword, rather than making the existing keywords
'magic':
class MyWidget(Widget):
...
class Media:
extend = ('css','js')
js = /some/form/javascript.js'
extend=False -> don't inherit anything
extend=('css',) -> only inherit css
extend undefined -> inherit everything
I'm still working through some of the details - the metaclass gets
really nasty when you have a class-based DSL extending a property on
the base class. I'll report back when I have something working.
> Of course, this would be exchanging the complexity of supporting two
> ways to define media with the complexity of looking at base class(es) if
> there is a <media_type>_extend being used. Although, this is already
> being done when no media is defined so maybe it wouldn't take too much
> code to determine this. Thing is, we would be doing all this to get
> inheritance, which we get for free if using a property instead.
If you think about it, it isn't really two syntaxes - the class-based
media declaration is just a DSL shorthand for defining a media as a
property. It would be nice to not need the method/property version -
it would certainly make the metaclass stuff easier, and would be more
efficient - but there are a few existing cases (such as in the
ModelAdmin) where there is more logic required than a simple list of
paths.
> On another note, in line 114 of django/newforms/widgets.py there is:
>
> media_property and media_property or Media(media_definition)
>
> is this, perhaps, supposed to be:
>
> media_property or media_definition and Media(media_definition)
PEP 308 lists <condition> and <expression1> or <expression2> as the
syntax for a one line if-then-else; I'm just following that
convention, with the proviso that condition and expression1 are the
same.
In this case, your statement evaluates the same, but I'd rather follow
the convention to avoid confusion.
Russ %-)