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

CommentFileSizeAuthor
#52 862590-52-cleanup-optimization-js.patch21.36 KBSebCorbin
#47 l10n_client-optimization-862590-46.patch17.7 KBseutje
#48 l10n_client-optimization-862590-48.patch17.73 KBseutje
#45 l10n_client-optimization-862590-45.patch17.59 KBseutje
#42 l10.patch17.58 KBdroplet
#35 l10n_client-complete5.patch13.06 KBdroplet
#34 l10n_client-complete4.patch13.06 KBdroplet
#29 l10n_client-complete3.patch12.06 KBdroplet
#25 l10.patch9.41 KBdroplet
#24 l10n_client-cache-client.patch1.09 KBseutje
#24 l10n_client-complete2.patch7.09 KBseutje
#23 l10n_client-global.patch728 bytesseutje
#23 l10n_client-complete1.patch6.25 KBseutje
#22 l10n_client-extend.patch6.24 KBseutje
#17 D7-patch-combined.patch9.46 KBGábor Hojtsy
#13 862590-l10n_client-form-selector.patch4.15 KBseutje
#12 862590-l10n_client-forgot-some-selectors.patch755 bytesseutje
#12 862590-l10n_client-cache-stringlist-selector.patch1.81 KBseutje
#10 862590-l10n_client-cache-wrapper-selector.patch4.26 KBseutje
#9 862590-l10n_client-global.patch746 bytesseutje
l10n_client.js_.cleanup.patch11.15 KBseutje
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Generally 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).

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Needs review » Postponed

so postponed until we got the D6 & D7 versions synced?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Well, 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).

seutje’s picture

seutje CreditAttribution: seutje commented

awesome, 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?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Several stages would be very helpful, also easier to apply/port to D6 and D7 simultaneously.

seutje’s picture

seutje CreditAttribution: seutje commented

roger!

I'd say getting rid of that global would be first priority, it's been poking my eyes out :P

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@seutje: looking forward to the improvements :)

seutje’s picture

seutje CreditAttribution: seutje commented

crap, totally forgot about this :x
I'll grab a clone right now for on the train so I don't forget!

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
862590-l10n_client-global.patch746 bytes

global be gone!

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
862590-l10n_client-cache-wrapper-selector.patch4.26 KB

cached $('#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 client

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Postponed » Needs review

this is probably a good start, might continue this on my commute back home

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
862590-l10n_client-cache-stringlist-selector.patch1.81 KB
862590-l10n_client-forgot-some-selectors.patch755 bytes

forgot 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

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
862590-l10n_client-form-selector.patch4.15 KB

also 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 like self.form and self.client which should make it a lot easier to read

this 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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Ok, great, thanks. Let's stop here for a bit, so we can apply these to Drupal 6 and 7 and then continue :)

seutje’s picture

seutje CreditAttribution: seutje commented

hokai, u can prolly just merge them into the 7.x branch, right?
or should I make ports for each of them?

seutje’s picture

seutje CreditAttribution: seutje commented

hmz, 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 #10

and 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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work
FileSize
D7-patch-combined.patch9.46 KB

Ok, 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!

seutje’s picture

seutje CreditAttribution: seutje commented

oh 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

droplet’s picture

droplet CreditAttribution: droplet commented 
$.extend(Drupal, {
   l10nClient: new (function() {
+    // Cache selector.
+    var $client = $('#l10n-client');

Is that execute before DOM ready ?? so $client catch nothing

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@sutje: could not find time to get back to this? It would be great if you could! Thanks!

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
l10n_client-extend.patch6.24 KB

k, another try

getting rid of the extend first this time, should apply to the 7.x branch of the git.drupalcode.org

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
l10n_client-complete1.patch6.25 KB
l10n_client-global.patch728 bytes

got rid of global
complete + incremental patch

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
l10n_client-complete2.patch7.09 KB
l10n_client-cache-client.patch1.09 KB

shallow cache of the client object
complete + incremental

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
l10.patch9.41 KB

more optimization

seutje’s picture

seutje CreditAttribution: seutje commented

nice, thanks!

should prolly also cache those repeating $(this) calls in the submit handler

unegro’s picture

unegro CreditAttribution: unegro commented
Version: 6.x-1.x-dev » 7.x-1.0-beta3

This 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')) {

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@unegro: for that, see #522646: jquery.cookie.js incompatible with Apache mod_security by default.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs work » Needs review
FileSize
l10n_client-complete3.patch12.06 KB

also cached
$(this),
$l10nClientForm
and use this keyword
and cached $('#l10n-client') in Drupal.l10nClient.ClientObj()

unegro’s picture

unegro CreditAttribution: unegro commented

Is 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

droplet’s picture

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.

seutje’s picture

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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs review » Needs work

I'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!

droplet’s picture

droplet CreditAttribution: droplet commented
Version: 7.x-1.0-beta3 » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
l10n_client-complete4.patch13.06 KB

OK.
- 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

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
l10n_client-complete5.patch13.06 KB

get rid of Tabs

the666bbq’s picture

the666bbq CreditAttribution: the666bbq commented
Version: 7.x-1.x-dev » 6.x-2.0
Category: task » bug
Priority: Normal » Major
Status: Needs review » Needs work

@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.

the666bbq’s picture

the666bbq CreditAttribution: the666bbq commented

was there a new version of l10n_client 6.x-2.0 today ? Seems fixed.

seutje’s picture

seutje CreditAttribution: seutje commented
Version: 6.x-2.0 » 7.x-1.x-dev
Category: bug » task
Priority: Major » Normal
Status: Needs work » Needs review

quit hijacking this issue please

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs review » Needs work 
+++ b/l10n_client.jsundefined
@@ -2,98 +2,100 @@
+          $('body').css('border-bottom', '22em solid #fff');

Please, please do this a different way.

seutje’s picture

seutje CreditAttribution: seutje commented

that 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

gagarine’s picture

gagarine CreditAttribution: gagarine commented
Status: Needs work » Needs review

Agre 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.

droplet’s picture

droplet CreditAttribution: droplet commented
Priority: Normal » Major
Issue tags: +JavaScript
FileSize
l10.patch17.58 KB

Okay. Reroll the patch with CSS Cleaup.

This is following all Drupal JS way: #1415788: Javascript winter clean-up

seutje’s picture

seutje CreditAttribution: seutje commented

when did our closure change? Don't get me wrong, I prefer it like that, but is this now a Core standard?

droplet’s picture

droplet CreditAttribution: droplet commented

Which closure change? The caches object?

+++ b/l10n_client.jsundefined
@@ -1,217 +1,221 @@
+(function ($, Drupal, undefined) {

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.

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
l10n_client-optimization-862590-45.patch17.59 KB

Thanks 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

droplet’s picture

droplet CreditAttribution: droplet commented

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.

erm. yup :)

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
l10n_client-optimization-862590-46.patch17.7 KB

okay, 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.

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
l10n_client-optimization-862590-48.patch17.73 KB

Think 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 to NaN.

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?

seutje’s picture

seutje CreditAttribution: seutje commented

also, is this hidden class still being used for anything? since we're using collapsed/expanded now

droplet’s picture

droplet CreditAttribution: droplet commented

is 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.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@seutje: hidden should be gone once #1181356: Class "hidden" for l10n-client DIV is bad naming is committed.

SebCorbin’s picture

SebCorbin CreditAttribution: SebCorbin commented
FileSize
862590-52-cleanup-optimization-js.patch21.36 KB

Re-rolled and applied changes like #50 and #1181356: Class "hidden" for l10n-client DIV is bad naming

SebCorbin’s picture

SebCorbin CreditAttribution: SebCorbin commented
Status: Needs review » Fixed

Tested and committed

Status: Fixed » Closed (fixed)
Issue tags: -JavaScript

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +JavaScript

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.