Add drag and drop and other dynamic behavior to the admin panels containing enum lists

[Ryan J Ollos <ryan.j.ollos@…>]

A reworked version of ​th:AdminEnumListPlugin will be provided as a patch to Trac. The primary behavior will apply to the Priorities, Resolutions, Ticket Types and Severities panels, but behaviors will be implemented for almost every admin panel.

The following behaviors will be provided in the initial version of the patch:

• The enum lists can be sorted by drag and drop of the items. ⇒ #11682
  • The Order column of select elements is hidden.
  • A Revert changes button is added. The button is disabled until the list is reordered.
• The Save changes button is disabled until a change is made.
• The Remove selected items button is disabled until a checkbox is selected. ⇒ #11056
• Addition of a Select All checkbox in the table header.
• Allow the Components, Milestones and Versions tables to be sorted (in jQuery?: ​th:#10876). For inspiration, see ​th:TableSorterPlugin. #11683

Similar behavior will be applied to other admin pages for consistency, where applicable.

[Remy Blank]

Sounds like a nice idea. Are you going to replace the current implementation completely, or are you going to build on the current (non-dynamic) functionality? That is, will the pages still work with JavaScript disabled?

We have always had a tradition in Trac to have static fallbacks for dynamic behavior. I'm not saying we should continue this forever (actually, I would rather advocate the opposite), just curious about your plan.

[Ryan J Ollos <ryan.j.ollos@…>]

I'm not planning to change the templates or Python code at all, so it should work the same when JavaScript is disabled. For example, the select elements are left in place and hidden by the Javascript. I took the same approach in #9609.

The latest version of the ​th:AdminEnumListPlugin has all the functionality I have in mind. It is basically just a ​piece of javascript that should be reworked to make use of ​jQuery UI's Sortable widget, and all of the backward compatibility stuff can be dropped. It currently works in Trac 0.11, which required a significant number of workarounds.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11508 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

Updated description with my current thinking on what the final patch will contain.

I've implemented the portion of the patch that enables buttons when checkboxes or a file have been selected. I still have a bit of work to do on the other features, but wanted to post this version of the patch to ask a few questions that will help me complete the implementation. I'll highlight a few parts of the patch that I'm uncertain about and looking for early feedback on:

1. I've put the disableIfNoneChecked function in trac.js with the idea that it could be useful outside of the administration panels. Specifically, I'm thinking we could use it for the Change tickets button on the batch modify form, and for the Add attachment button on the attachment page.
2. I'm confused about where I should be placing the CDATA section tags. I don't see any reason to not just place them within every script text=text/javascript element, but I see them very selectively placed in some files.
3. I've used this pattern change(function() {...}).eq(0).change() (in this case click(function() {...}).eq(0).click() would probably be the same), rather than .click(someFunction); someFunction(); (i.e. passing a function to click and then calling that function directly). Using change seems a bit cleaner and both patterns are present throughout the Trac codebase.

[Christian Boos]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

1. I've put the disableIfNoneChecked function in trac.js with the idea that it could be useful outside of the administration panels. Specifically, I'm thinking we could use it for the Change tickets button on the batch modify form, and for the Add attachment button on the attachment page.

Sounds good (for the attachment use case, I suppose there will be some additional check, for the file input).

2. I'm confused about where I should be placing the CDATA section tags.

We apparently do it only when needed (i.e. when using plain '<', '&' or '>' characters in the Javascript code), but I wouldn't object using it everywhere for consistency (and for not having to think about it!).

3. I've used this pattern change(function() {...}).eq(0).change() (in this case click(function() {...}).eq(0).click() would probably be the same), rather than .click(someFunction); someFunction(); (i.e. passing a function to click and then calling that function directly). Using change seems a bit cleaner and both patterns are present throughout the Trac codebase.

I suppose it all depends on whether there's a side-effect or not, and if yes, if the side-effect it predictable, or not (e.g. what would happen if there would be other event handlers?). The second pattern ensure you have a strict control over what you force during this initial call.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

Sounds good (for the attachment use case, I suppose there will be some additional check, for the file input).

I didn't describe it well, but I was thinking of putting a second function in trac.js that would be used for file / button pairs, such as Add attachment. Although, it might not be necessary since it seems possible to accomplish this in just 3 lines.

2. I'm confused about where I should be placing the CDATA section tags.

We apparently do it only when needed (i.e. when using plain '<', '&' or '>' characters in the Javascript code), but I wouldn't object using it everywhere for consistency (and for not having to think about it!).

Not having to think about it sounds good to me. Also, the code is a bit cleaner when the tags aren't interleaved.

It looks like I was responsible for confusing myself on this issue. The only cases that I see the tags being present when they are not required is due to a changeset from one of my patches [t 11104].

[Ryan J Ollos <ryan.j.ollos@…>]

Did you mean to target this to 1.0.2, or rather than 1.1.2? I'm happy to have that happen, but usually changes like those suggested in this ticket are targeted to major release.

I'll try to have a more extensive patch uploaded soon.

[Christian Boos]

Ah, I was just thinking about the current patch t10994-r11508-1.patch​ which doesn't contain anything potentially disruptive for 1.0.x. I suppose that part could be added to 1.0.x and the more ambitious change which this ticket is really about would better be suited for 1.1.x indeed.

[Ryan J Ollos <ryan.j.ollos@…>]

I like the idea of committing the existing patch separately. The existing patch is actually a bit of a deviation from the key things in mind that I had for this ticket anyway. I'll touch-up that patch and create a new ticket for it, before continuing with the other changes described for this ticket.

[Ryan J Ollos <ryan.j.ollos@…>]

I've created #11056 for the part that has been discussed for 1.0.2, and will create a new version of the patch soon.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

2. I'm confused about where I should be placing the CDATA section tags.

We apparently do it only when needed (i.e. when using plain '<', '&' or '>' characters in the Javascript code), but I wouldn't object using it everywhere for consistency (and for not having to think about it!).

In the latest patch for #11056, I've included CDATA tags for almost all of the changed code in script elements. The one case I've found where this shouldn't be done is if there is embedded Genshi in the script, such as <py:if test="preview">...</py:if>. I guess that's obvious in retrospect since we need that Genshi to be parsed!

3. I've used this pattern change(function() {...}).eq(0).change() (in this case click(function() {...}).eq(0).click() would probably be the same), rather than .click(someFunction); someFunction(); (i.e. passing a function to click and then calling that function directly). Using change seems a bit cleaner and both patterns are present throughout the Trac codebase.

I suppose it all depends on whether there's a side-effect or not, and if yes, if the side-effect it predictable, or not (e.g. what would happen if there would be other event handlers?). The second pattern ensure you have a strict control over what you force during this initial call.

The strict control sounds better for long-term maintainability. Thanks for giving it some thought and explaining the trade-off.

I'll put further comments on #11056, but just wanted to follow-up on this earlier discussion here.

[Ryan J Ollos]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

• Addition of a Select All checkbox in the table header.

Proposed changes in log:rjollos.git:t10994.2.

I've prefixed variable names containing jQuery objects with $ (​SO:553734/121694). I find the convention to be useful and fairly common in practice, but it should be discussed as to whether it might be adopted in TracDev/CodingStyle#JavaScript.

[Ryan J Ollos]

Change from comment:18 committed to trunk in [12954:12955]. Next I'll prepare changes to integrate the drag and drop functionality of the ​AdminEnumListPlugin.

[Ryan J Ollos]

Replying to rjollos:

I've prefixed variable names containing jQuery objects with $ (​SO:553734/121694). I find the convention to be useful and fairly common in practice, but it should be discussed as to whether it might be adopted in TracDev/CodingStyle#JavaScript.

I noticed two small things:

• I didn't follow the above convention everywhere in the changeset.
• this is a jQuery object, so we don't need to wrap it in $(). In fact, var $table = this; isn't needed at all, and is just present for code clarity.

  trac/htdocs/js/trac.js

  @@ -24 +24 @@
   
     // Add a Select All group toggler to each thead in the table.
     $.fn.addSelectAllTogglers = function() {
  -    var $table = $(this);
  +    var $table = this;
       if ($("tr td.sel", $table).length > 0) {
         $("tr th.sel", $table).append(
           $('<input type="checkbox" name="toggle_group" />').attr({
  @@ -36 +36 @@
           })
         );
         $("tr td.sel", $table).click(function() {
  -        var tbody = $(this).closest("tbody");
  -        var checkboxes = $("tr td.sel input", tbody);
  -        var numSelected = checkboxes.filter(":checked").length;
  +        var $tbody = $(this).closest("tbody");
  +        var $checkboxes = $("tr td.sel input", $tbody);
  +        var numSelected = $checkboxes.filter(":checked").length;
           var noneSelected = numSelected === 0;
  -        var allSelected = numSelected === checkboxes.length;
  -        $("tr th.sel input", tbody.prev())
  +        var allSelected = numSelected === $checkboxes.length;
  +        $("tr th.sel input", $tbody.prev())
             .prop({"checked": allSelected,
                    "indeterminate": !(noneSelected || allSelected)});
         });

[Ryan J Ollos]

Coding guideline added in TracDev/CodingStyle, but always open for further discussion of course.

[Christian Boos]

Replying to rjollos:

Coding guideline added in TracDev/CodingStyle, but always open for further discussion of course.

I agree, it's useful.

[Ryan J Ollos]

Created tickets #11682 and #11683 for work that won't be completed for 1.1.2.

[Ryan J Ollos]

Changes from comment:20 committed in [12963].

[Ryan J Ollos]

Renamed function in [13008], in consideration of changes that will be made in #11417.