I found the structure of l10n_client.js rather weird, especially in comparison to other projects
afaict, it puts the main function in Drupal.behaviors (as it should), but it puts the child functions and settings in Drupal.l10nClient, and it does this by pulling 1 major jquery extend on the Drupal object... it seems cheaper to just set it straight on the object instead of extending it, this namespace shouldn't be used by anything else anyway
so while I was at it, I reformed it to the usual Drupal.behaviors.l10nClient = function(context) { ... };
and Drupal.behaviors.l10nClient.toggle = function(state) { ... }
which makes for a much easier to read file
I also cached $('#l10n-client')
as $client
in most places, so the selector doesn't need to be re-evaluated every time, things like $('#l10n-client .labels .toggle')
was reformed to $client.find('.labels .toggle')
which should also boost performance a tiny bit
you'll also notice I created a reference to Drupal.behaviors.l10nClient as self
so Drupal.behaviors.l10nClient.toggle(state)
can be called as self.toggle(state)
, making it a lot less verbose
I also noticed that it was creating userSelection
as a global variable, which is just bound to conflict sometime
there's still a lot of room for improvement, some selectors like #l10n-client-form are still being called a ton of times and the settings should probably act as defaults, merging in with Drupal.settings.l10nClient, so if any module has set them explicitly, it would override the defaults
but I didn't want to make the patch too huge, which it probably already is... oh well
attached patch should apply to the 6.x dev
Comment | File | Size | Author |
---|---|---|---|
#52 | 862590-52-cleanup-optimization-js.patch | 21.36 KB | SebCorbin |
#47 | l10n_client-optimization-862590-46.patch | 17.7 KB | seutje |
#48 | l10n_client-optimization-862590-48.patch | 17.73 KB | seutje |
#45 | l10n_client-optimization-862590-45.patch | 17.59 KB | seutje |
#42 | l10.patch | 17.58 KB | droplet |
Comments
Comment #1
Gábor HojtsyGenerally looks good, although you seem to have not used "self" consistently. I just cut a new D6 release for the module since it had important fixes and improvements in the dev version that people were not actively using. Now I'd like to sync up the D7 version, since it has a very significant lag behind the D6 version in terms of bug fixes ported, and then we can look at making these changes in D6 and D7 too I'd say. Agreed that the patch is big enough (and we need to make the changes in D6 and D7 in parallel).
Comment #2
seutje CreditAttribution: seutje commentedso postponed until we got the D6 & D7 versions synced?
Comment #3
Gábor HojtsyWell, I did lots of work on this today, and now it is definitely not far :) Only #329858: Uninstall code is missing and #361147: Support for different text groups is outstanding. The first is minimal, the second is ported but needs some work (its close).
Comment #4
seutje CreditAttribution: seutje commentedawesome, do you think I should keep working on this in parallel or perhaps split it up into several stages so it isn't one big bulky patch?
Comment #5
Gábor HojtsySeveral stages would be very helpful, also easier to apply/port to D6 and D7 simultaneously.
Comment #6
seutje CreditAttribution: seutje commentedroger!
I'd say getting rid of that global would be first priority, it's been poking my eyes out :P
Comment #7
Gábor Hojtsy@seutje: looking forward to the improvements :)
Comment #8
seutje CreditAttribution: seutje commentedcrap, totally forgot about this :x
I'll grab a clone right now for on the train so I don't forget!
Comment #9
seutje CreditAttribution: seutje commentedglobal be gone!
Comment #10
seutje CreditAttribution: seutje commentedcached
$('#l10n-client')
selector and changed 2 instanced of$('#l10n-client #l10n-client-search-filter-clear')
to$('#l10n-client-search-filter-clear')
, being a (by definition) unique ID, I think we can assume it will only occur within the clientComment #11
seutje CreditAttribution: seutje commentedthis is probably a good start, might continue this on my commute back home
Comment #12
seutje CreditAttribution: seutje commentedforgot to use the cached selector in 2 instances in the patch in #10
1st attached patch fixes that
2nd attached patch also caches stringlist selector
both patches sort of assume the previous ones went in
bastard coworkers keep giving me a lift home, cutting down commute time :P
Comment #13
seutje CreditAttribution: seutje commentedalso cached the form selector. Again, this assumes the previous ones went in
as u've prolly noticed by now, having the thing live in multiple locations within the Drupal object makes it rather annoying to reuse variables.
next step will prolly be to put everything in
Drupal.behaviors.l10nClient
, have the sub-functions be methods and the variables be properties of that object, that way we could reference it internally likeself.form
andself.client
which should make it a lot easier to readthis would also theoretically allow other modules/scripts to manipulate this by referencing
Drupal.behaviors.l10nClient.form
or something...Considering the location within the object would change and most of the indentation would change, I'm afraid this next step will be a rather large chunk no matter how I go about it
Comment #14
Gábor HojtsyOk, great, thanks. Let's stop here for a bit, so we can apply these to Drupal 6 and 7 and then continue :)
Comment #15
seutje CreditAttribution: seutje commentedhokai, u can prolly just merge them into the 7.x branch, right?
or should I make ports for each of them?
Comment #16
seutje CreditAttribution: seutje commentedhmz, looks like line 71 of the 6.x branch (
$('#l10n-client #search-filter-clear').focus();
) and line 73 of the 7.x branch ($('#l10n-client #l10n-client-search-filter-clear').focus();
) will prolly break a seamless merge for the patch in #10and I just realised I've been patching the 7.x branch... >_<
if u want me to reroll the patch in #10 for the 6.x branch, lemme know
Comment #17
Gábor HojtsyOk, well. Looks like your patches did not apply to the Drupal 7 version either. Here is a roll of your patches (I think) to the Drupal 7 version. However, it breaks at least the initial bar behavior, the bar looks odd (with all the header text) and it does not open/close on click. Don't have lots of time to debug now, but it definitely worked before the patch. Tried to roll all your patches into one, so this is all of the above (that is since #9) rolled into one applying to the Drupal 7 branch. Can we (a) make this work (b) commit (c) get it ported to Drupal 6 (d) commit, (e) then work on further improvements :) Thank you!
Comment #18
seutje CreditAttribution: seutje commentedoh my, sorry about that, not sure what went wrong
is it maybe because I based it on the git.drupalcode.org repo?
will look into this as soon as I find a minute :x
Comment #19
droplet CreditAttribution: droplet commentedIs that execute before DOM ready ?? so $client catch nothing
Comment #21
Gábor Hojtsy@sutje: could not find time to get back to this? It would be great if you could! Thanks!
Comment #22
seutje CreditAttribution: seutje commentedk, another try
getting rid of the extend first this time, should apply to the 7.x branch of the git.drupalcode.org
Comment #23
seutje CreditAttribution: seutje commentedgot rid of global
complete + incremental patch
Comment #24
seutje CreditAttribution: seutje commentedshallow cache of the client object
complete + incremental
Comment #25
droplet CreditAttribution: droplet commentedmore optimization
Comment #26
seutje CreditAttribution: seutje commentednice, thanks!
should prolly also cache those repeating
$(this)
calls in the submit handlerComment #27
unegro CreditAttribution: unegro commentedThis don't fix the d7 version,
It crashes the Seven Theme Javascripts.
I also get this error
$.cookie is not a function
switch($.cookie('Drupal_l10n_client')) {
Comment #28
Gábor Hojtsy@unegro: for that, see #522646: jquery.cookie.js incompatible with Apache mod_security by default.
Comment #29
droplet CreditAttribution: droplet commentedalso cached
$(this),
$l10nClientForm
and use this keyword
and cached $('#l10n-client') in Drupal.l10nClient.ClientObj()
Comment #30
unegro CreditAttribution: unegro commentedIs this patch for drupal 7?
I don't find the dirs a/l10n_client.js b/l10n_client.js
I dont understood the command in line 5 of your patch...
diff --git a/l10n_client.js b/l10n_client.js
index df08e05..377d4f7 100644
--- a/l10n_client.js
+++ b/l10n_client.js
thanks
Comment #31
droplet CreditAttribution: droplet commented@ugegro,
are you review my patch ? If not, Google "how to apply a patch"
It's patch won't fix any error in this module but make the code more clean & better. If you face problem within module, crate a new issue.
Comment #32
seutje CreditAttribution: seutje commented@Droplet: thanks man, looking a lot cleaner!
@unegro: see docs on git apply -> http://www.kernel.org/pub/software/scm/git/docs/git-apply.html
Comment #33
Gábor HojtsyI'm afraid I just broke the patch with #1184960: Add context support to edit and save strings., which was important to commit soon sorry :) Can you please update the patch with changes from there included? Thanks!
Comment #34
droplet CreditAttribution: droplet commentedOK.
- added #1184960 changed
- use .class to styling element
I left one because it have buggy. I will fix on this new issue: #1195928: rendering bug on IE > 6
Comment #35
droplet CreditAttribution: droplet commentedget rid of Tabs
Comment #36
the666bbq CreditAttribution: the666bbq commented@Gabor #28
I don't think this has anything to do with apache security since you can use $.cookie just before you add the anonymous function to Drupal.behaviors.l10nClient (87) but not within this function.
The least you can say is that cookie.js is indeed loaded but that scope to jquery with cookie functionality is lost inside the function.
try
$.cookie('testMe','bake cookie');
alert($.cookie('testMe'));
and you can see that it works at line 86 (but still breakes at line 101) and that the snippet breaks if you put it inside the function.
"cookie is not a function" can break other scripts as reported by others.
Comment #37
the666bbq CreditAttribution: the666bbq commentedwas there a new version of l10n_client 6.x-2.0 today ? Seems fixed.
Comment #38
seutje CreditAttribution: seutje commentedquit hijacking this issue please
Comment #39
JacinePlease, please do this a different way.
Comment #40
seutje CreditAttribution: seutje commentedthat line was untouched, only reason it gets -'d and +'d is cause the indentation is different
although I agree that using an inline style isn't ideal, I deliberately didn't make any changes like that
the changes I did make shouldn't change anything in behavior, but are mainly about performance and readable code
adding changes like that would make this issue infinitely more complex
Comment #41
gagarine CreditAttribution: gagarine commentedAgre with #40 if we start to wan to fix the all JS in one issue... is not going to end. This patch fix mostly readability, let's commit it and see how bad the dev break :P.
BTW dev is 2011-Jun-21 so is not going to impact to anyone.
Comment #42
droplet CreditAttribution: droplet commentedOkay. Reroll the patch with CSS Cleaup.
This is following all Drupal JS way: #1415788: Javascript winter clean-up
Comment #43
seutje CreditAttribution: seutje commentedwhen did our closure change? Don't get me wrong, I prefer it like that, but is this now a Core standard?
Comment #44
droplet CreditAttribution: droplet commentedWhich closure change? The caches object?
or this ?
This is some new change from #1089300: Clean up drupal.js. (AFAIK, it's not a new standard.)
In Javascript world, it's not a bad practice.
Comment #45
seutje CreditAttribution: seutje commentedThanks for the link. I was just wondering when Drupal at large made the switch, now I can justify using it at work ;)
Fixed $l10nClient leaking into global namespace when using keybinds.
This got me thinking, maybe we should just stick it on the Drupal.l10nClient namespace so you don't define it in each function that needs it, but just have the attach handler set Drupal.l10nClient.$client and then you can just use that single instance.
attached patch only fixes the namespace leak, I'll cook up the other one in a second
Comment #46
droplet CreditAttribution: droplet commentederm. yup :)
Comment #47
seutje CreditAttribution: seutje commentedokay, I went for Drupal.l10nClient.$l10nClient and also changed $self to $l10nClient, for consistency.
Now there is only 1 call to $('#l10n-client'), in the once() callback it is stored in Drupal.l10nClient.$l10nClient and also in $l10nClient in the local scope.
Also grouped a couple var statements that had a function call in the middle (the toggle in the attach).
Also noticed we introduced a bug, the toggle handler expects either the strings "1" or "0", or boolean true or false, but for keyboard shortcuts, we're sending it integers 1 and 0. Fixing that right now by typecasting it to a boolean and removing the string cases.
Comment #48
seutje CreditAttribution: seutje commentedThink I got it, had to do some parsing and checking on the cookie, because cookies only return a string and the string '0' evaluates to true. Simply parsing it wasn't enough, because if the cookie isn't set, it'll return
null
, which parses toNaN
.attached patch fixes the bug we introduced with keybindings not properly triggering the toggle.
I'd say this is about good to go, no? Perhaps make the toggle function more ambiguous, so when you don't specify a state, it'll just toggle to "the other state", or should that be a separate issue?
Comment #49
seutje CreditAttribution: seutje commentedalso, is this
hidden
class still being used for anything? since we're using collapsed/expanded nowComment #50
droplet CreditAttribution: droplet commentedis it better if we convert toggle() into IF-ELSE.
I don't dig into the code deeply but I think .hidden do not use anywhere.
Comment #51
Gábor Hojtsy@seutje: hidden should be gone once #1181356: Class "hidden" for l10n-client DIV is bad naming is committed.
Comment #52
SebCorbin CreditAttribution: SebCorbin commentedRe-rolled and applied changes like #50 and #1181356: Class "hidden" for l10n-client DIV is bad naming
Comment #53
SebCorbin CreditAttribution: SebCorbin commentedTested and committed