Changes between Initial Version and Version 1 of Ticket #10714, comment 2
- Timestamp:
- Jun 8, 2012, 12:13:42 AM (12 years ago)
Legend:
- Unmodified
- Added
- Removed
- Modified
-
Ticket #10714, comment 2
initial v1 5 5 ... but these changes raise some very interesting questions. 6 6 7 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 right of each result row, are done once the [source:trunk/trac/ticket/templates/query_results.html query_results.html] template is generated. It was done using Javascript, but it could also have been done with Genshi using an [[TracDev/PluginDevelopment/ExtensionPoints/trac.web.api.ITemplateStreamFilter|ITemplateStreamFilter]] (well, with probably considerably more effort on one hand and more CPU on the other hand).7 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 [source:trunk/trac/ticket/templates/query_results.html query_results.html] template is generated. It was done using Javascript, but it could also have been done with Genshi using an [[TracDev/PluginDevelopment/ExtensionPoints/trac.web.api.ITemplateStreamFilter|ITemplateStreamFilter]] (well, with probably considerably more effort on one hand and more CPU on the other hand). 8 8 9 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 ([ repos:cboos.git:a8e4265]). Right after this fix, I realized that one such correction has already been done in Javascript! So I removed it ([repos:cboos.git:031166b]) as it's no longer necessary.9 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 ([changeset:a8e4265/cboos.git]). Right after this fix, I realized that one such correction had already been done in Javascript! So I removed it ([changeset:031166b/cboos.git]), as it was no longer necessary. 10 10 11 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 [source:trunk/trac/htdocs/js/query.js#/initializeBatch 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, expanding the correction in Javascript that I removed in the second commit, so that it covers all the other cases. It would be a bit painful to write selectors for each case, and this approach wouldn't even work if another extension added some other rows also with acolspan.11 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 [source:trunk/trac/htdocs/js/query.js#/initializeBatch 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. 12 12 13 The essence of the problem is that query_results.html (or, if you want, the base query results rendering logic) has to know about the total number of columns (`num_cols` as in the fix). That number is susceptible to vary, due to "extensions" (batch modification extra column here).13 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). 14 14 15 So ideally, in order to keep things modular, we would like to have a simple result structure (like the one produced by query_results.html) which is used as a skeleton for other features building on top of it (like batch modify in query.js). The `colspan`s attributes from the simple structure (or from other extensions) would need to be able to adapt to the changes made to this `num_cols` parameter by other 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...15 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 `colspan`s 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...
