Tabledrag column hiding not taking headers colspan into account

functionally seems good (not tested yet)
can you please modify the patch that the headerIndex variable stores the right value at the end. Then

var th = $('th:nth-child('+ headerIndex +')', this);

line could remain. Don't like that it is changed every time in the cycle.

I think there is still problem when row cell contains 'colspan' property, but that is an another issue, and should require mayor rewrite of this function.

Going trough every line in a table is too expensive. If you are trying to hide a table cell by the column index, then you have to go trough every line (see picture).
I don't know much about jquery, but the hiding of weight cell inside row should be done similar to the zebra striping. Add a class to every weight cell when you create the table, and hide (or do whatever with) that class. I don't know if it can be done or not.
If it can be done, then I would just solve it in another issue, because that need a mayor rewrite which should break things.
If it can't be done, I would first think about if we need such functionality.

About the other thing, you are right. I just did not catch that you are using the variable inside the loop.

I tried just about every combination I could think of to make the code less intensive, but seems like the less code I wrote the longer it took to execute the page. Often it's surprising when manually looping through elements is significantly faster than using a single jQuery selector.

I tried to take the approach Pasqualle recommended of just using a class and hiding the cell. Since we don't have a class on the cell we need to use the field class and then apply that to the parent cell. This is just about the shortest solution I could come up with, which doesn't even quite work 100% because it doesn't hide the header cell:

        var fields = $('.' + this.tableSettings[group][d]['target'], this.table);
        fields.each(function() {
          $(this).parents('td:first').css('display', 'none');
        });

Surprisingly though, finding all the fields in the table actually takes about 200ms longer than finding just the first one then looping through all the remaining cells in the table. I ran several solutions for timing and here's the *average* run time for each when rendering the admin/build/menu-customize/navigation page.

Before patch: 1700ms
After #4 patch: 1760ms
Solution in this comment: 2000ms

Btw, returning right away instead of hiding any table cells takes only 600ms. Clearly there would be an excellent place to improve performance considering it takes over half the processing time for the tableDrag behavior.

The good news is there just so happened to be a very obvious place for improvement. This call: $('.' + this.tableSettings[group][d]['target'] + ':first', this.table) was being done twice, right in a row. By setting this value to a variable and skipping the second call, it took off approx. 300ms of processing. Now that's an easy win.

This patch makes this improvement, reduces calls to jQuery for further efficiency, and uses the same method for cell hiding that yched included in #4.

The runtime after this patch is approximately 1500ms in Firefox. Safari 3 is flippin fast btw, only a 600ms increase in load time than if tabledrag.js is not included at all.

Timing was done by adding

console.time('foo'); // Add at beginning of Drupal.behaviors.tableDrag function
console.timeEnd('foo'); // Add at end of Drupal.behaviors.tableDrag function

By removing the row.children('[colspan]').size() and only calling row.children(':nth-child(' + index + ')') once per row, the runtime has been further reduced by 300-400ms. On admin/build/menu-customize/navigation I'm now getting processing times of approximately 1100ms, and it works with CCK fields. Much better!

re #6: my idea was to add the class inside drupal (in php) not with jquery..

but this patch could be backported to drupal6 and it makes it faster, so i am happy with it.

+1 Tested this patch out with D6 (since the .js file is identical) and it works great.

Committed to CVS HEAD.

I leave it up to Gabor to decide if this needs backporting to DRUPAL-6.

Committed to Drupal 6 as well, thanks!

Yes, I missed a commit previously ( http://drupal.org/node/227830 ), and accidentally committed both patches together. I figured it is better not to roll back, since it was intended to be committed anyway, even if credit was not given properly unfortunately with the commit.