batchmod js interaction with query result layout

[framay <franz.mayer@…>]

Note: the original problem report is resolved as works as designed, however this evolved into fixing a few HTML layout glitches, then into finding a way to keep the original query_results.html template unaware of the batch modification feature, yet still reactive to changes done to the layout by the batch mod Javascript code.

Original Description

Batch modify do not apply for all tickets when selecting a group

Not all tickets of a selected group are executed by batch modify.

Steps to reproduce:

• create a custom query with a group ("Group results by")
• select header where at bottom of the page it is written "(more results for this group on next page)"
• execute batch modify

Only displayed tickets are changed (and not the rest of the header).

Related branches

• repos:cboos.git:t525/cosmetic-fixes
• repos:cboos.git:ticket10714/updateData-colspan

[Christian Boos]

I think it works as designed: that checkbox is for selecting all the visible tickets of the group; it would be dangerous to modify tickets that are only "implicitly" selected. If you don't want to repeat the batch modify operation for each page of result, you could either narrow your search to select that group, or change the Max items per page value, or both…

There's a glitch though, the colspan for the (more results for this group on next page) row is off by one…

[Christian Boos]

There's a glitch though, the colspan for the (more results for this group on next page) row is off by one…

Fixed in repos:cboos.git:t525/cosmetic-fixes.

… but these changes raise some very interesting questions.

As the batch modification feature started its life as the ​TH:BatchModifyPlugin, it's no wonder that a lot of the HTML modifications were done "on top of" the already generated HTML. These modifications, like adding the checkboxes in an extra column to the left of each result row, are done once the query_results.html​ template is generated. It was done using Javascript, but it could also have been done with Genshi using an ITemplateStreamFilter (well, with probably considerably more effort on one hand and more CPU on the other hand).

Now, the feature is in core, and the "natural" way to fix a glitch like the one mentioned above (colspan not taking into account the presence of the extra checkbox column) was to look at the query_results.html template and fix it there (a8e4265/cboos.git). Right after this fix, I realized that one such correction had already been done in Javascript! So I removed it (031166b/cboos.git), as it was no longer necessary.

Here we have the interesting design problem. Working on the level of the template is certainly the easiest thing to do; it also gives you an integrated view which makes it easy to spot all the interactions (in this specific case, all the places for which the colspan was affected by the batch modification). We could even go further and do most of the remaining HTML generation currently done in initializeBatch()​ directly in the template. But going this way, we lose the interesting property that before my fix, the query_results.html template didn't have to know anything about the batch modification feature! It was therefore simpler, having one less dimension to deal with. OTOH, it may have been possible to expand the correction done in Javascript that I removed in the second commit, so that it covered all the other cases. But it would have been painful to write selectors for all the cases, and this approach wouldn't even work if there existed other extensions which added some other rows which also needed to use a "variable" colspan.

The essence of the problem is that query_results.html (or, if you want, the rendering logic for the query results) has to know about the total number of columns (num_cols as in the fix) and that number is susceptible to be modified by "extensions" (like the batch modification feature adding an extra column).

So ideally, in order to keep things modular, we would like to have a simple result structure which is used as a skeleton for other features building on top of it. In our example, the colspans attributes from the simple structure would need to be modified as a side-effect of the modification made to a num_cols variable, done by the batch modify js code. For that we need some kind of data linking. There are numerous Javascript framework which provide this, but in this scenario, we would just need unidirectional linking, from the data to the DOM, so we can perhaps cook something simpler using jQuery only…

[Christian Boos]

Ok, so here's a proof of concept, it works but can certainly be improved in many ways.

In 60fd4cd/cboos.git, I added an updateData() method which changes the data associated with a selector and tries to update all sub-elements which have specified to be updated. The specification is done in a very simple way, using classes. It's probably even too simple (what if an element wants to be updated for more than one data?).

Basically, it works like this. An element specifies that it wants to react on a change, and how:

<th colspan="$num_cols" class="colspan= trac_num_cols">

• colspan="$num_cols" is the initial value (Genshi data).
• class="colspan= trac_num_cols" means: set the colspan attribute to the value of the data trac_num_cols when it is updated via a call to updateData()

The latter happens when the batch modification Javascript code is activated:

var new_trac_num_cols = /* compute new value */;
$("table.listing").updateData('trac_num_cols', new_trac_num_cols);

• $("table.listing") is the element containing the data 'trac_num_cols'
• the way the new_trac_num_cols value is computed is a bit complex, i.e. we need to either propagate the initial value from the server via add_script_data or we look at the effective number of cols; it would be much more convenient if we could rely on the initial value being directly associated with the element when it's built, and this is what 3c99d5d/cboos.git hints at…

[Christian Boos]

Refocusing the ticket on the design issues discussed above.

However, to do justice to the original bug report, we could indeed add a tooltip or two… By saying Toggle selection of all tickets shown in this group in the tooltip of the checkbox in a group header, I think it becomes clear that the tickets not shown won't be concerned (fa1ccfb/cboos.git).

[Peter Suter]

Ah yes, the first colspan glitch was noticed in comment:94:ticket:525 and fixed a few comments later, but I must have missed the related glitch. As you mentioned, I fixed the first one in JS, as all other batchmodify template changes are done there, and I didn't want to radically change the design in that phase and make the template itself know about batchmodify.

Anyway, the repos:cboos.git:t525/cosmetic-fixes look good.

BTW, the resolution required​ check could be removed, too. It is now handled in the workflow.

repos:cboos.git:ticket10714/updateData-colspan sounds interesting. (Probably a bit too experimental for 1.0 though?)

Another idea would be to extend the existing hook​ into batchmodify (for preparing the template's data) to also increment some data['num_cols'] count there. Perhaps this could also be refactored into a ITicketQueryResultAnnotator interface, so other plugins have the same possibilities.

[Christian Boos]

Replying to psuter:

Anyway, the repos:cboos.git:t525/cosmetic-fixes look good.

I just rebased and pushed repos:cboos.git:t525/cosmetic-fixes.2:

• [e5e2cd3d/cboos.git] #525: consistent use of whitespace in query.js
• [a8cd3ede/cboos.git] #525: don't show batch_selector checkboxes on print.
• [d6a573ab/cboos.git] #10714: in query_results.html, fix colspan for batch modify.
• [5b705b07/cboos.git] #10714: remove one colspan correction done in js
• [3a4e7d7c/cboos.git] #10714: add tooltips for batch modify checkboxes

BTW, the resolution required​ check could be removed, too. It is now handled in the workflow.

• [69b46718/cboos.git] #525: remove js check for resolution required which is now done in the workflow.

Committed the series as r11095.

---

repos:cboos.git:ticket10714/updateData-colspan sounds interesting. (Probably a bit too experimental for 1.0 though?)

Right, let's keep that for 1.1dev (which is now in sight ;-) ).

Another idea would be to extend the existing hook​ into batchmodify (for preparing the template's data) to also increment some data['num_cols'] count there.

Well, I think it's just happens that the batch modify feature has to deal with this "variable". It's not in charge of it by itself, rather the Query module is.

Perhaps this could also be refactored into a ITicketQueryResultAnnotator interface, so other plugins have the same possibilities.

Exactly, yes. We'll need something easy to plug Javascript content and behavior contributors in a page, without having necessarily to resort to the request filters.