[Patch] Make it easier to switch off commit messages in timeline

[anonymous]

when having multiple repository, like on trac.edgewall.org, it is quite cumbersome to not display commits. unticking "changesets in all repositories" switches the view to show all repositories, and every single one ticked again. which of course is the exact opposite one wanted to reach with unticking.

[anonymous]

please fix the issue

[Remy Blank]

Replying to anonymous:

please fix the issue

Please provide a better idea and a patch.

[anonymous]

Replying to rblank:

Replying to anonymous:

please fix the issue

Please provide a better idea and a patch.

You have a better idea ?

[anonymous]

A standard UI for this kind of thing would present the list of repositories with editable checkboxes, and a "select all" checkbox at the head of the list. Ticking "select all" causes all subordinate checboxes to be ticked, un-ticking causes all subordinate checkboxes to be un-ticked.

un-ticking one of the subordinate checkboxes causes the "select all" checkbox to be un-ticked, but does not affect any of the other checkboxes.

this provides a quick shortcut for selecting and un-selecting all.

[Remy Blank]

Good suggestion.

[anonymous]

yes, please chage

[Carsten Klein <carsten.klein@…>]

Implemented and tested with Firefox. Changes are trivial enough that they should work with the other browsers. See pull request ​https://github.com/edgewall/trac/pull/3 over at github.

[anonymous]

Thank u

[Ethan Jucovy <ethan.jucovy@…>]

Replying to Carsten Klein <carsten.klein@…>:

Implemented and tested with Firefox. Changes are trivial enough that they should work with the other browsers. See pull request ​https://github.com/edgewall/trac/pull/3 over at github.

This patch works for me (tested on Chrome against trunk@11032) but it has a side effect: when "Changesets in all repositories" is checked, the list of repositories is not hidden. Before this patch, the list of all repositories only shows up when the "Changesets in all repositories" box is not checked.

I understand the intent of this change — it makes it possible to "select all" and then specifically deselect one or two repositories. But if you have a lot of repositories, being able to hide the list of all repos is really useful. (I have 20+ repos in one Trac site for example.)

Even in an unpatched Trac, though, I have the need to hide the list of repositories when I have (e.g) zero or only 2/20 repos selected.

So I think the user interface here needs to be expanded to include 2-3 separate functions:

1. "Show/hide all repositories"
2. "Show/hide unselected repositories"
3. "Select/unselect all repositories"

Or, alternatively, this set of functions would provide the same capabilities and more, using orthogonal functions:

1. "Show/hide selected repositories"
2. "Show/hide unselected repositories"
3. "Select/unselect all repositories"

I'm not sure how to present that UI in a clean and concise way though. :-/

[Christian Boos]

I think that using a tri-state (as in #10922) would greatly help here.

[anonymous]

​https://github.com/edgewall/trac/pull/3

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

Patch against r11682 of the trunk.

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

The change in comment:13 implements the group toggling behavior for the Changesets in all repositories checkbox (i.e. the group toggler), however it doesn't implement tri-state behavior for the group toggler. Also, we should use prop rather than attr.

t10144-r11682-1.patch​ adds to the patch in comment:13 to implement the tri-state behavior for the group toggler. The following behavior was intentional (though I don't mean to imply that it shouldn't be discussed and considered further):

• Clicking on Changesets in all repositories will show or hide the repositories depending on the state of the Changesets in all repositories checkbox.
• Unchecking all of the repositories will result in the Changesets in all repositories being unchecked, but will not hide all of the repositories. It seemed too odd to have the checkboxes dissappear after unchecking the last checked one.

Rather than showing/hiding, we could use folding, but I tend to think that won't be as good (haven't tried it yet though).

I'd even suggest that this change is suitable for 1.0.2 since the current behavior is pretty undesirable.

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

Patch against r11682 of the trunk.

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

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

t10144-r11682-1.patch​ adds to the patch in comment:13 to implement the tri-state behavior for the group toggler. The following behavior was intentional (though I don't mean to imply that it shouldn't be discussed and considered further):

I rethought this and went the opposite direction of my initial thinking. The repositories are always hidden now if all repositories are unselected.

I also had a mistake in the patch uploaded last evening in that I wasn't considering that the filters were persistent (i.e. stored in session data), and this affects what needs to be done on page load. t10144-r11682-2.patch​ corrects the error.

Regarding the change checked() → prop("checked"). Calling the method checked() seems to work fine, but I can't find any documentation of the method in jQuery, and ​jQuery prop suggests using prop("checked") or is(":checked"). checked() is used multiple places in the Trac codebase, I'm just bothered that it doesn't seem to be documented in the jQuery API.

(t10144)user@debian-wheezy:~/Workspace/t10144/trac-trunk$ grep -R --exclude-dir=.svn "checked()" .
./trac/admin/templates/admin_milestones.html:                  $("#completeddate").enable($("#completed").checked());
./trac/ticket/templates/milestone_edit.html:          $("#duedate").enable($("#due").checked());
./trac/ticket/templates/milestone_edit.html:          var checked = $("#completed").checked();
./trac/ticket/templates/milestone_edit.html:          $("#target").enable(checked && retarget.checked());
./trac/ticket/templates/report_list.html:          if ($(this).checked())
./trac/ticket/templates/ticket.html:            $(this).siblings().find("*[id]").enable($(this).checked());
./trac/ticket/templates/ticket.html:            $(this).siblings().filter("*[id]").enable($(this).checked());
./trac/ticket/templates/ticket.html:          if (!$('#trac-comments-oldest').checked())

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

The toggling behavior leaves us with an inconvenience: suppose you only want one of N repositories to be selected. To accomplish this you must select Changesets in all repositories checkbox to make all the of repository checkboxes visible, and then unchecked N-1 checkboxes.

Maybe some folding behavior would be better, and only unfold on page load if Changesets in all repositories checkbox is in the indeterminate state?

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

Now that I've re-read this, I see that several points in Ethan's comment:9 were similar to my comment:16. I'll see if folding the Changesets in all repositories checkbox might work well here.

[Ryan J Ollos]

#12013 closed a duplicate, requesting to collapse the repository list when some repositories are unchecked.

[anonymous]

Main point of #12013 was to add separate UI control for fold / collapse like in attached mockup​. Don't mix it with checkboxes. This solves all problems.

[Ryan J Ollos]

Folding the checkboxes below Changesets in all repositories has been discussed in this ticket. PatchWelcome.

[walty]

for issues like this (UI related), would the unit test still be needed? and if yes, what's the normal practice of doing this? thanks.

[Ryan J Ollos]

Our existing functional test infrastructure based on Twill doesn't support testing JavaScript (#11988), so writing functional tests for JavaScript code isn't yet possible. You are relieved of any responsibility to add functional tests for JavaScript code ;)

[walty]

To be honest, I am not too good with the front end thing. Anyway I tried my best for searching the reference materials. if there are major problems with the patch (e.g. coding style / patching strategy), pls let me know. thanks.

[Ryan J Ollos]

The toggler symbol pushes the checkboxes to the right which is undesirable. I'm unsure of how to best place the symbol, and I also wonder if it would be better to use a [+]/[-] symbol pair.

I think we definitely want the select all checkbox to be tri-state, as in t10144-r11682-2.patch​.

Probably the toggler that you implemented would work fine if you can keep the checkboxes horizontally aligned. It should be possible to expand/collapse using the left and right arrow keys when tabbed over Changesets in all repositories. At least, that seems intuitive to me, though we need to evaluate accessibility.

trunk/trac/htdocs/js/folding.js@11493:3​ may provide some inspiration or possibility for code re-use.

[walty]

using [+] / [-] pair would be actually much easier (it took me quite some effort to squeeze the checkboxes and labels). I just thought the latest mark up of ​timeline-fold-repos.png was agreed.

no problem, I would try to update the patch a little bit.

[Ryan J Ollos]

I don't think anything is definitely agreed yet. I'd be interested to hear what anyone else has to say, since I don't consider myself to have the greatest sense of design.

[walty]

hi,

would it be necessary for the system to remember the previous collapse/expand state of the multiple repository thing (e.g. during the screen refresh)?

previously, if 'check all' is not checked, it would be expanded automatically. and now, the two things (collpase and check all) are separate.

so if we are going to remember the previous state of collapse, I probably need to set up one more session variable, which make the thing a little bit more complicated.

[walty]

this is the patch so far (for your reference)

[Ryan J Ollos]

Replying to walty:

would it be necessary for the system to remember the previous collapse/expand state of the multiple repository thing (e.g. during the screen refresh)?

I think it probably isn't needed, but I'll give it some more thought. Sorry for the delay, I'll try to review your latest patch tomorrow.

[Ryan J Ollos]

I'm sorry for the long delay in reviewing this. I think we are moving in the right direction. The major things blocking still are:

• I'm unsure if placement of [+]/[-] to the right is good design.
• It's not possible to tab over [+]/[-] and toggle with the keyboard.
• With the font used, it's obvious that [-] is narrower than [+].

Also, I noticed a few minor issues:

• totalChecked is implicitly declared → replace with var totalChecked.
• Update for coding style guidelines in TracDev/CodingStyle#JavaScript, as well as the following minor additional style changes:
  • One space between // and first letter of comment.
  • One space for { in conditionals and functions.
  • One space after : in JS objects.
  • true/false can be used rather than "checked" and "".

I corrected a minor bug and did some refactoring, but did not correct all of the mentioned style issues yet:

attachment:multi_repos_toggle.v3.patch​

So basically I think we just need some inspiration on where to place the expander and what style to use, but otherwise I think the patch looks good so far. This has been a major usability issue for a while, but it seems no one has a good idea yet on how to solve the issue.

Thanks for your effort to move this forward!

[Peter Suter]

The usability this patch provides is great. Thanks for working on it.

Visually I think it would be nice to use the , , , images with tooltips and :hover styling, like in the source browser (CSS​ / JS​).

(There we also provide special keyboard navigation​. I'm not sure if that would be a useful alternative to the missing keyboard tabbing / toggling. Probably not worth it for just a single expander though.)

As for the placement, have you considered the expander after the checkbox, but before the Changesets in all repositories text? (csetfilter.after($repolist_expander);) It's not perfect, but maybe a bit better than the alternatives.

[Ryan J Ollos]

Yeah, placement of the expander to the right of the checkbox might work well.

I noticed that my patch (and some other JS code that I've committed) violates our style guide: Use lowercase for variable names.

• tags/trac-1.1.5/trac/admin/templates/admin_plugins.html@:28-30#L23​
• tags/trac-1.1.5/trac/htdocs/js/trac.js@:41-43#L26​

[Ryan J Ollos]

Replying to rjollos:

I noticed that my patch (and some other JS code that I've committed) violates our style guide: Use lowercase for variable names.

Fixed in [14183].

[walty]

hm.. just curious, should we put some icon before each repository name (like the source code browser)?

[Ryan J Ollos]

I'm unsure how that would look, or what icons you are referring to exactly. If you have a clear idea of what that would look like, it would be good to just post a patch and we can review it.

[walty]

I have implemented the new patch, based on comments from psuter. I really hate copy and paste the code, but I am not sure if it's appropriate to refactor out a common css class for this case, so eventually I decided to copy and modify the 4 classes from browser.css

The new look are like the following (hover and tooltip are added as well):

[Ryan J Ollos]

The changes look pretty good. I still wonder if there's something we could do to improve keyboard navigation / accessibility (maybe use left/right arrow keys to expand collapse when the Changesets in all repostiories checkbox has focus). If there are no objections, or further inspiration, I'll commit the patch with minor modifications by milestone:1.2.

[Ryan J Ollos]

Replying to rjollos:

(maybe use left/right arrow keys to expand collapse when the Changesets in all repostiories checkbox has focus)

Added in [0437b204/rjollos.git] (log:rjollos.git:t10144_multi_repos_toggle.v4.1).

[Ryan J Ollos]

Refactored CSS in [cf61171f/rjollos.git]. Title attribute properly translated in [9494467a/rjollos.git].

[Ryan J Ollos]

Committed to trunk in [14242]. Thanks walty!

[Christian Boos]

The .css changes in r14242 broke the expanded style in the source browser. I'm working on a follow-up.

[Christian Boos]

The .css changes in r14242 broke the expanded style in the source browser. I'm working on a follow-up.

Fix committed in [14579].