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

Files: 
CommentFileSizeAuthor
#52 862590-52-cleanup-optimization-js.patch21.36 KBSebCorbin
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#47 l10n_client-optimization-862590-46.patch17.7 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-optimization-862590-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 l10n_client-optimization-862590-48.patch17.73 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-optimization-862590-48.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 l10n_client-optimization-862590-45.patch17.59 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-optimization-862590-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#42 l10.patch17.58 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#35 l10n_client-complete5.patch13.06 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 l10n_client-complete4.patch13.06 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 l10n_client-complete3.patch12.06 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 l10.patch9.41 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 l10n_client-cache-client.patch1.09 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-cache-client.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 l10n_client-complete2.patch7.09 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 l10n_client-global.patch728 bytesseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-global.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 l10n_client-complete1.patch6.25 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 l10n_client-extend.patch6.24 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-extend.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 D7-patch-combined.patch9.46 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D7-patch-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 862590-l10n_client-form-selector.patch4.15 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-form-selector.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 862590-l10n_client-forgot-some-selectors.patch755 bytesseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-forgot-some-selectors.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 862590-l10n_client-cache-stringlist-selector.patch1.81 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-cache-stringlist-selector.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 862590-l10n_client-cache-wrapper-selector.patch4.26 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-cache-wrapper-selector.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 862590-l10n_client-global.patch746 bytesseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-global.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
l10n_client.js_.cleanup.patch11.15 KBseutje
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client.js_.cleanup.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Gábor Hojtsy’s picture

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

Status:Needs review» Postponed

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

Gábor Hojtsy’s picture

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

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

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

seutje’s picture

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

@seutje: looking forward to the improvements :)

seutje’s picture

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

StatusFileSize
new746 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-global.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

global be gone!

seutje’s picture

StatusFileSize
new4.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-cache-wrapper-selector.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Postponed» Needs review

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

seutje’s picture

StatusFileSize
new1.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-cache-stringlist-selector.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new755 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-forgot-some-selectors.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new4.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 862590-l10n_client-form-selector.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

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

Status:Needs review» Needs work
StatusFileSize
new9.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch D7-patch-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

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

seutje’s picture

StatusFileSize
new6.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-extend.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new6.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new728 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-global.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

got rid of global
complete + incremental patch

seutje’s picture

StatusFileSize
new7.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-cache-client.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

shallow cache of the client object
complete + incremental

droplet’s picture

StatusFileSize
new9.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

more optimization

seutje’s picture

nice, thanks!

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

unegro’s picture

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

droplet’s picture

Status:Needs work» Needs review
StatusFileSize
new12.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

unegro’s picture

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

@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

@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

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

Version:7.x-1.0-beta3» 7.x-1.x-dev
Status:Needs work» Needs review
StatusFileSize
new13.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new13.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-complete5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

get rid of Tabs

the666bbq’s picture

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

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

seutje’s picture

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

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

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

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

Priority:Normal» Major
Issue tags:+JavaScript
StatusFileSize
new17.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Okay. Reroll the patch with CSS Cleaup.

This is following all Drupal JS way: #1415788: [Meta] javascript winter clean-up

seutje’s picture

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

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

StatusFileSize
new17.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-optimization-862590-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

StatusFileSize
new17.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-optimization-862590-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new17.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch l10n_client-optimization-862590-48.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

droplet’s picture

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

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

SebCorbin’s picture

StatusFileSize
new21.36 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

SebCorbin’s picture

Status:Needs review» Fixed

Tested and committed

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

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