Page MenuHomePhabricator

Gerrit closes all but last comment after loading change view
Closed, ResolvedPublic

Description

When navigating to a change in Gerrit (e.g.:
https://gerrit.wikimedia.org/r/#/c/55048/
) all (but the most recent) comments are initially closed.

Thereby, it is hard to follow a change's discussion
without clicking 'Expand all'.

Allow the user to specify whether to use 'Expand Recent',
'Expand all', ... as default strategy for comment
visibility.


Version: wmf-deployment
Severity: normal

Details

Reference
bz46774

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 1:34 AM
bzimport added projects: Gerrit, Upstream.
bzimport set Reference to bz46774.
bzimport added a subscriber: Unknown Object (MLST).

I'd suggest a different solution - no user options, let's just expand everything apart from autogenerated comments (both the ones generated by gerrit itself, gerrit pretending to be you (for new patchsets etc.), and by Jenkins).

According to Chad, this is possible and seems reasonably easy to do. Also according to Chad, it would be nice to get https://gerrit-review.googlesource.com/#/c/43520/ merged upstream first so we can nicely inject jQuery instead of hardcoding it in the script.

Example barely tested "mockup" code (there are some more cases to handle), requiring jQuery:

$(window).on('hashchange', function() {

$('.commentPanel:not(.commentPanelLast) .commentPanelHeader')
  .filter(function(){
    var isJenkins = $(this).find('.commentPanelAuthorCell').text() == 'jenkins-bot';
    var isNewPatchset = $(this).find('.commentPanelSummaryCell').text().match(/^\s*Uploaded patch set \d+\.\s*$/);
    return !isJenkins && !isNewPatchset;
  })
  .click()

}).trigger('hashchange');

This seems to work for me on Opera and not throw exceptions.

Created attachment 12017
Code from comment 1 as attachment

Adding the Ccode from comment 1 as attachment to prevent Bugzilla from mangling it.

Attached:

The more we can get upstream, the less we have to maintain
ourselves :-)

A patch allowing the user to specify whether to use
'Expand Recent', 'Expand all', ... as default strategy
for comment visibility is uploaded to upstream gerrit for
review at:

https://gerrit-review.googlesource.com/#/c/44162/

(In reply to comment #0)

Allow the user to specify whether to use 'Expand Recent',
'Expand all', ... as default strategy for comment
visibility.

I'm not sure this is the correct approach to take here. The default behavior should just be sensible. It looks like Gerrit already supports a "expand recent comments" feature. Could the site-wide default be changed here?

(In reply to comment #3)

The more we can get upstream, the less we have to maintain
ourselves :-)

Thanks for working on this.

A patch allowing the user to specify whether to use
'Expand Recent', 'Expand all', ... as default strategy
for comment visibility is uploaded to upstream gerrit for
review at:

https://gerrit-review.googlesource.com/#/c/44162/

In software development and design, the defaults are hugely important as most users never set custom preferences. It's unclear whether this proposed changeset changes the site-wide default or just adds a user preference. As I said above, I'd rather not see a new user preference here, but if one is added, I think there's still the unresolved issue of a sane default here.

There's no time when I can think of that the current behavior (collapsing all but the most recent comment) would be appropriate. Auto-expanding the most recent comments (plural) seems like a sensible default site-wide and would likely eliminate the need for a user preference.

All that said, the JavaScript-based solutions have some advantages, such as auto-collapsing certain kinds of comments (such as those from jenkins-bot).

(In reply to comment #3)

[ Patch to set default comment visibility ]
https://gerrit-review.googlesource.com/#/c/44162/

This change has been merged upstream.

(In reply to comment #4)

Could the site-wide default be changed here?

No. It's only about user's choice.

Gerrit's current default visibility works well for me :-)

But given the comments of others, I seem to be the outlier. So I uploaded
an upstream change to change the default comment visibility strategy:
https://gerrit-review.googlesource.com/#/c/44275/

Aklapper triaged this task as Lowest priority.Mar 23 2015, 6:59 PM

Hi in gerrit 2.12, they now close all comments by default. There is a button you press at the top that expands all of them.

Can this be closed?

greg claimed this task.
greg subscribed.

Yep.