Collapse commit list by default in sheriff-o-matic.

Collapse commit list by default in sheriff-o-matic.

This CL changes the sheriff-o-matic to collapse the commit list by default.
Each repository has a button you can press to expand the list of commits.

BUG=399715
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180386

[dsinclair, 2014-08-06 14:50:34]

PTAL.

A first pass at collapsing the commit lists.

The one main issue with this is that I've, effectively, made the hash of
expanded commits global. It's attached to the prototype of the ct-commit-list. I
had to do this so that when the list of failures is refreshed we don't lose
information about what was expanded.

To make this work, I named each section <repository><commit range>. So, if there
happens to be two exact matches we'd expand and collapse them at the same time.
This seems to happen more often with repos that aren't blink/chromium (v8 for
example).

It looks like the refreshing of the failures causes the ct-commit-list element
to be re-created. I can go and see if I can make that not happen, but wanted to
put this up as a first pass and see if it was worth the effort.

[ojan, 2014-08-06 23:45:56]

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:34: <repository-info>
It's kinda weird to define new elements that are not actual components. Just use
<div class="repository-info">.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:35: <paper-button raisedbutton
role="button"
I'm sensitive to taking up too much screen real estate for these buttons. Maybe
use a core-icon like in https://codereview.chromium.org/410483002? Then, when
the thing is closed show a XXXX:YYYY style range (and have an onclick on that)
and when it's open, don't show that and just show the commit list. We'd still
show the drop-down button to the left of the commit list in both cases, it just
wouldn't take up so much space.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:40: <commit-list visible="{{
repositoryVisible[repository + _range(repository)] }}">
Use <template if> instead of attribute + display.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:59: //   list.
TL;DR: I think it's worth the effort to fix the underlying issue here.

It's a big-ish project, but I think the right solution for this is to fix the
fact that we blow away the whole model on each iteration. Instead we should
actually update the model. I think once
https://codereview.chromium.org/448503003 lands, you'll be able to do this since
we'll have CTFailureGroup objects in the model each with an almost unique key.
The key isn't totally unique yet, but we'll have to make it be unique for all
the features that depend on this, so lets just assume that it is for now. :)

That way we can add/update/remove the CTFailureGroups as appropriate instead of
blowing away the whole list each time. In addition to making it so we don't have
to go through strange contortions to avoid putting things on the model, it will
mean that we don't blow away and recreate the whole DOM each 30 seconds. :)

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:94: return commits[0].revision + "
: " + commits[commits.length - 1].revision;
Nit: sugarjs has first() and last() on array.

[ojan, 2014-08-11 19:42:45]

I just l g t m'ed https://codereview.chromium.org/458473003/. It has the bug of
losing collapse state on update. If we're going to land that, may as well land
this one with the same bug (i.e. store expand state in the model) and both will
get fixed when we fix the model to not get blown away on each update.

[dsinclair, 2014-08-11 20:20:21]

On 2014/08/11 19:42:45, ojan-only-code-yellow-reviews wrote:
> I just l g t m'ed https://codereview.chromium.org/458473003/. It has the bug
of
> losing collapse state on update. If we're going to land that, may as well land
> this one with the same bug (i.e. store expand state in the model) and both
will
> get fixed when we fix the model to not get blown away on each update.

Ok, I'll get the review back integrated and drop my hack to 'remember' the
expansions over refreshes so it will just start working when we get the other
bits done.

[dsinclair, 2014-08-12 01:11:20]

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:34: <repository-info>
On 2014/08/06 23:45:55, ojan-only-code-yellow-reviews wrote:
> It's kinda weird to define new elements that are not actual components. Just
use
> <div class="repository-info">.

Done.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:35: <paper-button raisedbutton
role="button"
On 2014/08/06 23:45:55, ojan-only-code-yellow-reviews wrote:
> I'm sensitive to taking up too much screen real estate for these buttons.
Maybe
> use a core-icon like in https://codereview.chromium.org/410483002 Then, when
> the thing is closed show a XXXX:YYYY style range (and have an onclick on that)
> and when it's open, don't show that and just show the commit list. We'd still
> show the drop-down button to the left of the commit list in both cases, it
just
> wouldn't take up so much space.


I dropped the button and went with text and an arrow. I found it really weird
when I'd click on the text and it would disappear down to just the arrow, so
I've left the text displaying for now. In general, the amount of space in the
collapsed view is a lot less then the current system so I think it's better.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:40: <commit-list visible="{{
repositoryVisible[repository + _range(repository)] }}">
On 2014/08/06 23:45:55, ojan-only-code-yellow-reviews wrote:
> Use <template if> instead of attribute + display.

Done.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:59: //   list.
On 2014/08/06 23:45:55, ojan-only-code-yellow-reviews wrote:
> TL;DR: I think it's worth the effort to fix the underlying issue here.
> 
> It's a big-ish project, but I think the right solution for this is to fix the
> fact that we blow away the whole model on each iteration. Instead we should
> actually update the model. I think once
> https://codereview.chromium.org/448503003 lands, you'll be able to do this
since
> we'll have CTFailureGroup objects in the model each with an almost unique key.
> The key isn't totally unique yet, but we'll have to make it be unique for all
> the features that depend on this, so lets just assume that it is for now. :)
> 
> That way we can add/update/remove the CTFailureGroups as appropriate instead
of
> blowing away the whole list each time. In addition to making it so we don't
have
> to go through strange contortions to avoid putting things on the model, it
will
> mean that we don't blow away and recreate the whole DOM each 30 seconds. :)

Acknowledged.

https://codereview.chromium.org/441393002/diff/20001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:94: return commits[0].revision + "
: " + commits[commits.length - 1].revision;
On 2014/08/06 23:45:55, ojan-only-code-yellow-reviews wrote:
> Nit: sugarjs has first() and last() on array.

Done.

[ojan, 2014-08-12 01:44:23]

This will need tests as well.

https://codereview.chromium.org/441393002/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/441393002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:42: <core-icon
icon="arrow-drop-down"></core-icon>
My previous advice turned out to be wrong I think.
https://codereview.chromium.org/458473003 uses a paper-icon-button for this. I
think that's what you want. Sorry for leading you in circles.

https://codereview.chromium.org/441393002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:102: _toggleRepository:
function(event, detail, sender, target) {
I was picturing that this would work more like
https://codereview.chromium.org/458473003. Particularly, that it would update an
expanded property on a model object. The problem here is that the old code was
written poorly. ct-commit-list should take a CTCommitList model class instead of
first/last/commits. The model class would have a way of vending only the
non-empty repository names and the commits for those repositories.

Incidentally, this also helps improve the code to prepare for the git migration
since doing arithmetic on first/last doesn't make any sense when you have git
hashes.

[dsinclair, 2014-08-15 14:40:05]

PTAL.

This is built on top of https://codereview.chromium.org/464963003/.

https://codereview.chromium.org/441393002/diff/40001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/441393002/diff/40001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:42: <core-icon
icon="arrow-drop-down"></core-icon>
On 2014/08/12 01:44:23, ojan-only-code-yellow-reviews wrote:
> My previous advice turned out to be wrong I think.
> https://codereview.chromium.org/458473003 uses a paper-icon-button for this. I
> think that's what you want. Sorry for leading you in circles.

Done.

[ojan, 2014-08-15 17:51:50]

lgtm

Thanks again for doing this!

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:26: <paper-icon-button
icon="more-vert"
Nit: use unfold-more instead of more-vert to be consistent with
https://codereview.chromium.org/475323002

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:50: _expand: function(event,
detail, sender, target) {
Maybe call this _toggle?

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:52: var range =
sender.getAttribute('repositoryRange');
Do sender.repositoryName/sender.repositoryRange not work? This isn't very well
documented, but in general you should never have to use get/setAttribute with
polymer components. Properties are preferred because then you can pass rich
things like objects instead of needing to serialize/deserialize to a string.

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:54: for(var i = 0; i <
this.repositories.length; i++) {
Missing space after "for". Also, if you wanted you could use array's find
function.

var r = this.repositories.find(function(r) {
  return r.name === repo && r.range === range;
});
r.expected = !r.expanded;

Also, do you actually need to check that the ranges are equal? Isn't just
checking repository.name sufficient?

[dsinclair, 2014-08-15 18:58:34]

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-commit-list.html (right):

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:26: <paper-icon-button
icon="more-vert"
On 2014/08/15 17:51:49, ojan-only-code-yellow-reviews wrote:
> Nit: use unfold-more instead of more-vert to be consistent with
> https://codereview.chromium.org/475323002

Done.

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:50: _expand: function(event,
detail, sender, target) {
On 2014/08/15 17:51:50, ojan-only-code-yellow-reviews wrote:
> Maybe call this _toggle?

Done.

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:52: var range =
sender.getAttribute('repositoryRange');
On 2014/08/15 17:51:49, ojan-only-code-yellow-reviews wrote:
> Do sender.repositoryName/sender.repositoryRange not work? This isn't very well
> documented, but in general you should never have to use get/setAttribute with
> polymer components. Properties are preferred because then you can pass rich
> things like objects instead of needing to serialize/deserialize to a string.

That doesn't seem to work in this case? I can never seem to get the data back
off of sender? Only by doing getAttribute do I get a non-blank string.

I really wanted to just attach repository="{{ repository }}" when I created the
button, but that always gives me back [Object object].

https://codereview.chromium.org/441393002/diff/60001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-commit-list.html:54: for(var i = 0; i <
this.repositories.length; i++) {
On 2014/08/15 17:51:49, ojan-only-code-yellow-reviews wrote:
> Missing space after "for". Also, if you wanted you could use array's find
> function.
> 
> var r = this.repositories.find(function(r) {
>   return r.name === repo && r.range === range;
> });
> r.expected = !r.expanded;
> 
> Also, do you actually need to check that the ranges are equal? Isn't just
> checking repository.name sufficient?

Done.

Don't need to check range, that was a hold-over from the global expanded hash
version.

[dsinclair, 2014-08-15 19:00:03]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2014-08-15 19:00:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dsinclair@chromium.org/441393002/80001

[commit-bot: I haz the power, 2014-08-15 19:01:48]

Committed patchset #4 (80001) as 180386