Opened 4 years ago

Last modified 17 months ago

#15231 new Cleanup/optimization

Admin DateTimeShortcuts + Inlines performance

Reported by: fabianbuechler Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In the admin, stacked or tabular inlines reinitialize the DateTimeShortcuts widgets for date or time fields whenever an inline row/form is added or removed.

This has serious performance problems if your inline has a couple of date/time fields per row.
About 95% of JS execution time is spent on the reinitialization of DateTimeShortcuts resulting in continious JS timeouts in even the most modern browsers.

A couple of days ago I've offered to fix this in the django-devleopers list:
http://groups.google.com/group/django-developers/browse_thread/thread/ff460085a3bfb12e/

I've rewritten admin/DateTimeShortcuts.js and calendar.js and combinded them in a new datetimeshortcuts.js (+ minified version), which is jQuery based and uses 1.4.2 event delegation.
This way, inlines do not need to reinitialize the DateTimeShortcuts widgets at all.

The usage of the new datetimeshortcuts.js is as follows:

By default, all .fieldsets are dts-enabled by the following call at the bottom of datetimeshortcuts.js

  $("fieldset.module").datetimeshortcuts();

Users can use the plugin as well, for their own fields, specifing field selectors and enabling or disabling the today, calendar, now and clock widgets separately:

  $('my-selector').datetimeshortcuts({
      date_fields: "input:text.date_from, input:text.date_to",
      time_fields: "input:text.time_from, input:text.time_to",
      enable_calendar: true,
      enable_today: true,
      enable_clock: false,
      enable_now: false
  });

In the admin's base.html, I've defined a basic set of options for DateTimeShortcuts, which override the default options of datetimeshortcuts.js:

  $('body').data('datetimeshortcuts_defaults', {
      enable_calendar: true,
      enable_today: true,
      enable_clock: true,
      enable_now: true
  });

It would be great to add a variable to Django's settings to define these in your settings.py and include the values via a template tag in the base.html.
I don't know if you are willing to do this, so I didn't prepare it for this patch.

A patch is ready and attached.

Attachments (6)

patch_admin_datetimeshortcuts.2.diff (60.5 KB) - added by fabianbuechler 4 years ago.
patch_admin_datetimeshortcuts.diff (60.2 KB) - added by fabianbuechler 4 years ago.
patch for current trunk (>= r16402)
patch_admin_datetimeshortcuts__1.3.diff (60.2 KB) - added by fabianbuechler 4 years ago.
patch for 1.3 tag
ticket15231.2.diff (59.2 KB) - added by fabianbuechler 3 years ago.
Patch for 1.4 master
ticket15231.diff (59.2 KB) - added by fabianbuechler 3 years ago.
Patch for 1.4 master
ticket15231_stable1.4.x.diff (59.2 KB) - added by fabianbuechler 3 years ago.
Patch for stable/1.4.x

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 follow-up: Changed 4 years ago by airstrike

  • Patch needs improvement set

Personally, I couldn't get the admin media prefix to work until I made a small change to the javascript. Maybe there's something wrong with my setup, but now I have:

(function($) {
    
    $.fn.datetimeshortcuts = function(options) {
        // variables set in base.html
        var datetimeshortcuts_defaults = $('body').data('datetimeshortcuts_defaults');
        
        // extend default settings with user options
        var o = $.extend({
                // admin_media_prefix: $('body').data('admin_media_prefix')
                admin_media_prefix: window.__admin_media_prefix__
            },

        // (...)

And it finally works for me.

In any way, this is an excellent patch and I can see a lot of users benefiting from it right away. Hopefully we can make it to 1.3!

comment:3 in reply to: ↑ 2 Changed 4 years ago by fabianbuechler

Replying to airstrike:

Personally, I couldn't get the admin media prefix to work until I made a small change to the javascript. Maybe there's something wrong with my setup, but now I have:

   admin_media_prefix: $('body').data('admin_media_prefix'

the admin_media_prefix you refer to is being set in the base.html template. Before it was set as window.__admin_media_prefix__, with my patch it is $('bdoy').data('admin_media_prefix').

Can you check if the patch modified your base.html template or if you have overwritten it?

Otherwise, thanks for reviewing!

comment:4 follow-up: Changed 4 years ago by airstrike

I have indeed overwritten the base.html template. Actually, I just patched my own app to see if it would work, rather than starting a new app from the patch. Perhaps the case is that it's not particularly clear to me (and maybe other users) how to set admin_media_prefix The Right Way. window.admin_media_prefix is what I found googling around.

On a side note, +1 on making the move to jQuery UI. Readability and flexibility are two key items that come to mind as strong arguments in favor of that decision.

(Meanwhile, I'd love to use jQuery UI even if it never makes it to Django officially, so if you happened to make one for yourself (that you didn't submit as patch), please share! My javascript skills are lackluster at best, so adapting your code would take hours that I can't afford to spend at the moment.)


EDIT: missing a couple of words

Last edited 4 years ago by airstrike (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 4 years ago by fabianbuechler

Replying to airstrike:

I have indeed overwritten the base.html template. Actually, I just patched my own app to see if it would work, rather than starting a new app from the patch. Perhaps the case is that it's not particularly clear to me (and maybe other users) how to set admin_media_prefix The Right Way. window.admin_media_prefix is what I found googling around.

Just for clarification:
The admin_media_prefix is being set in the base.html template, because that is the least likely to be overwritten (usually it is just being extended).
It is necessary to set this variable in a Django template, because there you can create the link between the variables known in Python code and the JavaScript which knows nothing about these variables.

Since I've rewritten the DateTimeShortcuts thingy using jQuery, I've also changed the way the admin_media_prefix variable is stored from window.__admin_media_prefix__ to jQuery('body').data('admin_media_prefix').

Maybe this would require some backwards compatibility fix - what do you think?

On a side note, +1 on making the move to jQuery UI. Readability and flexibility are two key items that come to mind as strong arguments in favor of that decision.

I'd vote for that too, but I'll better bring that topic up in the dev mailinglist some day.
The jQuery UI datepicker is propably far more advanced by now (in terms of browser compatibility, keyboard navigation, etc.) and the time-picker widget is far less useful than it could possibly be, imho.

(Meanwhile, I'd love to use jQuery UI even if it never makes it to Django officially, so if you happened to make one for yourself (that you didn't submit as patch), please share! My javascript skills are lackluster at best, so adapting your code would take hours that I can't afford to spend at the moment.)

I have not integrated jQuery UI by now because I wanted to make the move to jQuery first and separately, as I thought that would give the patch greater changes to be accepted. :)
Once a descision has been made to integrate it, I would definitely like to help.

Changed 4 years ago by fabianbuechler

comment:6 Changed 4 years ago by fabianbuechler

  • Patch needs improvement unset

Update patch to add some minor improvements to calendar/clock closing behaviour (duplicate patch due to me forgetting to check "replace attachment, sorry).
Unset "Patch needs improvement" flag because airstrike's problems are based upon his overwritten base.html template.

comment:7 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to Cleanup/optimization

Changed 4 years ago by fabianbuechler

patch for current trunk (>= r16402)

Changed 4 years ago by fabianbuechler

patch for 1.3 tag

comment:8 Changed 4 years ago by fabianbuechler

  • Easy pickings unset
  • UI/UX unset

Just fixed a minor bug in the calendar popup.
Updated patches for 1.3 (patch_admin_datetimeshortcuts__1.3.diff) and current trunk (>= r16402, patch_admin_datetimeshortcuts.diff).

Last edited 4 years ago by lukeplant (previous) (diff)

Changed 3 years ago by fabianbuechler

Patch for 1.4 master

Changed 3 years ago by fabianbuechler

Patch for 1.4 master

Changed 3 years ago by fabianbuechler

Patch for stable/1.4.x

comment:9 Changed 3 years ago by fabianbuechler

Attached new patches for 1.4 stable branch and 1.4 master.

comment:10 Changed 3 years ago by anonymous

Updated patch on request of apollo13 for current master (1.5+). See https://github.com/django/django/pull/618

comment:11 Changed 17 months ago by timo

  • Patch needs improvement set

Patch no longer applies cleanly.

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