Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

Last modified 9 years ago

#10766 closed defect (fixed)

'Comments only' filter is not respected during autopreview

Reported by: Ryan J Ollos <ryano@…> Owned by: Peter Suter
Priority: normal Milestone: 1.0
Component: ticket system Version: 1.0dev
Severity: minor Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

To reproduce:

  1. Create a new ticket.
  2. Select the Comment only filter.
  3. Add several non-comment edits to the ticket (i.e. ticket property changes).
  4. Start typing a ticket comment. Allow the autopreview to refresh.

The non-comment edits are shown in the comment history. The non-comment edits are not shown after the comment is submitted.

This is particularly troublesome because if there are many non-comment edits in the ticket history, the Add a comment box that the user is editing is shifted down and off the end of the screen.

Issue seen with r11112 of the trunk.

Attachments (1)

HidePropertyChanges.png (6.4 KB ) - added by Ryan J Ollos 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Peter Suter, 12 years ago

This is caused here: source:trunk/trac/ticket/templates/ticket.html@11112:68-71#L64

The change history is unthreaded/unrevert and updated. This ensures any new comments are visible and near the Add a comment form. See also comment:27:ticket:7145.

To keep ensuring that also non-comment changes are visible, we might want to keep the checkbox consistent:

  • trac/ticket/templates/ticket.html

    diff -r 6421f51ba312 trac/ticket/templates/ticket.html
    a b  
    6969          if (!$('#trac-comments-oldest').checked())
    7070            $('#trac-comments-oldest').click().change();
    7171          $("#changelog").replaceWith(items.filter("#changelog"));
     72          if ($('#trac-comments-only-toggle').attr('checked'))
     73            $('#trac-comments-only-toggle').attr('checked', false);
    7274          // Show warning
    7375          var new_changes = $("#changelog .trac-new");
    7476          $("#trac-edit-warning").toggle(new_changes.length != 0);

(Maybe also scroll the Add a comment box back into view if necessary?)

To change the behavior to keeping non-comment changes hidden, this seems to work ok:

  • trac/ticket/templates/ticket.html

    diff -r 6421f51ba312 trac/ticket/templates/ticket.html
    a b  
    6969          if (!$('#trac-comments-oldest').checked())
    7070            $('#trac-comments-oldest').click().change();
    7171          $("#changelog").replaceWith(items.filter("#changelog"));
     72          if ($('#trac-comments-only-toggle').attr('checked'))
     73            $('#trac-comments-only-toggle').click().attr('checked', true);
    7274          // Show warning
    7375          var new_changes = $("#changelog .trac-new");
    7476          $("#trac-edit-warning").toggle(new_changes.length != 0);

in reply to:  1 comment:2 by Ryan J Ollos <ryano@…>, 12 years ago

Replying to psuter:

To change the behavior to keeping non-comment changes hidden, this seems to work ok:

I favor this behavior. I've tested out your patch and it seems to work well. My reasoning is, if someone has chosen to hide non-comments, then there is no reason to think they will be interested in the most recent non-comment updates to the ticket.

Either way, the current behavior of the comment box automatically scrolling down the page on refresh is frustrating and I think it would be a good idea to fix this one way or another before 1.0 is released.

comment:3 by Peter Suter, 12 years ago

Milestone: 1.0
Owner: set to Peter Suter

I agree. Other opinions?

comment:4 by Christian Boos, 12 years ago

Well, what about the "conflicting" changes?

Suppose you're editing concurrently with someone else, the other changes come in first, we then tell the user to look for changes marked with the "green" border. In this case I think we'd better show these changes, and deselects Comments only.

in reply to:  4 ; comment:5 by Peter Suter, 12 years ago

Replying to cboos:

Well, what about the "conflicting" changes?

Right. Could we maybe always show new non-comments even if Comments only is checked?

  • trac/htdocs/js/threaded_comments.js

    diff -r bc3cb68f9a90 trac/htdocs/js/threaded_comments.js
    a b  
    2222  var commentsOnly = $("#trac-comments-only-toggle");
    2323  var applyCommentsOnly = function() {
    2424    if (commentsOnly.attr('checked')) {
    25       $("div.change ul.changes").hide();
    26       $("div.change:not(:has(.comment))").hide();
     25      $("div.change:not(.trac-new) ul.changes").hide();
     26      $("div.change:not(.trac-new):not(:has(.comment))").hide();
    2727    } else {
    2828      $("div.change ul.changes").show();
    2929      $("div.change:not(:has(.comment))").show();
Last edited 12 years ago by Peter Suter (previous) (diff)

comment:6 by Peter Suter, 12 years ago

A somewhat related issue that has been bugging me: The default for the Comments only checkbox is False, which compares != 'false', i.e. means True! (The weird != 'false' check is there because the non-default preferences are converted to strings in the DB.)

TODO Either we need to make that check even weirder (comments_prefs.comments_only.toString() != 'false'), or change the default to 'false'. Or switch to entirely different strings like 'comments only' and 'all changes' instead of 'true' and 'false'.

in reply to:  5 ; comment:7 by Remy Blank, 12 years ago

Replying to psuter:

Right. Could we maybe always show new non-comments even if Comments only is checked?

Yes, we should probably do this.

Another thing that I find annoying is that the events where attachments were added disappear when showing "comments only", if the attachment doesn't have a comment. If the attachment does have a comment, it's shown, but without showing the attachment link, which makes it confusing. We should always show attachment links in the ticket changes.

in reply to:  7 comment:8 by Ryan J Ollos <ryano@…>, 12 years ago

Replying to rblank:

Another thing that I find annoying is that the events where attachments were added disappear when showing "comments only", if the attachment doesn't have a comment. If the attachment does have a comment, it's shown, but without showing the attachment link, which makes it confusing. We should always show attachment links in the ticket changes.

I completely agree with this. This has been a confusing issue for me as well.

comment:9 by Peter Suter, 12 years ago

OK, so Comments only should show:

  • Comments
  • New changes
  • Attachments

Possibly not the best named preference :)

  • trac/htdocs/js/threaded_comments.js

    a b  
    2222  var commentsOnly = $("#trac-comments-only-toggle");
    2323  var applyCommentsOnly = function() {
    2424    if (commentsOnly.attr('checked')) {
    25       $("div.change ul.changes").hide();
    26       $("div.change:not(:has(.comment))").hide();
     25      $("div.change:not(.trac-new):not(:has(.trac-field-attachment)) ul.changes").hide();
     26      $("div.change:not(.trac-new):not(:has(.trac-field-attachment)):not(:has(.comment))").hide();
    2727    } else {
    2828      $("div.change ul.changes").show();
    2929      $("div.change:not(:has(.comment))").show();

Those selectors are getting quite long. Should / can they be split up somehow?

in reply to:  9 comment:10 by Remy Blank, 12 years ago

Replying to psuter:

Possibly not the best named preference :)

Not that bad either. In the normal case, it does show only comments. Attachments are a bit of a special case, and alternatively we could also hide them completely when "Comments only" is checked (that is, including the attachment comment). But I prefer seeing them anyway, as they provide a kind of visual anchor when scrolling.

Of course, if you have a better label, feel free to change it :)

Those selectors are getting quite long. Should / can they be split up somehow?

I find them quite readable, so I'm find with them.

Version 0, edited 12 years ago by Remy Blank (next)

comment:11 by Peter Suter, 12 years ago

Resolution: fixed
Status: newclosed

I guess the label is fine as long as it does what the user wants. And with this change that will hopefully be the case most of the time.

Applied in [11124].

comment:12 by Christian Boos, 12 years ago

Severity: normalminor

by Ryan J Ollos, 10 years ago

Attachment: HidePropertyChanges.png added

in reply to:  9 comment:13 by Ryan J Ollos, 10 years ago

Replying to psuter:

OK, so Comments only should show:

  • Comments
  • New changes
  • Attachments

Possibly not the best named preference :)

I've been thinking for a while that we could rename this to Hide property changes. The text is a bit longer, but still fits nicely in the available space. Thoughts?

comment:14 by Christian Boos, 9 years ago

See follow-up in #11794.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter 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.