Opened 14 years ago

Closed 8 years ago

Last modified 13 months ago

#15667 closed New feature (fixed)

Implement template-based widget rendering

Reported by: Bruno Renié Owned by:
Component: Forms Version: dev
Severity: Normal Keywords: form-rendering
Cc: carl@…, idan@…, Jannis Leidel, ironfroggy@…, Paul Oswald, Bruno Renié, trebor74hr@…, gregor@…, JMagnusson, dmclain, mathieu.agopian@…, tom@…, tgecho, dguardiola@…, mmitar@…, philipe.rp@…, Gwildor Sok, cmawebsite@…, slav0nic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Following this proposal on django-dev, this ticket tracks the status of replacing the widgets rendering code with a template-based system.

The proposal is based on an existing implementation, django-floppyforms. The api provides several ways of extending a widget:

  • Widget.template_name: the name of the template used to render the widget
  • Widget.get_context_data(): a way to inject additional context data
  • Widget.get_context(name, value, attrs=None): this method calls get_context_data() and provides the basic context variables: attrs, hidden, name, required, type.

I'm actively working on a patch and will attach it to the ticket as soon as I can so that the implementation and extension points can be discussed.

Attachments (1)

test.py (1.2 KB ) - added by Anssi Kääriäinen 14 years ago.
Performance test

Download all attachments as: .zip

Change History (65)

comment:1 by Carl Meyer, 14 years ago

Cc: carl@… added

comment:2 by Idan Gazit, 14 years ago

Cc: idan@… added

comment:3 by Jannis Leidel, 14 years ago

Cc: Jannis Leidel added

comment:4 by Calvin Spealman, 14 years ago

Cc: ironfroggy@… added

comment:5 by Paul Oswald, 14 years ago

Cc: Paul Oswald added

comment:6 by Julien Phalip, 14 years ago

Triage Stage: UnreviewedAccepted

This would be a very welcome feature addition, provided that the performance drawbacks would be kept to a minimum (as noted in the django-dev thread).

comment:7 by Bruno Renié, 14 years ago

Cc: Bruno Renié added

Right, so I have a patch but it's too large for Trac :). I'll be using a github branch instead:

https://github.com/brutasse/django/compare/15667-template-widgets

All the regressiontest changes are whitespace-related, or changes in the widget attrs ordering (name="foo" value="bar" vs value="bar" name="foo").

A few comments on the curent implementation:

  • ClearableFileInput no longer has its 'template_with_initial' and 'template_with_clear' arguments. This breaks a widget in the admin but I haven't started the admin widget migration yet. The question is whether this is supposed to be a public API or not.
  • The RadioSelect "renderer" API should be deprecated. I left it unchanged (although I added PendingDeprecationWarnings), it still works but it's not doing any template-based rendering. The templates give us enough flexibility so I'm in favor of starting its deprecation.
  • I added a template loader that is added to the list of loaders, this way the templates are always available and they can be overridden in project- or app-provided templates. Template caching works as expected, the cached loader also caches the forms templates.
  • I chose to explicitly add the input's "name" and "type" to the template context instead of leaving it in the "attrs" dictionary. I like it this way but I can put it back in the attrs if we decide to.

TODO:

  • Topic documentation and widget reference (I'll make a draft but I will need help here :)
  • Convert the admin widgets
  • Add tests for the new widgets API (make sure altering template_name, get_context_data and get_context work as expected)

I'll keep reporting my progress here and will update my branch on github. As always, comments welcome :)

in reply to:  7 comment:8 by Carl Meyer, 14 years ago

Replying to brutasse:

Right, so I have a patch but it's too large for Trac :). I'll be using a github branch instead:

https://github.com/brutasse/django/compare/15667-template-widgets

This looks great!

  • ClearableFileInput no longer has its 'template_with_initial' and 'template_with_clear' arguments. This breaks a widget in the admin but I haven't started the admin widget migration yet. The question is whether this is supposed to be a public API or not.

They aren't arguments, just class attributes. And they are not documented (I avoided documenting them specifically because I was hoping we could soon get rid of them by moving to template-based widgets). It's probably worth a note in the release notes in this patch for anyone who had a subclass overriding them, but since they aren't documented I don't think we need to provide the full deprecation path for them.

  • The RadioSelect "renderer" API should be deprecated. I left it unchanged (although I added PendingDeprecationWarnings), it still works but it's not doing any template-based rendering. The templates give us enough flexibility so I'm in favor of starting its deprecation.

Agreed.

  • I added a template loader that is added to the list of loaders, this way the templates are always available and they can be overridden in project- or app-provided templates. Template caching works as expected, the cached loader also caches the forms templates.

So I think this is the trickiest part here. I'm hesitant to introduce another implicit coupling between conceptually-separate components of Django, and it makes me cringe a bit that there's now no way to use the Django template language without having this forms template loader added automatically. That said, I don't have any other brilliant ideas for how to make this "just work" transparently when people upgrade. Needs more thought.

On a more minor note, I'm not a big fan of how you're finding the forms_template_dir, relying on the relative filesystem location of django/template/loaders/forms.py and django/forms. I think it might be better to import the django.forms module and use its file attribute?

  • I chose to explicitly add the input's "name" and "type" to the template context instead of leaving it in the "attrs" dictionary. I like it this way but I can put it back in the attrs if we decide to.

I prefer this as well; more explicit.

  • Topic documentation and widget reference (I'll make a draft but I will need help here :)

I'm happy to help with this at some point.

I'll keep reporting my progress here and will update my branch on github. As always, comments welcome :)

Great work, thanks!

comment:9 by Robert Lujo, 14 years ago

Cc: trebor74hr@… added

comment:10 by Gregor Müllegger, 14 years ago

Cc: gregor@… added

comment:11 by Luke Plant, 14 years ago

Type: New feature

comment:12 by JMagnusson, 14 years ago

Cc: JMagnusson added
Severity: Normal

comment:13 by dmclain, 14 years ago

Cc: dmclain added

comment:14 by Bruno Renié, 14 years ago

Easy pickings: unset

Some progress since the last update. The changes can be seen here:

https://github.com/brutasse/django/compare/15667-template-widgets

Carl, I integrated your suggestion to use django.forms.__file__ to locate the forms templates. I have to agree the coupling between the template system and the forms library may be an issue. Maybe this should be discussed on django-dev but the GSoC proposal about form rendering is going to make this coupling even stronger if it gets merged.

Almost all admin widgets have been converted. All tests now pass, I added a test for the admin RadioInput widget. Now for some discussion:

  • the MultiWidget class has a format_output() method that joins the outputs from all of its widgets. The base MultiWidget just does "".join(outputs) but in the admin (see AdminSplitDateTimeWidget) it inserts some markup between the outputs. I'm not sure of the best way to move this to the templates... Remove format_output and make MutiWidget use a higher-level template? Leave it as it is and let the users decide if they want format_output to use the template system or string interpolation?
  • Same for RelatedFieldWidgetWrapper. This basically wraps a widget, gets its output and adds the "Add Another" link next to it. I'd be tempted to make its render() method render the widget a separate template for the "Add Another" button, then join the outputs. Not sure if there are better options.

After these two points, the roadmap is to add tests for the extension points (get_context_data, get_context, template_name) and document it as a public API. Let me know if I overlooked something!

in reply to:  14 ; comment:15 by Carl Meyer, 14 years ago

Replying to brutasse:

I have to agree the coupling between the template system and the forms library may be an issue. Maybe this should be discussed on django-dev but the GSoC proposal about form rendering is going to make this coupling even stronger if it gets merged.

I don't think it's acceptable to have this enforced two-way coupling between forms and templates in the long run, but we do need to provide a deprecation-smoothed upgrade path. Here's my proposal:

  1. Add the form-defaults template loader to TEMPLATE_LOADERS in the startproject template settings.py, but not to the global_settings TEMPLATE_LOADERS default.
  2. Document that you need to have the form-defaults template loader listed in TEMPLATE_LOADERS, if you want to have any of Django's default form/widget templates available (we should leave open the option that someone wants to provide all the form/widget templates they will use themselves, and not use any of the defaults built in to Django, though I think this is pretty unlikely in practice).
  3. Have temporary code that automatically adds the form template loader (last in priority order) if it is not listed in TEMPLATE_LOADERS, like you do now.
  4. If the form template loader is not explicitly listed in TEMPLATE_LOADERS and loads a template, have it issue PendingDeprecationWarning. This would result in lots of warnings, which is tricky. I don't want to make the warning module-wide, since, in the case I mentioned above, if people don't need the form-defaults loader because they provide all their own templates, they should be able to escape the warning. So it should only warn if the form loader actually loads a template. We might want to consider using some internal state on the loader to make it a first-time-only warning? I don't really like that, but it's better than hundreds of warnings or issuing the warning at module scope, IMO.
  5. When this deprecation cycle concludes in Django 1.6, remove the automatically-add-form-template-loader code. At this point, if people don't have the form template loader in TEMPLATE_LOADERS they'll just get a TemplateNotFound error for any form template they need but don't provide themselves.

I think this solution would be able to adapt to the increased use of templates in Gregor's GSoC proposal without any trouble.

Does this sound like a reasonable approach? Anything I'm missing?

  • the MultiWidget class has a format_output() method that joins the outputs from all of its widgets. The base MultiWidget just does "".join(outputs) but in the admin (see AdminSplitDateTimeWidget) it inserts some markup between the outputs. I'm not sure of the best way to move this to the templates... Remove format_output and make MutiWidget use a higher-level template? Leave it as it is and let the users decide if they want format_output to use the template system or string interpolation?

I think we should leave format_output() in place for backwards compatibility, and have its default implementation render a MultiWidget template. Moving forward, format_output can become an internal implementation detail, and overriding the template becomes the documented way to customize MultiWidget rendering.

  • Same for RelatedFieldWidgetWrapper. This basically wraps a widget, gets its output and adds the "Add Another" link next to it. I'd be tempted to make its render() method render the widget a separate template for the "Add Another" button, then join the outputs. Not sure if there are better options.

I wouldn't implicitly join the outputs of two different templates; I'd have a related-field-widget-wrapper template that takes the wrapped widget's rendered HTML in its context and can wrap it however it wants.

in reply to:  15 ; comment:16 by Bruno Renié, 14 years ago

Replying to carljm:

Replying to brutasse:

I have to agree the coupling between the template system and the forms library may be an issue. Maybe this should be discussed on django-dev but the GSoC proposal about form rendering is going to make this coupling even stronger if it gets merged.

I don't think it's acceptable to have this enforced two-way coupling between forms and templates in the long run, but we do need to provide a deprecation-smoothed upgrade path. Here's my proposal:

  1. Add the form-defaults template loader to TEMPLATE_LOADERS in the startproject template settings.py, but not to the global_settings TEMPLATE_LOADERS default.
  2. Document that you need to have the form-defaults template loader listed in TEMPLATE_LOADERS, if you want to have any of Django's default form/widget templates available (we should leave open the option that someone wants to provide all the form/widget templates they will use themselves, and not use any of the defaults built in to Django, though I think this is pretty unlikely in practice).
  3. Have temporary code that automatically adds the form template loader (last in priority order) if it is not listed in TEMPLATE_LOADERS, like you do now.
  4. If the form template loader is not explicitly listed in TEMPLATE_LOADERS and loads a template, have it issue PendingDeprecationWarning. This would result in lots of warnings, which is tricky. I don't want to make the warning module-wide, since, in the case I mentioned above, if people don't need the form-defaults loader because they provide all their own templates, they should be able to escape the warning. So it should only warn if the form loader actually loads a template. We might want to consider using some internal state on the loader to make it a first-time-only warning? I don't really like that, but it's better than hundreds of warnings or issuing the warning at module scope, IMO.
  5. When this deprecation cycle concludes in Django 1.6, remove the automatically-add-form-template-loader code. At this point, if people don't have the form template loader in TEMPLATE_LOADERS they'll just get a TemplateNotFound error for any form template they need but don't provide themselves.

I think this solution would be able to adapt to the increased use of templates in Gregor's GSoC proposal without any trouble.

Does this sound like a reasonable approach? Anything I'm missing?

I like that, I agree it's much better than 2-way coupling. I'll have a look at the warnings and see how verbosity can be avoided.

  • the MultiWidget class has a format_output() method that joins the outputs from all of its widgets. The base MultiWidget just does "".join(outputs) but in the admin (see AdminSplitDateTimeWidget) it inserts some markup between the outputs. I'm not sure of the best way to move this to the templates... Remove format_output and make MutiWidget use a higher-level template? Leave it as it is and let the users decide if they want format_output to use the template system or string interpolation?

I think we should leave format_output() in place for backwards compatibility, and have its default implementation render a MultiWidget template. Moving forward, format_output can become an internal implementation detail, and overriding the template becomes the documented way to customize MultiWidget rendering.

So a MultiWidget gets a template attribute? (it currently doesn't have any). By default it'd be {% for output in rendered_widget %}{{ output }}{% endfor %} . If it gets implemented like this I'd say format_output() can be deprecated in favor of the API other widgets already have (get_context(), etc), unless there is a use case for format_output() that can't be implemented using a template. That'd be mostly for consistency if we want a MultiWidget to have the same customization hooks as every other widget.

  • Same for RelatedFieldWidgetWrapper. This basically wraps a widget, gets its output and adds the "Add Another" link next to it. I'd be tempted to make its render() method render the widget a separate template for the "Add Another" button, then join the outputs. Not sure if there are better options.

I wouldn't implicitly join the outputs of two different templates; I'd have a related-field-widget-wrapper template that takes the wrapped widget's rendered HTML in its context and can wrap it however it wants.

Ok, I'll do it this way. Thanks for your feedback :)

comment:17 by Mathieu Agopian, 14 years ago

Cc: mathieu.agopian@… added

in reply to:  16 comment:18 by Carl Meyer, 14 years ago

Replying to brutasse:

  • the MultiWidget class has a format_output() method that joins the outputs from all of its widgets. The base MultiWidget just does "".join(outputs) but in the admin (see AdminSplitDateTimeWidget) it inserts some markup between the outputs. I'm not sure of the best way to move this to the templates... Remove format_output and make MutiWidget use a higher-level template? Leave it as it is and let the users decide if they want format_output to use the template system or string interpolation?

I think we should leave format_output() in place for backwards compatibility, and have its default implementation render a MultiWidget template. Moving forward, format_output can become an internal implementation detail, and overriding the template becomes the documented way to customize MultiWidget rendering.

So a MultiWidget gets a template attribute? (it currently doesn't have any). By default it'd be {% for output in rendered_widget %}{{ output }}{% endfor %} .

Yes, I that seems to me like the most consistent and flexible option.

If it gets implemented like this I'd say format_output() can be deprecated in favor of the API other widgets already have (get_context(), etc), unless there is a use case for format_output() that can't be implemented using a template. That'd be mostly for consistency if we want a MultiWidget to have the same customization hooks as every other widget.

Yes, I agree that format_output could probably start a deprecation path. It's a little bit tricky - what you actually want to deprecate is a user-defined subclass relying on format_output being called. I'd need to take a closer look at the code to see the best way to do this.

I'm looking forward to getting this in; Gregor's GSoC on form rendering will depend on some of this (particularly the form template loader), and I don't want to hold up his progress (if necessary we can just apply this patch in his branch -- I'd rather do that than apply this to trunk before it's ready -- but it would simplify things if we just had this in trunk). If you have a latest-state-of-the-patch with some tasks remaining to be done, let me know and I'll see if I can help.

comment:19 by Bruno Renié, 14 years ago

Ok, so according to the chat we had on IRC with Carl an Jannis, format_output and renderer / get_renderer should be deprecated since it's too hard to keep them along with the new API and they're not considered public. This is done in the latest version of the patch:

https://github.com/brutasse/django/compare/15667-template-widgets

If a MultiWidget defines a format_output() method, calling render() raises a DeprecationWarning and uses template-based rendering anyway. Same with the renderers:

  • providing renderer as a kwarg during RadioSelect instanciation raises DeprecationWarning
  • the RadioSelect class doesn't have a renderer attribute anymore
  • any call to get_renderer() raises a DeprecationWarning and returns RadioFieldRenderer
  • RadioSelect's render() will always use templates

The admin's RelatedFieldWidgetWrapper now has its template attribute and get_context method.

The template loader still needs to be worked on (Carl's comment 15 above). I'll give it a go tomorrow night, CEST.

by Anssi Kääriäinen, 14 years ago

Attachment: test.py added

Performance test

comment:20 by Anssi Kääriäinen, 14 years ago

I did some performance testing using default settings of 1.3.0, except that caching template loader was enabled.

I had two test, in the first one I had a a form with 11 integer fields. using ipython %timeit, I got 1.76ms per loop with 1.3.0, and 3.72ms per loop with code downloaded from GitHub. This seems good enough.

The second test is more worrisome. In this test I had 11 MultipleChoiceFields, each having 100 choices. The result without this patch was 25ms per loop, with the patch 160ms per loop. With just 1 field with 100 choices the results were 2.27ms vs 14.6ms. So, when using a 100 choice field the performance difference is 6.5x. Performance test code attached.

comment:21 by Anssi Kääriäinen, 14 years ago

Just to add some more info, I tried template based widgets with Jinja2 (version 2.1.1). I got very encouraging results, though it might be that I am doing something wrong as the results seem to be too good to be true. I got 1.47ms with Jinja2 vs 2.27 with Django 1.3.0. Most likely the speed difference is because Jinja2 does not localize or escape some of the values in the template, while the 1.3.0 implementation does. I haven't looked more into this.

The conclusion that can be drawn from this example is that if template compilation with comparable speeds to Jinja2 can be included in Django, then this feature needs not be slower than current implementation at all.

For completeness, here are the steps needed to reproduce the jinja2 rendering test. I added the following code into widgets.py:

from jinja2 import Environment, PackageLoader
env = Environment(loader=PackageLoader('foobar', 'templates'))

def jinja_render_to_string(context):
    template = env.get_template("select.html")
    return template.render(**context)

and I added a directory foobar/templates/ to my testing root folder with select.html in it (you need to change attrs.items to attrs.items() in the first line of the template). Context is gotten by changing return Context(context) to return context in Widget.get_context(). Then, Select.render() just calls jinja_render_to_string(context).

in reply to:  15 comment:22 by Bruno Renié, 14 years ago

Replying to carljm:

I don't think it's acceptable to have this enforced two-way coupling between forms and templates in the long run, but we do need to provide a deprecation-smoothed upgrade path. Here's my proposal:

  1. Add the form-defaults template loader to TEMPLATE_LOADERS in the startproject template settings.py, but not to the global_settings TEMPLATE_LOADERS default.
  2. Document that you need to have the form-defaults template loader listed in TEMPLATE_LOADERS, if you want to have any of Django's default form/widget templates available (we should leave open the option that someone wants to provide all the form/widget templates they will use themselves, and not use any of the defaults built in to Django, though I think this is pretty unlikely in practice).
  3. Have temporary code that automatically adds the form template loader (last in priority order) if it is not listed in TEMPLATE_LOADERS, like you do now.
  4. If the form template loader is not explicitly listed in TEMPLATE_LOADERS and loads a template, have it issue PendingDeprecationWarning. This would result in lots of warnings, which is tricky. I don't want to make the warning module-wide, since, in the case I mentioned above, if people don't need the form-defaults loader because they provide all their own templates, they should be able to escape the warning. So it should only warn if the form loader actually loads a template. We might want to consider using some internal state on the loader to make it a first-time-only warning? I don't really like that, but it's better than hundreds of warnings or issuing the warning at module scope, IMO.
  5. When this deprecation cycle concludes in Django 1.6, remove the automatically-add-form-template-loader code. At this point, if people don't have the form template loader in TEMPLATE_LOADERS they'll just get a TemplateNotFound error for any form template they need but don't provide themselves.

1) and 3) are done on the github branch, I'm not sure about the right way to implement 4) while making it consistent across the cached and non-cached template loaders... I'll dig a bit more but I'm open to suggestions.

comment:23 by Bruno Renié, 14 years ago

Has patch: set
Needs documentation: set

I just pushed the code that warns if the forms template loader has been automatically added:

https://github.com/brutasse/django/commit/b76dd323ce48e5e38061d59954990951653bfa0b

The missing bit is now documentation, unless there is something fundamentally wrong with the patch.

comment:24 by Carl Meyer, 14 years ago

I'll try to find some time soon to review this in depth, write up some docs, and see if there's anything we can do to bring some of the worst-case performance numbers into better shape. Thanks for all the work on the patch!

comment:25 by Bruno Renié, 14 years ago

I generated some profiling graphs of the benchmarks here:

http://media.bruno.im/render_it.png
http://media.bruno.im/render_it2.png

I was wondering if the {% if %} check in the selectmultiple widget was a bottleneck but looking at the graphs it doesn't look so. Not sure about what can be done, the {% if %} check accounts for about 1% of the rendering time. Most of the stuff going on is variable resolving and this closed loop on the render_it2 graph, there is some circular stuff here because of the {% for %} loop I guess.

comment:26 by Anssi Kääriäinen, 14 years ago

I have a feeling there is not much that can be done in this ticket to improve the speed of MultipleChoiceField (or ChoiceField for that matter). Current implementation of Django templates is just a bit slow. Maybe it would be best to bring this up in django-developers for design decision? Or mark this ticket design decision needed? As I see it, there are three ways forward:

  1. Accept the performance loss. For some users of 1.3 this can be a decision which makes their current application unusable in 1.4.
  2. Decide that the performance loss is unacceptable. In this case, this ticket should wait for a faster implementation of Django template rendering. Personally, I have high hopes that the GSOC project of Armin Ronacher will achieve this.
  3. Keep the default implementation of the performance-problematic widgets in Python. One could still use template based rendering for these widgets, but by default the python implementation would be used. This is a bit uqly, but would allow this feature to get in without big worries of performance problems for users upgrading to 1.4.

One small thing that could theoretically have an impact on performance is the usage of context.update in get_context subclass methods. This is creating a new stack entry in the context which can slow down variable resolution. But as choices are in the upmost stack entry, this should not have a big impact. Still, it would be IMHO cleaner if there would be just one stack entry in the context given to the template.

comment:27 by Carl Meyer, 13 years ago

Keywords: form-rendering added
Patch needs improvement: set
UI/UX: unset

Currently, the most up-to-date version of this patch lives in the 2011 GSoC form-rendering branch. The original GSoC branch is at https://github.com/gregmuellegger/django/compare/master...soc2011%2Fform-rendering and I've been making some further updates in https://github.com/carljm/django/compare/master...soc2011%2Fform-rendering

That branch combines the templated widgets with a new form-rendering API. The problem with the branch is still speed, and since it doesn't appear we're going to get speed improvements in the template language in the short term from Armin's GSoC, we need to look for other solutions. My planned next step is to split back out just the templated-widgets from the form-rendering branch in order to focus on seeing if we can improve that to the point where we can commit it and close this ticket. Then we can deal with the rest of the form-rendering API in a separate step.

There's also a bit of a framework for speed comparisons using djangobench here: https://github.com/carljm/formrenderbench

comment:28 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:29 by Carl Meyer, 13 years ago

This patch would also fix #16630. Not closing that as duplicate because it's more narrowly focused and can be done separately.

comment:30 by Tom Christie, 13 years ago

Cc: tom@… added

comment:31 by tgecho, 13 years ago

Cc: tgecho added

comment:32 by domguard, 13 years ago

Cc: dguardiola@… added

comment:33 by Mitar, 13 years ago

Cc: mmitar@… added

comment:34 by Felipe 'chronos' Prenholato, 12 years ago

Cc: philipe.rp@… added

comment:35 by Anssi Kääriäinen, 12 years ago

My opinion: if we want this, lets implement the default rendering in Python. We could also provide default templates and compare output in testing so that they produce the same result.

This isn't beautiful. But this way those who need absolute speed can get it. In most of cases ultimate speed isn't a requirement.

Using template based forms does make so much sense that in my opinion the code duplication is worth it.

comment:36 by Gwildor Sok, 11 years ago

Cc: Gwildor Sok added

comment:37 by Gwildor Sok, 11 years ago

Are there any updates on this subject? In my humble opinion the widgets are one of the ugliest parts of Django codewise and could logically learn a lot from how the CBV's work. Most render() methods are a bit of a mess, and this makes creating custom widgets which subclass an existing widget, even subclassing something like Input, one of the hardest parts of the general things to do with the framework. Why has nothing happened in the past two years?

comment:38 by chrismedrela, 11 years ago

See the lengthy discussion about introducing out-of-the-box support for Jinja2 on django-developers: https://groups.google.com/forum/#!topic/django-developers/Bk-22bKqCTo.

in reply to:  35 comment:39 by Carl Meyer, 10 years ago

Replying to akaariai:

My opinion: if we want this, lets implement the default rendering in Python. We could also provide default templates and compare output in testing so that they produce the same result.

This isn't beautiful. But this way those who need absolute speed can get it. In most of cases ultimate speed isn't a requirement.

Using template based forms does make so much sense that in my opinion the code duplication is worth it.

I agree that using template-based forms makes so much sense that it is worth taking the performance hit by default, but I have a different proposal for what those people should do who need "absolute speed" in template rendering: use Jinja2 instead of DTL. Several adapters are available that make Jinja2 work as a Django template loader (e.g. jingo), which means that it can be a drop-in replacement for DTL (apart from needing to update template syntax, of course). I think it's easy enough to switch to Jinja2 if you really need the speed that it is not worth us maintaining two separate widget rendering systems in Django.

comment:40 by Aymeric Augustin, 10 years ago

Master now provides first-class support for Jinja2, removing the main objection to this ticket.

comment:41 by Asif Saifuddin Auvi, 10 years ago

Version: master

in reply to:  40 comment:42 by Asif Saifuddin Auvi, 10 years ago

Replying to aaugustin:

Master now provides first-class support for Jinja2, removing the main objection to this ticket.

Hi augustin are you proposing to convert the template or widget rendering in jinja2? how will be that adopt to existing widgets? like admin?

comment:43 by Tim Graham, 10 years ago

I haven't read through all the history of this ticket, but I guess it might work something like this: by default, widgets are rendered with Django templates. If you have jinja2 configured in TEMPLATES, then widgets can be rendered with it. The main point is not to add jinja2 as a requirement for Django.

comment:44 by Asif Saifuddin Auvi, 10 years ago

Owner: changed from Bruno Renié to Asif Saifuddin Auvi
Status: newassigned

in reply to:  43 comment:45 by Asif Saifuddin Auvi, 10 years ago

Replying to timgraham:

I haven't read through all the history of this ticket, but I guess it might work something like this: by default, widgets are rendered with Django templates. If you have jinja2 configured in TEMPLATES, then widgets can be rendered with it. The main point is not to add jinja2 as a requirement for Django.

That is what I'm thinking, as aymeric says masters has first class support for jinja2 does that mean using jinja2 for form rendering and then modify the contrib apps widgets with that? for standardizing django form rendering what should be the move? stick with django or switching to jinja2 permanently? or keeping to versions of different form rendering system? 1 for backward compat and another for futureistic?

comment:46 by Tim Graham, 9 years ago

Owner: Asif Saifuddin Auvi removed
Status: assignednew

Preston is working on this.

comment:47 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added

see also #16922

comment:48 by Sergey Maranchuk, 9 years ago

Cc: slav0nic@… added

comment:49 by Tim Graham <timograham@…>, 9 years ago

In 86573861:

Refs #15667 -- Removed choices argument from some RendererMixin methods.

RendererMixin will soon be removed but this removal and the corresponding
test changes stand on their own.

comment:50 by Tim Graham, 8 years ago

Needs documentation: unset
Patch needs improvement: unset

A small issue detailed in the PR description remains, but otherwise I think the updated PR is ready for review.

comment:51 by Tim Graham, 8 years ago

Keywords: 1.10 added

comment:52 by Tim Graham, 8 years ago

Keywords: 1.10 removed
Patch needs improvement: set

We're a bit stuck on the backwards-compatibility issue, so deferring from 1.10.

comment:53 by GitHub <noreply@…>, 8 years ago

In 26d0023c:

Refs #15667 -- Fixed crash when indexing RadioFieldRenderer with ModelChoiceIterator.

Regression in 86573861a95e5a47dc7ff906443117d75b73dca1

comment:54 by Tim Graham <timograham@…>, 8 years ago

In 1213ef2b:

[1.10.x] Refs #15667 -- Fixed crash when indexing RadioFieldRenderer with ModelChoiceIterator.

Regression in 86573861a95e5a47dc7ff906443117d75b73dca1

comment:55 by Tim Graham, 8 years ago

Patch needs improvement: unset

The PR is ready for review again.

comment:56 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed

In b52c73008a9d67e9ddbb841872dc15cdd3d6ee01:

Fixed #15667 -- Added template-based widget rendering.

Thanks Carl Meyer and Tim Graham for contributing to the patch.

comment:57 by Tim Graham <timograham@…>, 8 years ago

In 4e89082f:

Refs #15667 -- Fixed form renderer test for Python 2 non-ASCII path.

comment:58 by Tim Graham <timograham@…>, 8 years ago

In 12cefee5:

Refs #15667 -- Prevented newlines in attrs.html widget rendering.

Removed the trailing newline from widget attrs.html template.
The solution may be revisited by fixing refs #9198 but not
for Django 1.11.

Thanks Dmitry Ivanchenko for the report and Preston Timmons for advice.

comment:59 by Tim Graham <timograham@…>, 8 years ago

In 0a47b67:

Refs #15667 -- Removed hardcoded icon size for related widget wrapper.

The template-based widget rendering branch was started years ago.
This is obsolete now.

comment:60 by GitHub <noreply@…>, 8 years ago

In 0f46bc67:

Fixed #27735 -- Doc'd form widget l10n change (refs #15667).

comment:61 by Tim Graham <timograham@…>, 7 years ago

In f2b69863:

Fixed #28308 -- Doc'd removal of Select.render_option() (refs #15667).

comment:62 by Tim Graham <timograham@…>, 7 years ago

In f20168e8:

[1.11.x] Fixed #28308 -- Doc'd removal of Select.render_option() (refs #15667).

Backport of f2b698631719c6df082a627b6f7ddf2d7f9fa751 from master

comment:63 by Tim Graham <timograham@…>, 7 years ago

In 2bd207a:

Refs #15667 -- Removed support for Widget.render() methods without the renderer argument.

Per deprecation timeline.

comment:64 by GitHub <noreply@…>, 13 months ago

In 6ad0dbc8:

Refs #15667 -- Added resetting default renderer when FORM_RENDERER is changed.

Note: See TracTickets for help on using tickets.
Back to Top