On Mon, 2011-07-04 at 20:46 +0530, Thilanka Kaushalya wrote:

Hi Rouslan,

On Mon, Jul 4, 2011 at 8:04 PM, Rouslan Placella rouslan@placella.comwrote:

...

Hi Thilanka (and everyone else),

You filed bug #3350790 on the tracker, which I looked into and fixed earlier. This also gave me a chance to look at the AJAXification of the index editor, which as I can see from your blog you have deemed completed. Anyway, I found some issues in your implementation and thought you might want to know about them.

First, the JS errors from bug #3350790 are gone, but the functionality from checkIndexName() function in indexes.js only works in the QA_3_4 branch. This is due to the AJAXification that you did. That function was written in plain JS, when there was no jQuery or AJAX. Line 44 in that file should have been a dead giveaway that the function was about to break (onload = checkIndexName;), since a browser does not fire an onload event for AJAX requests. Anyway, I think that the checkIndexName() function should be rewritten in jQuery and then called inside the AJAX request from tbl_structure.js. Also there is some inline JS in tbl_indexes.php, which you may want to rewrite in jQuery if you will be rewriting checkIndexName().

Now for the fun stuff...

Try this:

• go to `sakila` db
• go to `actor` table
• go to structure tab
• click on "Edit" link for the primary key
• in the ajax dialog click 'Cancel'
• click again on "Edit" link for the primary key
• in the dialog click on "Go" (to add more rows to the table)

Expected result:

• 1 row is added to the table of indices

Actual result:

• The table of indices is cloned and 1 row is added to the first table

Another thing is that the AJAX dialog should really be modal (you can make the dialog modal by adding "modal: true," to the dialog options in jQuery). Try this:

• go to `sakila` db
• go to `actor` table
• go to structure tab
• click on "Edit" link for the primary key
• without closing the AJAX dialog, move it out of the way
• click on "Edit" link for the `idx_actor_last_name` index
• inside this new dialog click "Save"

Expected result:

• the dialog for `idx_actor_last_name` is submitted

Actual result:

• the dialog for the primary key is submitted

Another bug:

• go to `sakila` db
• go to `actor` table
• go to structure tab
• click on "Edit" link for the primary key
• set all rows in the table to "--ignore--"
• click "Save"

Expected result:

• An error message

Actual result:

• An empty AJAX message

Another thing is that the new function that you wrote to handle the SPATIAL indices doesn't actually work. The problem is that its event is attached using jQuery's 'bind' method, which is unsuitable for your purposes. I think that you should use the 'live' method that allows for late binding. After you do that, you will notice that the function still doesn't work as expected. This is because in this function you assume that you will always have one instance of each element that you reference by ID. However once the DOM has registered an element with a particular ID (open a dialog), even if you destroy that element and recreate it (close dialog/ open new dialog), the DOM will still reference the original, and now non-existent, element. The best way to deal with this is to use classes, then search for objects inside some common parent element.

I guess that this is quite a mouthful of an email, hope it helps.

This is very interesting and thanks for reporting. I'll quickly go through these facts and update you.

OK, great.

BTW, before I forget, personally I'm not a big fan of the "'Save' Or Add to index 'X' column(s)" part in the fieldset footer. I think that the "Save" button should go into the bottom part of the dialog, near the "Cancel" button.

Rouslan

Le 2011-07-06 10:38, Thilanka Kaushalya a écrit :

Hi Marc and Rouslan,

You filed bug #3350790 on the tracker, which I looked into and fixed
earlier. This also gave me a chance to look at the AJAXification of the
index editor, which as I can see from your blog you have deemed
completed. Anyway, I found some issues in your implementation and
thought you might want to know about them.

First, the JS errors from bug #3350790 are gone, but the functionality
from checkIndexName() function in indexes.js only works in the QA_3_4
branch. This is due to the AJAXification that you did. That function was
written in plain JS, when there was no jQuery or AJAX. Line 44 in that
file should have been a dead giveaway that the function was about to
break (onload = checkIndexName;), since a browser does not fire an
onload event for AJAX requests. Anyway, I think that the
checkIndexName() function should be rewritten in jQuery and then called
inside the AJAX request from tbl_structure.js. Also there is some inline
JS in tbl_indexes.php, which you may want to rewrite in jQuery if you
will be rewriting checkIndexName().

I used to call this checkIndexName() function within my "*$("#table_index tbody tr td.edit_index.ajax").live('click')*" event in tbl_structure.js file after getting the form loaded in the dialog. Then it runs at as the "onload" in non ajax scenario. Is it okay to handle that or should I rewrite the function for ajax.

Thilanka, as Rouslan said, please rewrite this function in jQuery, probably passing to it the form name as a parameter. Then move it to js/functions.js.

Then you'll be able to call it from tbl_structure.js in your Ajax logic; you'll be able to call this function also from indexes.js at load time.

Finally in tbl_indexes.php, instead on the "onchange", use jQuery in indexes.js to bind to changes in select_index_type.

On Thu, 2011-07-07 at 23:59 +0530, Thilanka Kaushalya wrote:

Hi Marc,

as Rouslan said, please rewrite this function in jQuery, probably

...

passing to it the form name as a parameter. Then move it to js/functions.js.

Then you'll be able to call it from tbl_structure.js in your Ajax logic; you'll be able to call this function also from indexes.js at load time.

Finally in tbl_indexes.php, instead on the "onchange", use jQuery in indexes.js to bind to changes in select_index_type.

I did the above mentioned changes and pushed the changes to the repo. Please check that. Thank you.

I checked out your branch and it looks good to me (for whatever it's worth). The only few things that I noticed that might need improvement are:

* `form_name` in checkIndexName() is misleading as a variable name, since it will contain the ID of an element (maybe form_id would be better).

* in checkIndexName() you defined: [code] var the_idx_name = $("#input_index_name"); [/code] but then you use several times $("#input_index_name") instead of the cached `the_idx_name` variable (which BTW should be `$the_idx_name` because it's a jQuery object and not an arbitrary JS variable).

* You copied an pasted the PHPDOC header from the old version of the checkIndexName() function, which did not take any arguments, so you are missing `@param` for the function input.

* [code] the_idx_name.attr("disabled") [/code] will never be undefined (it's a jQuery object now). Those two times that you are comparing it against 'undefined' are pointless, so you can just remove those 'if' statements.

* missing semicolon after 'false' in this code snippet from functions.js: [code] if ($("#"+form_name).length == 0) { return false } [/code]

Hope this helps.

Bye, Rouslan

Le 2011-07-08 00:02, Thilanka Kaushalya a écrit :

Hi Rouslan,

I checked out your branch and it looks good to me (for whatever it's

...

worth). The only few things that I noticed that might need improvement are:

• `form_name` in checkIndexName() is misleading as a variable name,

since it will contain the ID of an element (maybe form_id would be better).

• in checkIndexName() you defined:

[code] var the_idx_name = $("#input_index_name"); [/code] but then you use several times $("#input_index_name") instead of the cached `the_idx_name` variable (which BTW should be `$the_idx_name` because it's a jQuery object and not an arbitrary JS variable).

• You copied an pasted the PHPDOC header from the old version of the

checkIndexName() function, which did not take any arguments, so you are missing `@param` for the function input.

• [code] the_idx_name.attr("disabled") [/code] will never be undefined

(it's a jQuery object now). Those two times that you are comparing it against 'undefined' are pointless, so you can just remove those 'if' statements.

• missing semicolon after 'false' in this code snippet from

functions.js: [code] if ($("#"+form_name).length == 0) { return false } [/code]

I fixed those bugs and pushed to the repo.

Thilanka, looking at your repo with latest commit 526c72652fefca7025614552f5f5ad80d912462c, there is still the problem of variables which are jQuery objects and are not prefixed with $, in the checkIndexName() function.

Otherwise, the code works good and is almost ready for a merge.

Le 2011-07-08 11:42, Thilanka Kaushalya a écrit :

Hi Marc,

looking at your repo with latest commit
526c72652fefca7025614552f5f5ad80d912462c, there is still the problem of
variables which are jQuery objects and are not prefixed with $, in the
checkIndexName() function.

Otherwise, the code works good and is almost ready for a merge.

I changed those variables and pushed the code. Please check the repo. Next I'll work with the data piker issue. Thank you.

Feature is now merged.

Le 2011-07-11 05:16, Thilanka Kaushalya a écrit :

Hi Marc,

...

I changed those variables and pushed the code. Please check the repo.

...

Next I'll work with the data piker issue. Thank you.

Madhura reported me an issue on not checking the type at initial form load. I have fixed it and pushed the changes to the repo with commit 3d7cfa7b979178dafb3b7e87928d9b94ec87331a. Please check that. Thank you.

Regards, Thilanka.

Fix looks fine.

P.S. in this section, the "div" variable is a jQuery object so should be named "$div".

Le 2011-07-11 13:34, Thilanka Kaushalya a écrit :

Hi Marc,

Fix looks fine.

P.S. in this section, the "div" variable is a jQuery object so should be
named "$div".

I changed some of the variable nams to"$<name>" format and pushed to the repo. Please check that. Thank you.

Regards, Thilanka.

OK. This will be merged soon, see my previous reply.