Opened 14 years ago
Last modified 11 years ago
#15231 new Cleanup/optimization
Admin DateTimeShortcuts + Inlines performance
Reported by: | Fabian Büchler | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
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)
Change History (17)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
follow-up: 3 comment:2 by , 14 years ago
Patch needs improvement: | set |
---|
comment:3 by , 14 years ago
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!
follow-up: 5 comment:4 by , 14 years ago
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
comment:5 by , 14 years ago
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.
by , 14 years ago
Attachment: | patch_admin_datetimeshortcuts.2.diff added |
---|
comment:6 by , 14 years ago
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 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
by , 13 years ago
Attachment: | patch_admin_datetimeshortcuts.diff added |
---|
patch for current trunk (>= r16402)
comment:8 by , 13 years ago
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).
comment:10 by , 12 years ago
Updated patch on request of apollo13 for current master (1.5+). See https://github.com/django/django/pull/618
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:
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!