[Phpmyadmin-devel] About the AJAXification of the index editor
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. Bye, Rouslan
Hi Rouslan, On Mon, Jul 4, 2011 at 8:04 PM, Rouslan Placella <rouslan@placella.com>wrote:
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. Regards, Thilanka. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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.com>wrote:
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
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.
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
Fixed and I think now it works.
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
Have to work with these two issues.
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 replaced the "bind" with "live" and now it seems okay. Please check that. I updated the repo with those changes and will work with the rest tomorrow. Thank you. Regards, Thilanka. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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. -- Marc Delisle http://infomarc.info
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. Regards, Thilanka. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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
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.
Hope this helps.
Yes. Those are really helpful for me and thanks for the guidance you gave.
Regards, Thilanka. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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. -- Marc Delisle http://infomarc.info
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. Regards, Thilanka. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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. -- Marc Delisle http://infomarc.info
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. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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". -- Marc Delisle http://infomarc.info
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. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
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. -- Marc Delisle http://infomarc.info
Hi Marc and Rouslan, 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
Fixed this issue.
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
Fixed this issue.
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.
It seems now the SPATIAL indices working correctly. Should I go for classes in this.
I guess that this is quite a mouthful of an email, hope it helps.
I pushed the code to the repo, so please check that. Thanks for your help for improving the feature. Regards, Thilanka. -- L G Thilanka Kaushalya Computer Science & Engineering, University of Moratuwa, Sri Lanka Connect with me at, Gmail : lgtkaushalya@gmail.com Twitter : @thilanka_k Facebook : Thilanka Kaushalya Yahoo : lgtkaushalya@yahoo.com Blog : http://coders-view.blogspot.com/
participants (3)
-
Marc Delisle -
Rouslan Placella -
Thilanka Kaushalya