'Comments only' filter is not respected during autopreview

[Ryan J Ollos <ryano@…>]

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.

[Peter Suter]

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

@@ -69 +69 @@
           if (!$('#trac-comments-oldest').checked())
             $('#trac-comments-oldest').click().change();
           $("#changelog").replaceWith(items.filter("#changelog"));
+          if ($('#trac-comments-only-toggle').attr('checked'))
+            $('#trac-comments-only-toggle').attr('checked', false);
           // Show warning
           var new_changes = $("#changelog .trac-new");
           $("#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

@@ -69 +69 @@
           if (!$('#trac-comments-oldest').checked())
             $('#trac-comments-oldest').click().change();
           $("#changelog").replaceWith(items.filter("#changelog"));
+          if ($('#trac-comments-only-toggle').attr('checked'))
+            $('#trac-comments-only-toggle').click().attr('checked', true);
           // Show warning
           var new_changes = $("#changelog .trac-new");
           $("#trac-edit-warning").toggle(new_changes.length != 0);

[Ryan J Ollos <ryano@…>]

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.

[Peter Suter]

I agree. Other opinions?

[Christian Boos]

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.

[Peter Suter]

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

@@ -22 +22 @@
   var commentsOnly = $("#trac-comments-only-toggle");
   var applyCommentsOnly = function() {
     if (commentsOnly.attr('checked')) {
-      $("div.change ul.changes").hide();
-      $("div.change:not(:has(.comment))").hide();
+      $("div.change:not(.trac-new) ul.changes").hide();
+      $("div.change:not(.trac-new):not(:has(.comment))").hide();
     } else {
       $("div.change ul.changes").show();
       $("div.change:not(:has(.comment))").show();

[Peter Suter]

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'.

[Remy Blank]

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.

[Ryan J Ollos <ryano@…>]

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.

[Peter Suter]

OK, so Comments only should show:

• Comments
• New changes
• Attachments

Possibly not the best named preference :)

trac/htdocs/js/threaded_comments.js

@@ -22 +22 @@
   var commentsOnly = $("#trac-comments-only-toggle");
   var applyCommentsOnly = function() {
     if (commentsOnly.attr('checked')) {
-      $("div.change ul.changes").hide();
-      $("div.change:not(:has(.comment))").hide();
+      $("div.change:not(.trac-new):not(:has(.trac-field-attachment)) ul.changes").hide();
+      $("div.change:not(.trac-new):not(:has(.trac-field-attachment)):not(:has(.comment))").hide();
     } else {
       $("div.change ul.changes").show();
       $("div.change:not(:has(.comment))").show(); 

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

[Remy Blank]

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 fine with them.

[Peter Suter]

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].

[Ryan J Ollos]

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?

[Christian Boos]

See follow-up in #11794.