Edgewall Software
Modify

Opened 6 years ago

Closed 14 months ago

#10144 closed defect (fixed)

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

Reported by: anonymous Owned by: walty <walty8@…>
Priority: high Milestone: 1.2
Component: timeline Version: 1.0dev
Severity: normal Keywords: multirepos bitesized
Cc: ethan.jucovy@…, ryan.j.ollos@…, walty
Release Notes:

The multirepository timeline filter has separate toggle all and tree expander functions.

API Changes:

Description

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.

Attachments (11)

t10144-r11682-1.patch (1.1 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 4 years ago.
Patch against r11682 of the trunk.
t10144-r11682-2.patch (1.2 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 4 years ago.
Patch against r11682 of the trunk.
timeline-fold-repos.png (17.4 KB ) - added by anonymous 2 years ago.
mutli-repo-toggle.v1.patch (1.6 KB ) - added by walty 2 years ago.
Collapsed.png (31.5 KB ) - added by Ryan J Ollos 2 years ago.
Expanded.png (23.8 KB ) - added by Ryan J Ollos 2 years ago.
multi-repo-toogle.v2.patch (1.7 KB ) - added by walty 2 years ago.
this is the patch so far (for your reference)
multi_repos_toggle.v3.patch (1.7 KB ) - added by Ryan J Ollos 22 months ago.
multi_repos_toggle.v4.patch (2.5 KB ) - added by walty 21 months ago.
collapsed.v4.png (9.1 KB ) - added by walty 21 months ago.
open.v4.png (10.6 KB ) - added by walty 21 months ago.

Download all attachments as: .zip

Change History (58)

comment:1 Changed 6 years ago by anonymous

please fix the issue

comment:2 in reply to:  1 ; Changed 6 years ago by Remy Blank

Replying to anonymous:

please fix the issue

Please provide a better idea and a patch.

comment:3 in reply to:  2 Changed 6 years ago by anonymous

Replying to rblank:

Replying to anonymous:

please fix the issue

Please provide a better idea and a patch.

You have a better idea ?

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by Remy Blank

Milestone: next-major-0.1X

Good suggestion.

comment:6 Changed 5 years ago by anonymous

yes, please chage

comment:7 Changed 5 years ago by 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.

comment:8 Changed 5 years ago by anonymous

Thank u

comment:9 in reply to:  7 Changed 5 years ago by 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. :-/

comment:10 Changed 5 years ago by Ethan Jucovy <ethan.jucovy@…>

Cc: ethan.jucovy@… added

comment:11 Changed 4 years ago by Christian Boos

Milestone: next-major-releasesnext-dev-1.1.x
Priority: highesthigh
Severity: trivialnormal

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

comment:12 Changed 4 years ago by Christian Boos

Component: generaltimeline
Keywords: multirepos added
Version: 1.0dev

Changed 4 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: t10144-r11682-1.patch added

Patch against r11682 of the trunk.

comment:14 Changed 4 years ago by 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.

Changed 4 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: t10144-r11682-2.patch added

Patch against r11682 of the trunk.

comment:15 in reply to:  14 Changed 4 years ago by 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())

comment:16 Changed 4 years ago by 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?

comment:17 Changed 4 years ago by Ryan J Ollos <ryan.j.ollos@…>

Cc: ryan.j.ollos@… added

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.

comment:18 Changed 2 years ago by Ryan J Ollos

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

Changed 2 years ago by anonymous

Attachment: timeline-fold-repos.png added

comment:19 Changed 2 years ago by 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.

comment:20 Changed 2 years ago by Ryan J Ollos

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

comment:21 Changed 2 years ago by Ryan J Ollos

Keywords: bitesized added

comment:22 Changed 2 years ago by 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.

comment:23 Changed 2 years ago by 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 ;)

Changed 2 years ago by walty

Attachment: mutli-repo-toggle.v1.patch added

comment:24 Changed 2 years ago by 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.

Changed 2 years ago by Ryan J Ollos

Attachment: Collapsed.png added

Changed 2 years ago by Ryan J Ollos

Attachment: Expanded.png added

comment:25 Changed 2 years ago by 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.

comment:26 Changed 2 years ago by 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.

comment:27 Changed 2 years ago by 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.

comment:28 Changed 2 years ago by 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.

Changed 2 years ago by walty

Attachment: multi-repo-toogle.v2.patch added

this is the patch so far (for your reference)

comment:29 in reply to:  28 Changed 2 years ago by 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.

comment:30 Changed 22 months ago by 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!

Last edited 22 months ago by Ryan J Ollos (previous) (diff)

Changed 22 months ago by Ryan J Ollos

Attachment: multi_repos_toggle.v3.patch added

comment:31 Changed 22 months ago by Peter Suter

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

Visually I think it would be nice to use the /chrome/common/expander_normal.png, /chrome/common/expander_normal_hover.png, /chrome/common/expander_open.png, /chrome/common/expander_open_hover.png 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.

comment:32 Changed 22 months ago by 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.

comment:33 Changed 22 months ago by figaro

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

comment:34 in reply to:  32 Changed 22 months ago by 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].

comment:35 Changed 21 months ago by walty

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

comment:36 Changed 21 months ago by 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.

comment:37 Changed 21 months ago by walty

Cc: walty added

Changed 21 months ago by walty

Attachment: multi_repos_toggle.v4.patch added

Changed 21 months ago by walty

Attachment: collapsed.v4.png added

Changed 21 months ago by walty

Attachment: open.v4.png added

comment:38 Changed 21 months ago by 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):

http://trac.edgewall.org/raw-attachment/ticket/10144/collapsed.v4.png

http://trac.edgewall.org/raw-attachment/ticket/10144/open.v4.png

comment:39 Changed 21 months ago by Ryan J Ollos

Milestone: next-dev-1.1.x1.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:40 Changed 21 months ago by 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.

Last edited 21 months ago by Ryan J Ollos (previous) (diff)

comment:41 in reply to:  40 Changed 21 months ago by 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).

comment:42 Changed 21 months ago by Ryan J Ollos

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

comment:43 Changed 21 months ago by Ryan J Ollos

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [14242]. Thanks walty!

comment:44 Changed 21 months ago by Ryan J Ollos

Owner: changed from Ryan J Ollos to walty

comment:45 Changed 20 months ago by Ryan J Ollos

Owner: changed from walty to walty <walty8@…>

comment:46 Changed 14 months ago by Christian Boos

Resolution: fixed
Status: closedreopened

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

comment:47 in reply to:  46 Changed 14 months ago by Christian Boos

Resolution: fixed
Status: reopenedclosed

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

Fix committed in [14579].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain walty <walty8@…>.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from walty <walty8@…> to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .

 
Note: See TracTickets for help on using tickets.