ChangeScreen2 is now in master

[Shawn Pearce]

My new ChangeScreen2 is mostly feature complete... so I merged it to
master where we can continue to iterate on the concept.

To use the screen modify the URL to be /#/c2/... instead of /#/c/...
it remembers this decision for the current tab only. To get back to
the old screen load /#/c/... again, if its already /#/c/... in the
address bar just reload the page in the browser.

Missing features:
- Add reviewer(s)
- Remove reviewer(s)
- Edit commit message
- Diff files in two patch sets
- Dependencies and dependents
- Download links by URL and action (cherry-pick, checkout, etc.)

Obviously these must be fixed to move forward with ChangeScreen2.

I am going to try to deploy tip of master to gerrit-review tomorrow. I
will reply to this thread when it is live and its easier for users to
try ChangeScreen2 in a real side-by-side comparison with the existing
ChangeScreen.

[Edwin Kempin]

Hey cool, I'm curious to try it out :)

2013/7/15 Shawn Pearce <s...@google.com>

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Shawn Pearce]

ChangeScreen2 is live on gerrit-review. We have a few issues that are
non-critical:

- Avatars don't display quite where I wanted them in the History
section. The alignment is incorrect. I failed to test this correctly
locally.

- CodeMirror JS doesn't seem to always load? I had trouble with it
this morning. It may have been an artifact of testing before
deployment completed, or it may be an issue with our unique build
environment. Hitting reload seemed to "fix" things but I need to
investigate further.

[Thomas Swindells (tswindel)]

The new screen is looking good, and I'm sure it's going to get even better.

One minor thing I've noticed is it's a bit confusing that on merged changes the strategy says "Cannot Merge" in bold red.

Thomas

[Robert Ward]

I like the look and feel of the new screen, however I can see a couple of things which are missing/non obvious that haven't been mentioned already.

    I can't see where it states the number of review comments on the file.

    There seems no obvious way to compare the list of files modified in given
    changes, before when the patchsets were all displayed on the one page
    you could compare the two very easily.

    There is no expand all on the comments.

    There are a couple of alignment issues on my setup, The "Change X by Y"
    banner slightly covers the menu(open, merge Abandoned etc) - Firefox 22.0

    There doesn't seem to be a permalink for the change.

    Given that you have to select a different patchset and update the page
    it seems a bit more difficult to see which authors have created which patchsets.

Overall I like the look and think it will make reviewing easier and also simplify the interface for new users.

Cheers,

Rob

------------------------------

Rob Ward
www.rob-ward.co.uk

[Edwin Kempin]

I liked the feature of the old ChangeScreen that I was able to review (and change)
my inline comments on the moment of publish. This seems to be missing with the new screen.

2013/7/16 Robert Ward <robert....@googlemail.com>

[Dariusz Luksza]

The new ChangeScreen looks for me like ported vi interface... I'm not
sure whether it is good or bad. Also I've noticed that font in
CodeMirror could be slightly bigger. Other than that, new version
feels more readable (except of yellowish 'Code-review' and 'Verify'
labels).

Best regards

GSM: +49 017 445 41235
Blog: http://luksza.org
LinkedIn: http://www.linkedin.com/in/dariuszluksza

[Shawn Pearce]

On Mon, Jul 15, 2013 at 9:52 PM, Thomas Swindells (tswindel)
<tswi...@cisco.com> wrote:
> The new screen is looking good, and I'm sure it's going to get even better.

Thanks.

> One minor thing I've noticed is it's a bit confusing that on merged changes the strategy says "Cannot Merge" in bold red.

This is a bug. I have added it to my list of issues to be addressed, thanks.

[Shawn Pearce]

On Tue, Jul 16, 2013 at 12:02 AM, Dariusz Luksza
<dariusz...@gmail.com> wrote:
> The new ChangeScreen looks for me like ported vi interface... I'm not
> sure whether it is good or bad.

Eh? Not sure what you mean by this. I do use vi so maybe something
about a simple interface rubbed off. I should also confess not all of
this design is mine, I borrowed some aspects from some internal tools
we have at Google.

Or do you mean we have enabled vi keybindings in CodeMirror? We did
that since the old UI used j/k for down/up like vi. It seemed to make
sense to just enable the rest of the keys for navigation. Michael will
soon start working on range selection for comments, so we wanted to
enable character level navigation to support that. The vi keybindings
were an easy solution.

> Also I've noticed that font in
> CodeMirror could be slightly bigger.

Maybe. We'll have to see what we can do there.

> Other than that, new version
> feels more readable (except of yellowish 'Code-review' and 'Verify'
> labels).

Yes I agree. This looked better on the airplane after 4 glasses of
wine than it does sober on my desktop. :-)

That color will change.

[Shawn Pearce]

On Mon, Jul 15, 2013 at 11:32 PM, Edwin Kempin <edwin....@gmail.com> wrote:
> I liked the feature of the old ChangeScreen that I was able to review (and
> change)
> my inline comments on the moment of publish. This seems to be missing with
> the new screen.

Me too. This is a missing feature that I forgot to document in the
commit message. On the other hand I do like how much faster the reply
box is to open, and how its completely keyboard driven.

Did you discover the hidden magic where typing "LGTM" as the first
part of the message updates the radio boxes to +2/+1? :-)

[Shawn Pearce]

On Mon, Jul 15, 2013 at 10:17 PM, Robert Ward
<robert....@googlemail.com> wrote:
> I like the look and feel of the new screen, however I can see a couple of
> things which are missing/non obvious that haven't been mentioned already.
>
> I can't see where it states the number of review comments on the file.

Missing feature. Thank you for pointing it out.

> There seems no obvious way to compare the list of files modified in
> given
> changes, before when the patchsets were all displayed on the one page
> you could compare the two very easily.

This is a known missing feature. My plan is to add a widget to the
right hand side of the "Files" bar that lets you select what the
compare should be.

> There is no expand all on the comments.

Intentionally not implemented. The new history view starts with all
comments collapsed because I suspected for most current usage most
text fits within the snippet view the collapsed display uses. If we
were also showing inline comments I might feel differently, but that
has been an open feature request for nearly 5 years. *sigh*

I'm open to reviewing a change that adds an expand all. But I wanted
to at least try using the screen without one to see just how important
that button is.

> There are a couple of alignment issues on my setup, The "Change X by Y"
> banner slightly covers the menu(open, merge Abandoned etc) - Firefox
> 22.0

That is... bad, thanks for the report. I only tested the UI on Chrome
Linux and Chrome Mac and did not test with any other browsers yet.
Given the CSS used here I'm not sure why Firefox is positioning the
banner as you said... it just seems wrong. Obviously the CSS is at
fault. Yay!

> There doesn't seem to be a permalink for the change.

Its there, the reload almost-full-circle arrow to the right of the
status is also the permalink.

> Given that you have to select a different patchset and update the page
> it seems a bit more difficult to see which authors have created which
> patchsets.

Noted. I want to replace the current Revisions pulldown in the top
right with a different UI that displays information from each commit.
This would allow us to include the author/committer information.

> Overall I like the look and think it will make reviewing easier and also
> simplify the interface for new users.

Thanks.

[Brad Larson]

Very nice and fun!  I noticed that https://gerrit-review.googlesource.com/#/c/35520/6/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java,codemirror seems to be stacking the 'skipped x common lines' bars at the very bottom of the window?  Some other files in that patch set don't seem to do that...

The file delta size boxes on the change overview page seem to be summing the numbers... so instead of +1 -1 I see 0.  See https://gerrit-review.googlesource.com/#/c2/35520/.

I'm a huge fan of being able to copy-paste text from the diff view now - it is like Christmas and 4th of July all in one!

On Monday, July 15, 2013 7:26:13 PM UTC-5, Shawn Pearce wrote:

ChangeScreen2 is live on gerrit-review. We have a few issues that are
non-critical:

- Avatars don't display quite where I wanted them in the History
section. The alignment is incorrect. I failed to test this correctly
locally.

Any chance you could turn on avatars for gerrit-review so we could see this?  ;-)

[Dariusz Luksza]

On 07/16/2013 06:02 PM, Shawn Pearce wrote:
> On Tue, Jul 16, 2013 at 12:02 AM, Dariusz Luksza
> <dariusz...@gmail.com> wrote:
>> The new ChangeScreen looks for me like ported vi interface... I'm not
>> sure whether it is good or bad.
>
> Eh? Not sure what you mean by this. I do use vi so maybe something
> about a simple interface rubbed off. I should also confess not all of
> this design is mine, I borrowed some aspects from some internal tools
> we have at Google.
>
> Or do you mean we have enabled vi keybindings in CodeMirror? We did
> that since the old UI used j/k for down/up like vi. It seemed to make
> sense to just enable the rest of the keys for navigation. Michael will
> soon start working on range selection for comments, so we wanted to
> enable character level navigation to support that. The vi keybindings
> were an easy solution.

I meant that layout of change info table, commit info table, file diff table, comments and borders around those sections remind me spited window in vi. At least that was my first impression ;)

As far as I see key bindings didn't change comparing to the previous version and I really like them.

>> Also I've noticed that font in
>> CodeMirror could be slightly bigger.
>
> Maybe. We'll have to see what we can do there.
>
>> Other than that, new version
>> feels more readable (except of yellowish 'Code-review' and 'Verify'
>> labels).
>
> Yes I agree. This looked better on the airplane after 4 glasses of
> wine than it does sober on my desktop. :-)
>
> That color will change.
>

[Shawn Pearce]

On Tue, Jul 16, 2013 at 11:05 AM, Brad Larson <bkla...@gmail.com> wrote:
> Very nice and fun! I noticed that
> https://gerrit-review.googlesource.com/#/c/35520/6/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java,codemirror
> seems to be stacking the 'skipped x common lines' bars at the very bottom of
> the window? Some other files in that patch set don't seem to do that...

I forwarded this to Michael Zhou since he is working on that part of the UI.

> The file delta size boxes on the change overview page seem to be summing the
> numbers... so instead of +1 -1 I see 0. See
> https://gerrit-review.googlesource.com/#/c2/35520/.

Yes its showing the total number of lines added. +1 and -1 is 0 lines added.

> I'm a huge fan of being able to copy-paste text from the diff view now - it
> is like Christmas and 4th of July all in one!

:-)

> On Monday, July 15, 2013 7:26:13 PM UTC-5, Shawn Pearce wrote:
>>
>> ChangeScreen2 is live on gerrit-review. We have a few issues that are
>> non-critical:
>>
>> - Avatars don't display quite where I wanted them in the History
>> section. The alignment is incorrect. I failed to test this correctly
>> locally.
>
> Any chance you could turn on avatars for gerrit-review so we could see this?
> ;-)

:-)

Coming soon-ish. I can't enable it with the way avatars work right
now. The /accounts/<id|email>/avatar URL needs to disappear.

>> - CodeMirror JS doesn't seem to always load? I had trouble with it
>> this morning. It may have been an artifact of testing before
>> deployment completed, or it may be an issue with our unique build
>> environment. Hitting reload seemed to "fix" things but I need to
>> investigate further.

I can't reproduce this anymore. I think it was caused by the partial
rollout, for a brief period some servers in our cluster had a
different version of JS than others.

[Shawn Pearce]

On Tue, Jul 16, 2013 at 11:05 AM, Brad Larson <bkla...@gmail.com> wrote:

> I'm a huge fan of being able to copy-paste text from the diff view now - it
> is like Christmas and 4th of July all in one!

Has anyone tried search within a CodeMirror view? Use / key to start
the CodeMirror search. :-)

[Brad Larson]

On Tuesday, July 16, 2013 1:45:24 PM UTC-5, Shawn Pearce wrote:

On Tue, Jul 16, 2013 at 11:05 AM, Brad Larson <bkla...@gmail.com> wrote:
> Very nice and fun!  I noticed that
> https://gerrit-review.googlesource.com/#/c/35520/6/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java,codemirror
> seems to be stacking the 'skipped x common lines' bars at the very bottom of
> the window?  Some other files in that patch set don't seem to do that...

I  forwarded this to Michael Zhou since he is working on that part of the UI.

> The file delta size boxes on the change overview page seem to be summing the
> numbers... so instead of +1 -1 I see 0.  See
> https://gerrit-review.googlesource.com/#/c2/35520/.

Yes its showing the total number of lines added. +1 and -1 is 0 lines added.

Just my two cents... I prefer how the old page split them apart.  If I see 0 lines added, it might be a +1 and -1 or it might be a +500 and -500.  The bar graph helps, but I like seeing the raw numbers as well.  Certainly not a deal breaker for me though, just a personal preference.

Also, I am a fan of how the diff layout allows the possibility of the vertical overview bar concept in the future :)

[Chad Horohoe]

I noticed that both / and ? will kick off a search (a-la vim). The latter seems to

conflict with bringing up the shortcuts menu.

-Chad 

[Shawn Pearce]

On Mon, Jul 15, 2013 at 10:17 PM, Robert Ward
<robert....@googlemail.com> wrote:

> There are a couple of alignment issues on my setup, The "Change X by Y"
> banner slightly covers the menu(open, merge Abandoned etc) - Firefox
> 22.0

I now see this alignment issue even in Chrome. I think it has to do
with the way the header works on gerrit-review. My local test instance
does not have a site header. The site header on gerrit-review is
"interesting" because it pulls itself up and into the left edge, even
though its in the DOM below the menu. Looks like the header isn't
saving enough space for the menu and this is allowing the new
ChangeScreen2 div to start earlier on the page.

I'm guessing that even the old ChangeScreen had this alignment issue,
we just got lucky because of a large margin on the huge "Change
I.....: ...." header text at the top of the page.

[Dave Borowitz]

On Tue, Jul 16, 2013 at 11:52 AM, Brad Larson <bkla...@gmail.com> wrote:

On Tuesday, July 16, 2013 1:45:24 PM UTC-5, Shawn Pearce wrote:

On Tue, Jul 16, 2013 at 11:05 AM, Brad Larson <bkla...@gmail.com> wrote:
> Very nice and fun!  I noticed that
> https://gerrit-review.googlesource.com/#/c/35520/6/gerrit-gwtui/src/main/java/com/google/gerrit/client/Gerrit.java,codemirror
> seems to be stacking the 'skipped x common lines' bars at the very bottom of
> the window?  Some other files in that patch set don't seem to do that...

I  forwarded this to Michael Zhou since he is working on that part of the UI.

> The file delta size boxes on the change overview page seem to be summing the
> numbers... so instead of +1 -1 I see 0.  See
> https://gerrit-review.googlesource.com/#/c2/35520/.

Yes its showing the total number of lines added. +1 and -1 is 0 lines added.

Just my two cents... I prefer how the old page split them apart.  If I see 0 lines added, it might be a +1 and -1 or it might be a +500 and -500.  The bar graph helps, but I like seeing the raw numbers as well.  Certainly not a deal breaker for me though, just a personal preference.

How about something like git log --stat, which does kind of a stacked bar graph with +s and -s.

 

Also, I am a fan of how the diff layout allows the possibility of the vertical overview bar concept in the future :)

 

> I'm a huge fan of being able to copy-paste text from the diff view now - it
> is like Christmas and 4th of July all in one!

:-)

> On Monday, July 15, 2013 7:26:13 PM UTC-5, Shawn Pearce wrote:
>>
>> ChangeScreen2 is live on gerrit-review. We have a few issues that are
>> non-critical:
>>
>> - Avatars don't display quite where I wanted them in the History
>> section. The alignment is incorrect. I failed to test this correctly
>> locally.
>
> Any chance you could turn on avatars for gerrit-review so we could see this?
> ;-)

:-)

Coming soon-ish. I can't enable it with the way avatars work right
now. The /accounts/<id|email>/avatar URL needs to disappear.

>> - CodeMirror JS doesn't seem to always load? I had trouble with it
>> this morning. It may have been an artifact of testing before
>> deployment completed, or it may be an issue with our unique build
>> environment. Hitting reload seemed to "fix" things but I need to
>> investigate further.

I can't reproduce this anymore. I think it was caused by the partial
rollout, for a brief period some servers in our cluster had a
different version of JS than others.

--

[David Pursehouse]

On 07/17/2013 12:44 AM, Shawn Pearce wrote:
>> There is no expand all on the comments.
>
> Intentionally not implemented. The new history view starts with all
> comments collapsed because I suspected for most current usage most
> text fits within the snippet view the collapsed display uses. If we
> were also showing inline comments I might feel differently, but that
> has been an open feature request for nearly 5 years. *sigh*
>

What about the "Comment Visibility" setting from the user preferences?

Will support for that be implemented, or the setting obsoleted?

[Bruce Zu]

On Tuesday, July 16, 2013 11:44:42 PM UTC+8, Shawn Pearce wrote:

On Mon, Jul 15, 2013 at 10:17 PM, Robert Ward
<robert....@googlemail.com> wrote:
> I like the look and feel of the new screen, however I can see a couple of
> things which are missing/non obvious that haven't been mentioned already.
>
>     I can't see where it states the number of review comments on the file.

Missing feature. Thank you for pointing it out.

>     There seems no obvious way to compare the list of files modified in
> given
>     changes, before when the patchsets were all displayed on the one page
>     you could compare the two very easily.

This is a known missing feature. My plan is to add a widget to the
right hand side of the "Files" bar that lets you select what the
compare should be.

 in this chance of the new widget, for comparing rebased  patchsets case, will you also plan to provide filter function to filter out those irrelated file or code block :)  

[Saša Živkov]

This is great I often need that.

What would be really great is to have such a search over the whole patch-set delta.

For example: if I am reviewing a change and see a new class/interface/method introduced,

then I would like to see where it is used. Having a possibility to search within the changed

lines only through the whole patch-set would make that possible :-)

[Marcelo Avila de Oliveira]

16-Jul-13 - Shawn Pearce <s...@google.com> wrote:

16-Jul-13 - Brad Larson <bkla...@gmail.com> wrote:

> The file delta size boxes on the change overview page seem to be summing the
> numbers... so instead of +1 -1 I see 0.  See
> https://gerrit-review.googlesource.com/#/c2/35520/.

Yes its showing the total number of lines added. +1 and -1 is 0 lines added.

This is not the usual way that Git show stats...

git log --stat example:

  etc/cds.conf            | 2 +-

  install/gerrit/setup.sh | 3 ++-

  2 files changed, 3 insertions(+), 2 deletions(-)

It would be better to calc +1 + |-1| = 2 instead of +1 + (-1) = 0

--

Marcelo Ávila de Oliveira

CPqD - Information Technology Engineer

Tel.: +55 19 3705-4125

mav...@cpqd.com.br

www.cpqd.com.br

2013/7/16 Shawn Pearce <s...@google.com>

[Edwin Kempin]

2013/7/17 Marcelo Avila de Oliveira <mav...@cpqd.com.br>

16-Jul-13 - Shawn Pearce <s...@google.com> wrote:

16-Jul-13 - Brad Larson <bkla...@gmail.com> wrote:

> The file delta size boxes on the change overview page seem to be summing the
> numbers... so instead of +1 -1 I see 0.  See
> https://gerrit-review.googlesource.com/#/c2/35520/.

Yes its showing the total number of lines added. +1 and -1 is 0 lines added.

This is not the usual way that Git show stats...

git log --stat example:

  etc/cds.conf            | 2 +-

  install/gerrit/setup.sh | 3 ++-

  2 files changed, 3 insertions(+), 2 deletions(-)

It would be better to calc +1 + |-1| = 2 instead of +1 + (-1) = 0

+1
 

[Shawn Pearce]

On Wed, Jul 17, 2013 at 5:31 AM, Edwin Kempin <edwin....@gmail.com> wrote:
> 2013/7/17 Marcelo Avila de Oliveira <mav...@cpqd.com.br>
>>
>> 16-Jul-13 - Shawn Pearce <s...@google.com> wrote:
>>
>>> 16-Jul-13 - Brad Larson <bkla...@gmail.com> wrote:
>>>
>>> > The file delta size boxes on the change overview page seem to be
>>> > summing the
>>> > numbers... so instead of +1 -1 I see 0. See
>>> > https://gerrit-review.googlesource.com/#/c2/35520/.
>>>
>>> Yes its showing the total number of lines added. +1 and -1 is 0 lines
>>> added.
>>
>>
>> This is not the usual way that Git show stats...
>>
>> git log --stat example:
>>
>> etc/cds.conf | 2 +-
>> install/gerrit/setup.sh | 3 ++-
>> 2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> It would be better to calc +1 + |-1| = 2 instead of +1 + (-1) = 0
>
> +1

https://gerrit-review.googlesource.com/47950

[Shawn Pearce]

My current intention is to obsolete this setting. However if we show
the inline comments it starts to make sense again as a preference, so
basically I am on the fence. I don't like the option myself, and I
think its a UI flaw that we even need it.

[Chad Horohoe]

Probably a flaw, but I think it's too early to axe the setting unless we've

found a better solution. I'm like you and don't use it, but I've got a number

of power users who do and would be quite upset to see the behavior fall

back to always collapsed.

-Chad 

[David Ostrovsky]

On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:

My new ChangeScreen2 is mostly feature complete... so I merged it to
master where we can continue to iterate on the concept.

Why is Verified+1 red? i would expect it to be green and Verified-1 (missing?) to be red.

How about to move some change specific buttons from action panel to change header, like 

Abandon & Restore as icons, left to reload icon? To better differentiate between change

and revision specific actions.

[Shawn Pearce]

On Thu, Jul 18, 2013 at 1:28 AM, David Ostrovsky
<david.o...@gmail.com> wrote:
> On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:
>>
>> My new ChangeScreen2 is mostly feature complete... so I merged it to
>> master where we can continue to iterate on the concept.
>>
>
> Why is Verified+1 red? i would expect it to be green and Verified-1
> (missing?) to be red.

Puzzled. It should be green. I see Verified+1 as green as you expect.
Was there also a Verified-1 at the same time?

> How about to move some change specific buttons from action panel to change
> header, like
> Abandon & Restore as icons, left to reload icon? To better differentiate
> between change
> and revision specific actions.

I'm not sure this matters. The user is reading a view, they want to
interact with the view they have open. They don't really care what
that means.

[David Ostrovsky]

Am Donnerstag, 18. Juli 2013 17:34:36 UTC+2 schrieb Shawn Pearce:

On Thu, Jul 18, 2013 at 1:28 AM, David Ostrovsky
<david.o...@gmail.com> wrote:
> On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:
>>
>> My new ChangeScreen2 is mostly feature complete... so I merged it to
>> master where we can continue to iterate on the concept.
>>
>
> Why is Verified+1 red? i would expect it to be green and Verified-1
> (missing?) to be red.

Puzzled. It should be green. I see Verified+1 as green as you expect.
Was there also a Verified-1 at the same time?

nope, there isn't. Interestingly I see it on both latest FF and latest Chrome (Linux).
I uploaded the Chrome version for you (Version 28.0.1500.89 beta):

http://ostrovsky.org/gerrit-review.googlesource.I2e28dbf5.html

> How about to move some change specific buttons from action panel to change
> header, like
> Abandon & Restore as icons, left to reload icon? To better differentiate
> between change
> and revision specific actions.

I'm not sure this matters. The user is reading a view, they want to
interact with the view they have open. They don't really care what
that means.

Agreed, but consider that a plugin (or why not even core?) may want to add discard revision action.
Then it's matter if you have Discard/Delete right to Abandon: that's hard to grasp. Moving some change
specific actions to change header with cool icons, like cherries and garbage can may help. 

Given that all actions now are actually UiActions, we could very easy make
the action places and action look and feel customizable, i. e. we could define a couple of places
inside change screen and admins (gerrit.config) or even project owner (project.config) could easy
move core- and plugin-specific actions to different places with custom icons, loaded from site specific dir.

They would just override per action the attributes, that we hard coded in UiAction.Description
class and additionally type = (button | icon), style, icon name, always hide, ...

[Shawn Pearce]

On Thu, Jul 18, 2013 at 9:45 AM, David Ostrovsky

<david.o...@gmail.com> wrote:
> Am Donnerstag, 18. Juli 2013 17:34:36 UTC+2 schrieb Shawn Pearce:
>>
>> On Thu, Jul 18, 2013 at 1:28 AM, David Ostrovsky
>> <david.o...@gmail.com> wrote:
>> > On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:
>> >>
>> >> My new ChangeScreen2 is mostly feature complete... so I merged it to
>> >> master where we can continue to iterate on the concept.
>> >>
>> >
>> > Why is Verified+1 red? i would expect it to be green and Verified-1
>> > (missing?) to be red.
>>
>> Puzzled. It should be green. I see Verified+1 as green as you expect.
>> Was there also a Verified-1 at the same time?
>
>
> nope, there isn't. Interestingly I see it on both latest FF and latest
> Chrome (Linux).
> I uploaded the Chrome version for you (Version 28.0.1500.89 beta):
>
> http://ostrovsky.org/gerrit-review.googlesource.I2e28dbf5.html

Are you talking about the Verified+1 that is in red above the commit
message? Its the button that is red. Clicking it adds Verified+1
immediately. Its red as a warning to encourage you to push it. :-)

Hmm, sure. This is one reason I moved to UiAction.Description. We can
now freely add optional styling features. :-)

[Chad Horohoe]

On Thu, Jul 18, 2013 at 11:02 AM, Shawn Pearce <s...@google.com> wrote:

On Thu, Jul 18, 2013 at 9:45 AM, David Ostrovsky
<david.o...@gmail.com> wrote:
> Am Donnerstag, 18. Juli 2013 17:34:36 UTC+2 schrieb Shawn Pearce:
>>
>> On Thu, Jul 18, 2013 at 1:28 AM, David Ostrovsky
>> <david.o...@gmail.com> wrote:
>> > On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:
>> >>
>> >> My new ChangeScreen2 is mostly feature complete... so I merged it to
>> >> master where we can continue to iterate on the concept.
>> >>
>> >
>> > Why is Verified+1 red? i would expect it to be green and Verified-1
>> > (missing?) to be red.
>>
>> Puzzled. It should be green. I see Verified+1 as green as you expect.
>> Was there also a Verified-1 at the same time?
>
>
> nope, there isn't. Interestingly I see it on both latest FF and latest
> Chrome (Linux).
> I uploaded the Chrome version for you (Version 28.0.1500.89 beta):
>
> http://ostrovsky.org/gerrit-review.googlesource.I2e28dbf5.html

Are you talking about the Verified+1 that is in red above the commit
message? Its the button that is red. Clicking it adds Verified+1
immediately. Its red as a warning to encourage you to push it. :-)

This is a usability concern. The color red implies errors, canceling, or

an otherwise "bad" action. For progressive actions you tend to want

cooler colors, like blue or green.

-Chad 

[David Ostrovsky]

the red colour for "Verified+1" is well, at least misleading. If i
would try to sell it that would be the way to go:

Hey guys, i got a button for you, to mark your change as sane/green,
but the button doesn't have a confirmation message, making your change
immediately
sane/green, that why the button is red
;-)

I would suggest to convert "Verified" to grey label, and to have two
icons "+1" (green) and "-1" (red).

I just wanted to mention another aspect that i very like with the new
design: simplicity.
The main advantage for me is that we show only one patch set on the screen.

Once we have buildbot notifications, we are going to lose the simplicity again.
For us that means 3-5 platform specific builds per patch set and thus
3-5 x 2 + 1 messages in the history table,
for start message per platform, end message per platform and combined
result message),
so that you get such a mess [1], mixing human and bot notifications.

I would love to address that problem with the new change screen. The
best approach that I've heard so far
was Martin's non-intrusive design suggestion:

Gerrit side:
1. extend the existing reviewer table to be reviewer/verifier table
2. extend that table with "href" column: link to the result of that buildbot
3. extend gerrit review SSH command to natively consume that --href
command option and not only work with a message.
(schema migration is needed to make it work)
4. silently discard/filter stale buildbot notifications (for old patch
set) and only show in the reviewer/verifier table the buildbot
notifications for current patch set (and filter out the build bot
notifications completely from the history table, may be optionally?).
Obviously, Gerrit must know who is a buildbot and who is a human.
5. done

Buildbot side:
1. only one new parameter --href must be passed to gerrit review command
2. done

Result:
* on the top of the change screen the states (red/green) and the links
to the buildbot results are available
it works only if each buildbot has its own identity, i. e. to get
one line per buildbot in the verifier table.
* sane history/comment table: no mixing of human review messages and
bot notifications.
* just works for the sites that don't have CI buildbot verification
and/or don't have Verify label.


[1] https://gerrit.libreoffice.org/#/c/1164/

2013/7/18 Chad Horohoe <chor...@wikimedia.org>:

[Chad Horohoe]

On Sun, Jul 14, 2013 at 9:46 PM, Shawn Pearce <s...@google.com> wrote:

    - Diff files in two patch sets

Comparing between patchsets is pretty important, so I got to thinking

about how we could possibly handle this. I think the dropdown in the

upper right to switch between patches isn't all that clear...so I've come

up with an alternative.

Make that area a button, which flies out (like the reply dropdown) allowing

you to switch between patchsets. I've come up with a very rough sketch

to give you an idea of what I was thinking: 

https://www.mediawiki.org/wiki/File:ComparePatchSets.png

Clicking outside the box would close the flyout again. "Show all" would

expand the table to show you all the patchsets on the change. Let me

know what you think of this approach...I think it'd be easy for users

without cluttering the screen.

-Chad

[Shawn Pearce]

On Thu, Jul 18, 2013 at 3:36 PM, Chad Horohoe <chor...@wikimedia.org> wrote:
> On Sun, Jul 14, 2013 at 9:46 PM, Shawn Pearce <s...@google.com> wrote:
>>
>> - Diff files in two patch sets
>
>
> Comparing between patchsets is pretty important, so I got to thinking
> about how we could possibly handle this. I think the dropdown in the
> upper right to switch between patches isn't all that clear...so I've come
> up with an alternative.
>
> Make that area a button, which flies out (like the reply dropdown) allowing
> you to switch between patchsets. I've come up with a very rough sketch
> to give you an idea of what I was thinking:
>
> https://www.mediawiki.org/wiki/File:ComparePatchSets.png

I like this. I'm just a bit terrified that MediaWiki has never before
had a PNG named "ComparePatchSets". Clearly we are breaking new
ground. :-)

> Clicking outside the box would close the flyout again. "Show all" would
> expand the table to show you all the patchsets on the change. Let me
> know what you think of this approach...I think it'd be easy for users
> without cluttering the screen.

Right. A popup panel does this, its how ReplyBox works.

[Shawn Pearce]

On Thu, Jul 18, 2013 at 2:40 PM, David Ostrovsky
<david.o...@gmail.com> wrote:
> the red colour for "Verified+1" is well, at least misleading. If i
> would try to sell it that would be the way to go:
>
> Hey guys, i got a button for you, to mark your change as sane/green,
> but the button doesn't have a confirmation message, making your change
> immediately
> sane/green, that why the button is red
> ;-)
>
> I would suggest to convert "Verified" to grey label, and to have two
> icons "+1" (green) and "-1" (red).

Chad also pointed this out so I changed the button to be blue, like
Submit and Send (in the reply popout). This should be live on
gerrit-review.

Something you may not have noticed is the button is not always
Verified+1. The button appears if you have permission to vote the max
score in exactly one label that the rules have marked as NEED. For a
maintainer with both Verified+1 and Code-Review+2 powers the button
does not appear as both categories are still marked NEED and the
maintainer has permission to use both. If another maintainer does
Code-Review+2 the button displays as Verified+1. If a verifier does
Verified+1 the button says Code-Review+2.

> I just wanted to mention another aspect that i very like with the new
> design: simplicity.
> The main advantage for me is that we show only one patch set on the screen.

Thanks, this was a major motivation for rebuilding the screen. I
wanted to show exactly one revision at a time.

> Once we have buildbot notifications, we are going to lose the simplicity again.
> For us that means 3-5 platform specific builds per patch set and thus
> 3-5 x 2 + 1 messages in the history table,
> for start message per platform, end message per platform and combined
> result message),
> so that you get such a mess [1], mixing human and bot notifications.

Is it worth pointing out this mess exists in the old UI too? All
messages wind up in the history and its messy.

I'm not happy about adding a new href flag to the review command and
making a role account per platform build is also not always feasible,
e.g. in our our environment this would not work. I understand the
value behind this proposal, but I'm on the fence about how much the
core server should have to bend to support this.

In theory if we build the UI plugin system right a build system plugin
(e.g. Jenkins glue) could contribute a REST API to export build
results and a UI component that displays in ChangeScreen2 to show
whatever information it wants, in whatever style it finds suitable.
Then its up to you to build a plugin that surfaces the build results
the way you want them to. But there is overlap between labels and
build system results, e.g. you may want to say each platform has its
own label and all must be successful in order to submit a change. So
now there is core UI already handling the permutations and you just
want to attach a small bit of metadata (href) to each of the build
label entries. *sigh*

[Shawn Pearce]

On Sun, Jul 14, 2013 at 9:46 PM, Shawn Pearce <s...@google.com> wrote:

> My new ChangeScreen2 is mostly feature complete... so I merged it to
> master where we can continue to iterate on the concept.
>

> To use the screen modify the URL to be /#/c2/... instead of /#/c/...
> it remembers this decision for the current tab only. To get back to
> the old screen load /#/c/... again, if its already /#/c/... in the
> address bar just reload the page in the browser.

FWIW I only use ChangeScreen2 and SideBySide2 (aka CodeMirror) now.
I'm porting features that I find I need to do reviews, e.g. the file
reviewed markers, keyboard binding fixes in SideBySide2.

I realize its still far from complete as a UI, but I'm forcing myself
to use it and continue iterating until I have no remaining issues for
my own workflow. I need maybe another week or two before its done
enough to suggest making it the default on gerrit-review.

[Shawn Pearce]

On Sun, Jul 14, 2013 at 11:44 PM, Edwin Kempin <edwin....@gmail.com> wrote:
>
> Hey cool, I'm curious to try it out :)

Take a look at your ProjectConfig series now, especially the top-right corner:

https://gerrit-review.googlesource.com/#/c2/47942/3

:-)

[Shawn Pearce]

The new feature also has keyboard bindings so check '?' for details. I
maybe should also mention we had to change the 'm' key in CodeMirror
based diff view to 'r' so the toggle flag on the change screen is also
now 'r'.

[Edwin Kempin]

2013/7/24 Shawn Pearce <s...@google.com>

Thanks, this looks really cool! :-)

[Saša Živkov]

A couple of observations:

1. While key repeat for left 'h' and right 'l' navigation works, it doesn't

work for up 'k' and down 'j' keys.

2. Pressing and keeping the key 'l' will produce a strange little popup:

I am using Chrome.

[Edwin Kempin]

2013/7/20 Shawn Pearce <s...@google.com>

I have to admit that I got quite frustrated by trying to add inline comments in SideBySide2.
It seems that it's not possible anymore to add inline comments by either double clicking on a line or by clicking on the line number.
I assumed that there is a keybinding but pressing '?' always started the vi like search. Only after a while
I noticed that I can unfocus the table and then get the help popup when pressing '?' to find out that I need to press 'c'
for adding inline comments...
Will you add support to add inline comments by mouse?
 

[Shawn Pearce]

On Wed, Jul 24, 2013 at 11:09 PM, Saša Živkov <ziv...@gmail.com> wrote:
>
> A couple of observations:
>
> 1. While key repeat for left 'h' and right 'l' navigation works, it doesn't
> work for up 'k' and down 'j' keys.

Are you on Mac? This may be an Apple feature.

> 2. Pressing and keeping the key 'l' will produce a strange little popup:

Yea I think this is an Apple feature. See this link
http://lifehacker.com/5826055/make-your-keyboard-keys-repeat-properly-when-held-down-in-mac-os-x-lion

[Shawn Pearce]

On Wed, Jul 24, 2013 at 11:17 PM, Edwin Kempin <edwin....@gmail.com> wrote:
> 2013/7/20 Shawn Pearce <s...@google.com>
>>
>> On Sun, Jul 14, 2013 at 9:46 PM, Shawn Pearce <s...@google.com> wrote:
>> > My new ChangeScreen2 is mostly feature complete... so I merged it to
>> > master where we can continue to iterate on the concept.
>> >
>> > To use the screen modify the URL to be /#/c2/... instead of /#/c/...
>> > it remembers this decision for the current tab only. To get back to
>> > the old screen load /#/c/... again, if its already /#/c/... in the
>> > address bar just reload the page in the browser.
>>
>> FWIW I only use ChangeScreen2 and SideBySide2 (aka CodeMirror) now.
>> I'm porting features that I find I need to do reviews, e.g. the file
>> reviewed markers, keyboard binding fixes in SideBySide2.
>

> I have to admit that I got quite frustrated by trying to add inline comments
> in SideBySide2.
> It seems that it's not possible anymore to add inline comments by either
> double clicking on a line or by clicking on the line number.

Correct, this feature has been lost. I don't remember which it is but
either the vim keybinding mode or the CM3 editor widget itself is
stealing the click event and not making it possible for us to identify
a double click on a line. We punted for now and are relying on the 'c'
key binding.

> I assumed that there is a keybinding but pressing '?' always started the vi
> like search. Only after a while
> I noticed that I can unfocus the table and then get the help popup when
> pressing '?' to find out that I need to press 'c'
> for adding inline comments...

This is another challenge with vim mode enabled. ? is the reverse
search of / and is useful once you have / and start using it. It also
conflicts with Gerrit's help popup. Do we disable reverse search or do
we disable the help popup when focus is on a CM3 instance?

[Edwin Kempin]

On Jul 25, 2013 7:05 PM, "Shawn Pearce" <s...@google.com> wrote:
>
> On Wed, Jul 24, 2013 at 11:17 PM, Edwin Kempin <edwin....@gmail.com> wrote:
> > 2013/7/20 Shawn Pearce <s...@google.com>
> >>
> >> On Sun, Jul 14, 2013 at 9:46 PM, Shawn Pearce <s...@google.com> wrote:
> >> > My new ChangeScreen2 is mostly feature complete... so I merged it to
> >> > master where we can continue to iterate on the concept.
> >> >
> >> > To use the screen modify the URL to be /#/c2/... instead of /#/c/...
> >> > it remembers this decision for the current tab only. To get back to
> >> > the old screen load /#/c/... again, if its already /#/c/... in the
> >> > address bar just reload the page in the browser.
> >>
> >> FWIW I only use ChangeScreen2 and SideBySide2 (aka CodeMirror) now.
> >> I'm porting features that I find I need to do reviews, e.g. the file
> >> reviewed markers, keyboard binding fixes in SideBySide2.
> >
> > I have to admit that I got quite frustrated by trying to add inline comments
> > in SideBySide2.
> > It seems that it's not possible anymore to add inline comments by either
> > double clicking on a line or by clicking on the line number.
>
> Correct, this feature has been lost. I don't remember which it is but
> either the vim keybinding mode or the CM3 editor widget itself is
> stealing the click event and not making it possible for us to identify
> a double click on a line. We punted for now and are relying on the 'c'
> key binding.

It's completely fine for now. But I think it's important to have this fixed before making this the default diff screen. Otherwise we will have tons of confused users.

>
> > I assumed that there is a keybinding but pressing '?' always started the vi
> > like search. Only after a while
> > I noticed that I can unfocus the table and then get the help popup when
> > pressing '?' to find out that I need to press 'c'
> > for adding inline comments...
>
> This is another challenge with vim mode enabled. ? is the reverse
> search of / and is useful once you have / and start using it. It also
> conflicts with Gerrit's help popup. Do we disable reverse search or do
> we disable the help popup when focus is on a CM3 instance?

I like the possibility to do a reverse search by ?. Still I think that the diff screen is probably the one screen where it is most interesting to check the help for the key bindings and it should be easy to get to this help. Could we display the help popup after pressing ? for the second time?
? starts the reverse search
?? opens the help popup
It's not very intuitive but I expect that most users who expect to get the popup on ? will just try once more to open it by ?, at least this is what I did.

[Michael Zhou]

CodeMirror blocks "click" and "dblclick" when the text portion of a line is clicked on, so the old way of creating line comments by double click is kind of hard to implement.

That being said, they do have a handy "gutterClick" that gets fired whenever a line number is clicked on. I just added https://gerrit-review.googlesource.com/#/c/48171/ to make use of it.

[Michael Zhou]

https://gerrit-review.googlesource.com/#/c/48171/ This change probably implements what you want. CodeMirror blocks click and double click events if fired on the text portion of a line, so trying to get double click back isn't easy.

On Thursday, July 25, 2013 11:11:47 AM UTC-7, Edwin Kempin wrote:

[David Ostrovsky]

Am Samstag, 20. Juli 2013 21:14:11 UTC+2 schrieb Shawn Pearce:

On Thu, Jul 18, 2013 at 2:40 PM, David Ostrovsky
<david.o...@gmail.com> wrote:
> the red colour for "Verified+1" is well, at least misleading. If i
> would try to sell it that would be the way to go:
>
> Hey guys, i got a button for you, to mark your change as sane/green,
> but the button doesn't have a confirmation message, making your change
> immediately
> sane/green, that why the button is red
> ;-)
>
> I would suggest to convert "Verified" to grey label, and to have two
> icons "+1" (green) and  "-1" (red).

Chad also pointed this out so I changed the button to be blue, like
Submit and Send (in the reply popout). This should be live on
gerrit-review.

Thanks for changing the colour. It looks much better now.
 

Something you may not have noticed is the button is not always
Verified+1. The button appears if you have permission to vote the max
score in exactly one label that the rules have marked as NEED. For a
maintainer with both Verified+1 and Code-Review+2 powers the button
does not appear as both categories are still marked NEED and the
maintainer has permission to use both. If another maintainer does
Code-Review+2 the button displays as Verified+1. If a verifier does
Verified+1 the button says Code-Review+2.

That's cool, didn't know that, ... how should i? Is this documented somewhere ;-)
We should add "News and Noteworthy" section somewhere or may be even maintain
weekly Gerrit news blog?

 

> I just wanted to mention another aspect that i very like with the new
> design: simplicity.
> The main advantage for me is that we show only one patch set on the screen.

Thanks, this was a major motivation for rebuilding the screen. I
wanted to show exactly one revision at a time.

> Once we have buildbot notifications, we are going to lose the simplicity again.
> For us that means 3-5 platform specific builds per patch set and thus
> 3-5 x 2 + 1 messages in the history table,
> for start message per platform, end message per platform and combined
> result message),
> so that you get such a mess [1], mixing human and bot notifications.

Is it worth pointing out this mess exists in the old UI too? All
messages wind up in the history and its messy.

yes, sure, but we want to be better with ChangScreen2, right?
The problem with old change screen was, that the multi patch set
was shown in disclosure panel. So to show verification status you
would need to inlclude the table in that panel, because the result are
patch set specific. First with ChangeScreen2 it would be sufficient to
only have one table: switching to another patch set would reload the
 verification status table. for that patch set.

> I would love to address that problem with the new change screen. The
> best approach that I've heard so far
> was Martin's non-intrusive design suggestion:

> and/or don't have Verify label.

...

I'm not happy about adding a new href flag to the review command and
making a role account per platform build is also not always feasible,
e.g. in our our environment this would not work.

 

Thanks for pointing this out. Ironically i had the same concerns, that requiring a separate
role/identity pre job/platform is not always feasible. In fact, OpenStack build queue coordinator Zule
uses only one unique identity, reporting combined status for N different jenkins jobs,
so that wouldn't work for them either.
(Martin argued, so they (and all other?) should split it to multiple Gerrit roles/identities ;-).
 

I understand the
value behind this proposal, but I'm on the fence about how much the
core server should have to bend to support this.

I would argue pretty much ;-)
Let's face it: this is a missing core feature. Like plugin-owned capabilities,
where all plugin fans are abusing existing core capabilities, the missing
per CI-Job/Platform/Category verification status
table is clearly a missing feature, forcing all CI-fans to abuse comment/history
table.
 

In theory if we build the UI plugin system right a build system plugin
(e.g. Jenkins glue) could contribute a REST API to export build
results and a UI component that displays in ChangeScreen2 to show
whatever information it wants, in whatever style it finds suitable.
Then its up to you to build a plugin that surfaces the build results
the way you want them to. But there is overlap between labels and
build system results, e.g. you may want to say each platform has its
own label and all must be successful in order to submit a change. So
now there is core UI already handling the permutations and you just
want to attach a small bit of metadata (href) to each of the build
label entries. *sigh*

exactly, but as pointed out above, it is not, well not always the 100% overlap.
If one CI-identity is used, so then only one combined status is reported back to Gerrit,
but in the verification status table we should have multiple entries per each job:

Job/Platform/Category | Start-Time | End-Time | Status | href | 

I see the different approaches to achieve this:

1. 100% core solution:

* add SSH command/Rest API to report per Job/Platform/Category status
* persist that status in RewviewDB
* render the verification status table on changeScreen2 pre revision

2. friendly plugin/provider solution (a lá avatar provider, where core is aware of that feature)

* add VerificationStatus Extension point in core and enable plugin to implement/provide that support
* implement default plugin that would provide the SSH, Rest-API and Persistence features
* render the verification status table on changeScreen2 pre revision

3. 100% generic plugin solution

* implement all features in a plugin, say verification-status-plugin
* find a way to extend change screen2 with a HTML table,
like something similar to UiActions, i. e. UiFragments with specific places, RPC,
or even a combination with recently introduced JS API?

In all three solutions we have to somehow filter the history table from bots notification.
(plugin- or core-owned RestAPI interceptors, that seamlessly filter the comment results?)

Thoughts on this? What would be the most promising way to be integrated in Gerrit?

[Doug Kelly]

Having a few minor issues with the change screen (and maybe I'm just blind--if that's true, feel free to let me know):

On Friday, July 26, 2013 10:48:40 AM UTC-5, David Ostrovsky wrote:

Am Samstag, 20. Juli 2013 21:14:11 UTC+2 schrieb Shawn Pearce:

On Thu, Jul 18, 2013 at 2:40 PM, David Ostrovsky
<david.o...@gmail.com> wrote:
> the red colour for "Verified+1" is well, at least misleading. If i
> would try to sell it that would be the way to go:
>
> Hey guys, i got a button for you, to mark your change as sane/green,
> but the button doesn't have a confirmation message, making your change
> immediately
> sane/green, that why the button is red
> ;-)
>
> I would suggest to convert "Verified" to grey label, and to have two
> icons "+1" (green) and  "-1" (red).

Chad also pointed this out so I changed the button to be blue, like
Submit and Send (in the reply popout). This should be live on
gerrit-review. 

Thanks for changing the colour. It looks much better now.

Is it just me, or are all the icons gone now?  In fact, it seems that the screen doesn't show the reviewer grid, or have a way to add new reviewers anymore.  This especially seems like a step back from the current "user cannot vote in this category" change.

> I just wanted to mention another aspect that i very like with the new
> design: simplicity.
> The main advantage for me is that we show only one patch set on the screen.

Thanks, this was a major motivation for rebuilding the screen. I
wanted to show exactly one revision at a time.

I like the simplistic design (and one patch set per screen is fine), but is there still a way to change the reference version?  I know this is usually useful to me, especially when reviewing minor differences between two revisions.

Thanks!

Doug Kelly

[Shawn Pearce]

On Thu, Aug 8, 2013 at 10:41 AM, Doug Kelly <doug...@gmail.com> wrote:
> Having a few minor issues with the change screen (and maybe I'm just
> blind--if that's true, feel free to let me know):
>
>
> On Friday, July 26, 2013 10:48:40 AM UTC-5, David Ostrovsky wrote:
>>
>>
>>
>> Am Samstag, 20. Juli 2013 21:14:11 UTC+2 schrieb Shawn Pearce:
>>>
>>> On Thu, Jul 18, 2013 at 2:40 PM, David Ostrovsky
>>> <david.o...@gmail.com> wrote:
>>> > the red colour for "Verified+1" is well, at least misleading. If i
>>> > would try to sell it that would be the way to go:
>>> >
>>> > Hey guys, i got a button for you, to mark your change as sane/green,
>>> > but the button doesn't have a confirmation message, making your change
>>> > immediately
>>> > sane/green, that why the button is red
>>> > ;-)
>>> >
>>> > I would suggest to convert "Verified" to grey label, and to have two
>>> > icons "+1" (green) and "-1" (red).
>>>
>>> Chad also pointed this out so I changed the button to be blue, like
>>> Submit and Send (in the reply popout). This should be live on
>>> gerrit-review.
>>>
>>>
>> Thanks for changing the colour. It looks much better now.
>
> Is it just me, or are all the icons gone now?

You mean the checkmark for +2 and the red X for -2? They are not used
in ChangeScreen2, so yes they are gone.

> In fact, it seems that the
> screen doesn't show the reviewer grid,

The reviewer grid is gone. It has been replaced by the summary on the left side.

> or have a way to add new reviewers
> anymore. This especially seems like a step back from the current "user
> cannot vote in this category" change.

This is a missing feature that has not been implemented yet.

>>> > I just wanted to mention another aspect that i very like with the new
>>> > design: simplicity.
>>> > The main advantage for me is that we show only one patch set on the
>>> > screen.
>>>
>>> Thanks, this was a major motivation for rebuilding the screen. I
>>> wanted to show exactly one revision at a time.
>
> I like the simplistic design (and one patch set per screen is fine), but is
> there still a way to change the reference version? I know this is usually
> useful to me, especially when reviewing minor differences between two
> revisions.

Also a missing feature that has not been implemented yet.

[Doug Kelly]

On Thursday, August 8, 2013 12:51:27 PM UTC-5, Shawn Pearce wrote:

> Is it just me, or are all the icons gone now?

You mean the checkmark for +2 and the red X for -2? They are not used
in ChangeScreen2, so yes they are gone.

Okay, good to know.  The visual feedback by color was nice before, so I'm not sure how to leverage it in the new design, but at least I'm not crazy!

 

>  In fact, it seems that the
> screen doesn't show the reviewer grid,

The reviewer grid is gone. It has been replaced by the summary on the left side.

Yeah, I figured as much.  Actually, I like the "needs Code-Review" that the summary displays--I know 2.7 had a similar version of this, but it's much more prominent now.

 

> or have a way to add new reviewers
> anymore.  This especially seems like a step back from the current "user
> cannot vote in this category" change.

This is a missing feature that has not been implemented yet.

OK, good to know.

 

> I like the simplistic design (and one patch set per screen is fine), but is
> there still a way to change the reference version?  I know this is usually
> useful to me, especially when reviewing minor differences between two
> revisions.

Also a missing feature that has not been implemented yet.

Thanks for letting me know!  Overall, I think it'll be much nicer to use once I break myself of all the things I was used to since Gerrit 2.1. :) 

[Edwin Kempin]

Starting to use the new change screen, I noticed that the change messages don't get refreshed when
doing a rebase from the web UI. The rebase adds the 'Patch Set X was rebased' message but you
need to reload the page in order to see it.

[Edwin Kempin]

One more about working with a series of changes.
It is really great that I can see the whole series under 'Related Changes'.
But after rebasing the first change, the other changes disappear from the related changes list.
This is inconvinient since as a next step after rebase I would like to navigate to the next change in this series
and rebase it as well. On the old ChangeScreen I can actually see the next change under 'Needed By' even if
it now depends on an outdated patch set.
Can we change related changes so that also changes are displayed which have an outdated dependency
(maybe with a different style)?

2013/8/14 Edwin Kempin <edwin....@gmail.com>

--

[Deniz Türkoglu]

I am a little bit late to the game. I like the cleaner interface, some
stuff is not obvious to me, as is;

- Reviewed checkbox is a mystery, there is no clear sign of what it does
- Related to that, there is no 'diff all in single page', that feature
has been requested for a long time[1], the previous diff screen was
built with only one diff on screen, while we are moving forward, I
would like to actually make that checkbox a 'Choose files to diff', I
myself have used 'Reviewed' flag very rarely, I think we can even
remove it for the sake of simplicity, I am curious if it brings value
to others?

-deniz
[1] https://code.google.com/p/gerrit/issues/detail?id=938

[Shawn Pearce]

On Wed, Aug 14, 2013 at 12:42 AM, Edwin Kempin <edwin....@gmail.com> wrote:
> Starting to use the new change screen, I noticed that the change messages
> don't get refreshed when
> doing a rebase from the web UI. The rebase adds the 'Patch Set X was
> rebased' message but you
> need to reload the page in order to see it.

This is a bug. The refresh logic isn't working in RebaseAction. I'll
send a change shortly.

[Shawn Pearce]

Actually, I don't understand the failure. RebaseAction calls
Gerrit.display(PageLinks.toChange2(id)) which is the same as what
happens when you refresh the UI with the little reload icon.

Is the problem actually a caching issue with the /changes/{id}/detail
REST API? Do you observe this on gerrit-review?

[Shawn Pearce]

On Wed, Aug 14, 2013 at 12:52 AM, Edwin Kempin <edwin....@gmail.com> wrote:
> One more about working with a series of changes.
> It is really great that I can see the whole series under 'Related Changes'.
> But after rebasing the first change, the other changes disappear from the
> related changes list.

This is a bug. I did not intend for this to happen.

> This is inconvinient since as a next step after rebase I would like to
> navigate to the next change in this series
> and rebase it as well. On the old ChangeScreen I can actually see the next
> change under 'Needed By' even if
> it now depends on an outdated patch set.
> Can we change related changes so that also changes are displayed which have
> an outdated dependency

I agree, the behavior you desire is what I meant to happen with
Related Changes. Unfortunately my fingers typed something else into
the implementation. :-(

> (maybe with a different style)?

Also a good idea. Again, fingers did not convey the correct intent. Or
something like it.

[Shawn Pearce]

On Wed, Aug 14, 2013 at 1:04 AM, Deniz Türkoglu <de...@spotify.com> wrote:
> I am a little bit late to the game. I like the cleaner interface, some
> stuff is not obvious to me, as is;
>
> - Reviewed checkbox is a mystery, there is no clear sign of what it does

Its a per-user persistent marker on each file. Use it as you see fit.

> - Related to that, there is no 'diff all in single page', that feature
> has been requested for a long time[1], the previous diff screen was
> built with only one diff on screen, while we are moving forward, I
> would like to actually make that checkbox a 'Choose files to diff', I
> myself have used 'Reviewed' flag very rarely, I think we can even
> remove it for the sake of simplicity, I am curious if it brings value
> to others?

I use it on every change I review and depend on it heavily. I do not
want to remove this feature. It would be nice if I could because its
one of the largest and most accessed database tables.

[Deniz Türkoglu]

On Wed, Aug 14, 2013 at 7:43 PM, Shawn Pearce <s...@google.com> wrote:
> On Wed, Aug 14, 2013 at 1:04 AM, Deniz Türkoglu <de...@spotify.com> wrote:
>> I am a little bit late to the game. I like the cleaner interface, some
>> stuff is not obvious to me, as is;
>>
>> - Reviewed checkbox is a mystery, there is no clear sign of what it does
>
> Its a per-user persistent marker on each file. Use it as you see fit.

There is a checkbox, and filename next to it, there is no header to
the checkbox. There is a top header that says 'Files', which aligns
with the checkbox, this is rather confusing since the 'Reviewed'
header is now missing.

>> - Related to that, there is no 'diff all in single page', that feature
>> has been requested for a long time[1], the previous diff screen was
>> built with only one diff on screen, while we are moving forward, I
>> would like to actually make that checkbox a 'Choose files to diff', I
>> myself have used 'Reviewed' flag very rarely, I think we can even
>> remove it for the sake of simplicity, I am curious if it brings value
>> to others?

To rephrase this one, the previous diff screen was built with only one
file, when I patched it to show multiple files (i.e. commit message +
files), the keyboard navigation became the bottleneck, as there were
now more than one table and jumping from each other required a rewrite
--or at least looked like it.

When we are moving forward with this change, can we keep in mind that
this functionality is easier to add when start supporting it from the
start?

[Shawn Pearce]

I just rebased a change on gerrit-review and it did refresh with the
rebased message in the history section. Unfortunately the message was
"Patch Set 2: Patch Set 2 was rebased" as I rebased patch set 2 to
create patch set 3. This is because we have allowed gerrit-review to
lag too far behind master and are missing
8b4f28671b987ba050431f510218b211437dc209 ("Correct review message when
a change is rebased"). I'm going to try to get it updated today.

[Edwin Kempin]

2013/8/15 Shawn Pearce <s...@google.com>

On Wed, Aug 14, 2013 at 10:38 AM, Shawn Pearce <s...@google.com> wrote:
> On Wed, Aug 14, 2013 at 10:32 AM, Shawn Pearce <s...@google.com> wrote:
>> On Wed, Aug 14, 2013 at 12:42 AM, Edwin Kempin <edwin....@gmail.com> wrote:
>>> Starting to use the new change screen, I noticed that the change messages
>>> don't get refreshed when
>>> doing a rebase from the web UI. The rebase adds the 'Patch Set X was
>>> rebased' message but you
>>> need to reload the page in order to see it.
>>
>> This is a bug. The refresh logic isn't working in RebaseAction. I'll
>> send a change shortly.
>
> Actually, I don't understand the failure. RebaseAction calls
> Gerrit.display(PageLinks.toChange2(id)) which is the same as what
> happens when you refresh the UI with the little reload icon.
>
> Is the problem actually a caching issue with the /changes/{id}/detail
> REST API? Do you observe this on gerrit-review?

I just rebased a change on gerrit-review and it did refresh with the
rebased message in the history section.

I can confirm that this also works for me now

[Edwin Kempin]

Today I've had a closer at the new change screen and I've noticed some missing features and some nits.

I guess some of the points have already been discussed, but I'll include them anyway, since with this mail thread

it's hard to keep track of everything and I would like to have a "complete" list. I would then create open issues
for those points where we have an agreement that they should be addressed. Having it in the issue tracker should

make it easier to discuss, to keep track and to see what is already solved.

1. Adding new reviewers is not possible

2. It is not possible to remove reviewers including their votes.

This is important in case there is some veto voting from a technical user, e.g. a build bot.
In this case a project owner should be able to remove the vote (e.g. in case it is incorrect due to instable test environment).

3. Permalink to change can't be easily (by single click) copied to clipboard anymore

I know it's possible to get it from the reload icon by choosing 'Copy Link Address' from the context menu,
but I use this very frequently and would like to see the copy-to-clipboard-icon coming back

4. The download commands are missing.

Very important to me as I use it very frequently to fetch changes.

5. Reply doesn't allow to review inline draft comments before they are published

6. There is no possibility to go to the Unified Diff view

7. Patch Sets which have unpublished draft comments are not highlighted anymore in the revision list

8. Changes that depend on an outdated patch set do not appear in the related changes list

9. A Reference Version as base for the comparison cannot be selected anymore.

Personally I almost never use this feature.

10. The preference to reverse the order of patch sets is not honored.

I would expect that the Revisions in the Revision drop-down would be reversed if this preference is set.

11. After replying to an old patch set, the screen automatically loads the current patch set.

I expected to stay with the revision on which I just replied.

12. When changing the topic a message is added to the change, but the message is only shown

after the change screen is reloaded

13. When editing the commit message but not doing any changes to it, clicking the save button has no effect.

Either the Save button should be disabled in this case (as in the old UI) or it should close the dialog

14. The owner, author and committer are no links anymore that open the change queries for these users

15. There is no possibility to navigate to the project screen and the default project dashboard anymore,
as the project is no link anymore and the project query icon is missing

16. The branch now is no link anymore that opens the change query for this branch

17. The name of the submit strategy is not correctly formatted if the submit type comes from the Prolog submit_type rule.

E.g. applying this example [1] results in submit strategy 'cherry_pick' but it should be displayed as "Cherry Pick'

18. 'Expand Recent' for expanding the recent summary comments is missing.

Personally I don't need this feature.

19. The uploaded timestamp is not shown anymore after a change has been updated.

I find it sometimes interesting to see the age of a change, which I now can't compute anymore since the uploaded time is not shown anymore.

That's probably not so important.

20. In the header there is a convinient button to apply Code-Review+2, it would be nice to have the Submit button at the exact same place when a change gets submittable.

There is no 'publich and submit' button anymore. Having the submit button at this place would make it easy to Code-Review+2 and submit a change, as I would just need to click two times at the same position.

21. The change state for a not current patch set is shown as 'Review In Progress', would it make sense to show instead something like 'Not Current' to highlight that the user views an outdated patch set?

22. It is not possible anymore to see if a reviewer can't vote on a label

Not sure how this could be done with the new UI.

23. There is no possibility anymore to open all diff in own tabs

I never used this functionality and it's probably no big deal to remove it.

[1] https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html#_example_1_set_a_tt_cherry_pick_tt_submit_type_for_all_changes

2013/8/20 Edwin Kempin <edwin....@gmail.com>

[Shawn Pearce]

On Tue, Aug 20, 2013 at 7:49 AM, Edwin Kempin <edwin....@gmail.com> wrote:
> Today I've had a closer at the new change screen and I've noticed some
> missing features and some nits.
> I guess some of the points have already been discussed, but I'll include
> them anyway, since with this mail thread
> it's hard to keep track of everything and I would like to have a "complete"
> list. I would then create open issues
> for those points where we have an agreement that they should be addressed.
> Having it in the issue tracker should
> make it easier to discuss, to keep track and to see what is already solved.
>
> 1. Adding new reviewers is not possible

Missing feature; one reason of many ChangeScreen2 is not a default.

> 2. It is not possible to remove reviewers including their votes.
> This is important in case there is some veto voting from a technical user,
> e.g. a build bot.
> In this case a project owner should be able to remove the vote (e.g. in case
> it is incorrect due to instable test environment).

Missing feature; one reason of many ChangeScreen2 is not a default.

> 3. Permalink to change can't be easily (by single click) copied to clipboard
> anymore
> I know it's possible to get it from the reload icon by choosing 'Copy Link
> Address' from the context menu,
> but I use this very frequently and would like to see the
> copy-to-clipboard-icon coming back

I don't agree with you here. The UI has very little space in this
section so I killed the copy link icon. I'm probably going to lose
here.

> 4. The download commands are missing.
> Very important to me as I use it very frequently to fetch changes.

David Ostrovsky is starting to fix this in
https://gerrit-review.googlesource.com/48945

I have also been missing this and typing out refs/changes/ paths by
hand for weeks. Its getting old.

> 5. Reply doesn't allow to review inline draft comments before they are
> published

Certainly, no reason these can't be displayed inside of ReplyBox. I
was thinking about having them below the Send/Cancel button row and
making the content of ReplyBox scrollable so you can get to the
comments.

> 6. There is no possibility to go to the Unified Diff view

I almost never use the unified view. I know some people only use it. I
wonder if this should just be a user diff preference and not have the
stupid link on every row in the ChangeScreen2's FileTable?

> 7. Patch Sets which have unpublished draft comments are not highlighted
> anymore in the revision list

The current revision list on ChangeScreen2 is a hack and needs to be
replaced. I am not happy with it.

The draft comment issue on prior revisions: we should consider a
behavior change where all drafts from all revisions are published at
once when you send a reply. These would then all appear together in
the same email.

> 8. Changes that depend on an outdated patch set do not appear in the related
> changes list

I have already told you this is a bug, but I have not had a chance to
investigate it myself.

> 9. A Reference Version as base for the comparison cannot be selected
> anymore.
> Personally I almost never use this feature.

Missing feature. I use it when re-reviewing a change but the reviewed
boxes being auto-filled on follow-up revisions has fixed some of that
workflow issue for me. Not entirely, but some.

> 10. The preference to reverse the order of patch sets is not honored.
> I would expect that the Revisions in the Revision drop-down would be
> reversed if this preference is set.

This is a stupid preference and it should just be removed. :-)

> 11. After replying to an old patch set, the screen automatically loads the
> current patch set.
> I expected to stay with the revision on which I just replied.

So ReplyBox should update the URL when it refreshes to pin to the
revision it replied on? Simple enough change in the ReplyBox send
code.

> 12. When changing the topic a message is added to the change, but the
> message is only shown
> after the change screen is reloaded

I didn't bother with a full screen reload here. Very small change to
modify it to do that.

> 13. When editing the commit message but not doing any changes to it,
> clicking the save button has no effect.
> Either the Save button should be disabled in this case (as in the old UI) or
> it should close the dialog

The save button is disabled, the @(!(@!#*!@( CSS is just not rendering
disabled differently from enabled. CSS bug.

> 14. The owner, author and committer are no links anymore that open the
> change queries for these users

I found the number of links on the change screen... annoying. So I did
not repeat them. I would be open to making links again if we do the
CSS such that they aren't styled as links unless you hover over them.

> 15. There is no possibility to navigate to the project screen and the
> default project dashboard anymore,
> as the project is no link anymore and the project query icon is missing

Eh, maybe we should add some of these back.

> 16. The branch now is no link anymore that opens the change query for this
> branch

Same as above.

> 17. The name of the submit strategy is not correctly formatted if the submit
> type comes from the Prolog submit_type rule.
> E.g. applying this example [1] results in submit strategy 'cherry_pick' but
> it should be displayed as "Cherry Pick'

Ick. We have a glitch then somewhere in the formatting or translation code.

> 18. 'Expand Recent' for expanding the recent summary comments is missing.
> Personally I don't need this feature.

I agree. I tried hard to have the comments mostly fit collapsed.

> 19. The uploaded timestamp is not shown anymore after a change has been
> updated.
> I find it sometimes interesting to see the age of a change, which I now
> can't compute anymore since the uploaded time is not shown anymore.
> That's probably not so important.

Hmm. I think we should start writing a new ChangeMessage when the
change is first created that shows the uploaded time.

> 20. In the header there is a convinient button to apply Code-Review+2, it
> would be nice to have the Submit button at the exact same place when a
> change gets submittable.
> There is no 'publich and submit' button anymore. Having the submit button at
> this place would make it easy to Code-Review+2 and submit a change, as I
> would just need to click two times at the same position.

Two click on exactly the same pixels might be a problem, the user
might submit by mistake. But we could move submit to the header row. I
kind of prefer where it is as a short mouse drag down to the left
taking a bit more intent to submit.

> 21. The change state for a not current patch set is shown as 'Review In
> Progress', would it make sense to show instead something like 'Not Current'
> to highlight that the user views an outdated patch set?

This is a good idea.

> 22. It is not possible anymore to see if a reviewer can't vote on a label
> Not sure how this could be done with the new UI.

Yea, I don't really miss this feature. I'm sure someone somewhere
does. But I also have no idea how to represent this so its omitted for
now.

> 23. There is no possibility anymore to open all diff in own tabs
> I never used this functionality and it's probably no big deal to remove it.

It also doesn't always work. Its notably broken in Chrome right now,
with no fix in sight.

[David Pursehouse]

On 08/21/2013 10:42 AM, Shawn Pearce wrote:

>> 6. There is no possibility to go to the Unified Diff view
>
> I almost never use the unified view. I know some people only use it. I
> wonder if this should just be a user diff preference and not have the
> stupid link on every row in the ChangeScreen2's FileTable?
>

I never use it either. Having it as a preference seems like a good idea
to me.

>> 7. Patch Sets which have unpublished draft comments are not highlighted
>> anymore in the revision list
>
> The current revision list on ChangeScreen2 is a hack and needs to be
> replaced. I am not happy with it.
>
> The draft comment issue on prior revisions: we should consider a
> behavior change where all drafts from all revisions are published at
> once when you send a reply. These would then all appear together in
> the same email.
>

Not sure I agree here.

I often add draft comments on older patch sets as reminders to myself.
Having them automatically published when I comment on a newer patch set
would be annoying.

I can also see this causing confusion. The comments on older patch sets
may not be relevant any more, but if they're all included in the same
email they'd need to be clearly separated from comments on the current
patch.

>> 10. The preference to reverse the order of patch sets is not honored.
>> I would expect that the Revisions in the Revision drop-down would be
>> reversed if this preference is set.
>
> This is a stupid preference and it should just be removed. :-)
>

I never use this and can't see how it's useful, but it's perhaps worth
checking how many people do use it before removing it :)

>> 14. The owner, author and committer are no links anymore that open the
>> change queries for these users
>
> I found the number of links on the change screen... annoying. So I did
> not repeat them. I would be open to making links again if we do the
> CSS such that they aren't styled as links unless you hover over them.
>

I use these links a lot, and would like them back.

>> 15. There is no possibility to navigate to the project screen and the
>> default project dashboard anymore,
>> as the project is no link anymore and the project query icon is missing
>
> Eh, maybe we should add some of these back.
>
>> 16. The branch now is no link anymore that opens the change query for this
>> branch
>
> Same as above.
>

As above, I use these a lot and would like them back.

>> 20. In the header there is a convinient button to apply Code-Review+2, it
>> would be nice to have the Submit button at the exact same place when a
>> change gets submittable.
>> There is no 'publich and submit' button anymore. Having the submit button at
>> this place would make it easy to Code-Review+2 and submit a change, as I
>> would just need to click two times at the same position.
>
> Two click on exactly the same pixels might be a problem, the user
> might submit by mistake. But we could move submit to the header row. I
> kind of prefer where it is as a short mouse drag down to the left
> taking a bit more intent to submit.
>

In the past there has been confusion about the meaning of "publish and
submit". I am in favour of keeping the submit action separate from the
publish comments/approval action.

>> 22. It is not possible anymore to see if a reviewer can't vote on a label
>> Not sure how this could be done with the new UI.
>
> Yea, I don't really miss this feature. I'm sure someone somewhere
> does. But I also have no idea how to represent this so its omitted for
> now.
>

I find this feature useful. Before it was implemented there were a
number of cases where I was waiting for someone to verify their change
but then it turned out they didn't have permission.

[Mark Derricutt]

On 21/08/2013, at 2:05 PM, David Pursehouse <david.pu...@sonymobile.com> wrote:

10. The preference to reverse the order of patch sets is not honored.
I would expect that the Revisions in the Revision drop-down would be
reversed if this preference is set.

This is a stupid preference and it should just be removed. :-)

I never use this and can't see how it's useful, but it's perhaps worth checking how many people do use it before removing it :)

Ironically, I was quite happy when I found this configuration option appear in 2.6 ( if it was there earlier I must have missed it ) as I quite prefer seeing the most recent patch set on the top of the stack - especially when looking at a review that's at patch set 100!

Personally, from the way I see my work flow of re-reviewing patch sets going, one thing I'd really love here is if Gerrit by default, only showed the patch sets following the last one I commented on/reviewed, which a link to "show all NNN patch sets".  When looking at something with 100 revisions, not having to think about the fact that I last reviewed #97 and by default only showing me 97, 98, 99, and 100 would rock. Likewise, when changing diff views, by default filtering to that limited set of revisions would be handy.

Of course, the other solution is NOT having 100+ patch sets on a single review :)

Mark

[Edwin Kempin]

I have now created issues for most of the points [1] so that the further discussion can happen in the issue tracker.

Having the issues allows use to assign them and to mark them as solved.

I've created one issue for making ChangeScreen2 the default change screen [2] and blocked it on a few issues which I see as important.

This makes it transparent when we can switch to the new screen. We should discuss if more features are required for the switch.

In this issue I have also documented those features which have been intentionally removed so that we don't need to discuss about them again.

I've not created issues for those things which have been fixed in the meantime (thanks Shawn):
* Display revision after posting review [3]
* Refresh screen after updating topic [4]
* Honor reverse patch set order preference [5]
* Fix /submit_type to return only uppercase names [6]

One more fix is in review:
* Add CSS style to show disabled buttons as disabled [7]

[1] http://code.google.com/p/gerrit/issues/list?q=ChangeScreen2
[2] http://code.google.com/p/gerrit/issues/detail?id=2065
[3] https://gerrit-review.googlesource.com/49050
[4] https://gerrit-review.googlesource.com/49051
[5] https://gerrit-review.googlesource.com/49053
[6] https://gerrit-review.googlesource.com/49052
[7] https://gerrit-review.googlesource.com/49060

2013/8/21 Mark Derricutt <ma...@talios.com>

[Thomas Swindells (tswindel)]

>
> >> 7. Patch Sets which have unpublished draft comments are not
> >> highlighted anymore in the revision list
> >
> > The current revision list on ChangeScreen2 is a hack and needs to be
> > replaced. I am not happy with it.
> >
> > The draft comment issue on prior revisions: we should consider a
> > behavior change where all drafts from all revisions are published at
> > once when you send a reply. These would then all appear together in
> > the same email.
> >
> Not sure I agree here.
>
> I often add draft comments on older patch sets as reminders to myself.
> Having them automatically published when I comment on a newer patch set
> would be annoying.
>
> I can also see this causing confusion. The comments on older patch sets may
> not be relevant any more, but if they're all included in the same email they'd
> need to be clearly separated from comments on the current patch.

I find that people (me at leat) get confused that drafts don't get published when they expect them to.
A quick experiment shows that it isn't actually currently consistent on the existing screen.

If you have a side by side review of patch 2 against base and put a comments on both sides of the screen then when you submit your review comments both the comment against base and against patch set 2 will be published.
If you have a side by side review of patch 2 against patch 1 and put a comments on both sides of the screen then when you submit your review comments only the comments on patch2 will be submitted leaving the comments in patch 1 in draft form probably never to be published.

Thomas

[Deniz Türkoglu]

I suggest having a quick remote-hackaton for this, on 27th August Tuesday next week, for 24hours. Right now mainly Shawn works on this and more people will speed up the process and bring different use cases. For example, I also don't use the unified view, yet I recall Jonathan was asking for it in the diff-all-patch in a previous hackaton.

Since Edwin already filed the bugs, we can quickly distribute the work and get it out soon.

-deniz

[David Ostrovsky]

Am Mittwoch, 21. August 2013 10:09:41 UTC+2 schrieb Edwin Kempin:

I have now created issues for most of the points [1] so that the further discussion can happen in the issue tracker.

Having the issues allows use to assign them and to mark them as solved.

I've created one issue for making ChangeScreen2 the default change screen [2] and blocked it on a few issues which I see as important.

So with download feature almost ready, CS2 is blocked only on two issues: 

* Support adding reviewers [1]

* Support removing reviewers (including their votes) [2]

I would like to start to work on [1]. Can someone please assign it to me (if of course nobody working on it)?

[1] https://code.google.com/p/gerrit/issues/detail?id=2066

[2] https://code.google.com/p/gerrit/issues/detail?id=2067

[Shawn Pearce]

Done. I also gave you permission to edit issues in the issue tracker
so you can do this yourself in the future. I should have done that a
while ago but ... I forget to do these sorts of things. :-)

[David Ostrovsky]

With adding reviewers change ready for review [1], i wonder what would be the best way to implement

removing the reviewers? For adding the reviewers it was sufficiently to add a popdown panel with

SuggestOracle to the "CC" field, because only here the reviewers appear that didn't vote yet.

For removing the reviewers is not obviously where to put a popdown, if at all?

We should be able to remove the reviewers with votes - they appear in both "Reviewers" field and

"Code-Review" and "Verify" fields below, and without votes - the appears only in "CC" field. 

I am thinking to combine adding and removing feature in one popdown panel, and put it right to 

"CC" field. In this combined control we could have SuggestOracle (like it is the case now) and below

have the list of current reviewers with votes and without, that can be deleted.

But then we can delete reviewers with votes on "CC" field, that are not appear on this field. They

should be removed from "Reviewers" and "Code-Review", "Verify" and other custom labels below.

Other ideas?

[1] https://gerrit-review.googlesource.com/49142

[Edwin Kempin]

2013/8/28 David Ostrovsky <david.o...@gmail.com>

Just some thoughts about this.

I believe the main use-case for this feature is to remove a blocking vote of a (technical) user to make the change submittable.

Removing a reviewer including all its votes was the only possibility to do this on the old change screen.

Maybe what we really want is to offer the remove action on a vote and not on the reviewer?

Ok, another use case may be that I accidentally added the wrong reviewer and I want to remove him.

If we offer the remove action on the reviewer I think it's important that at this moment you can clearly see which votes are removed by removing this reviewer.

 

[1] https://gerrit-review.googlesource.com/49142

[Thomas Swindells (tswindel)]

 

With adding reviewers change ready for review [1], i wonder what would be the best way to implement

removing the reviewers? For adding the reviewers it was sufficiently to add a popdown panel with

SuggestOracle to the "CC" field, because only here the reviewers appear that didn't vote yet.

 

For removing the reviewers is not obviously where to put a popdown, if at all?

We should be able to remove the reviewers with votes - they appear in both "Reviewers" field and

"Code-Review" and "Verify" fields below, and without votes - the appears only in "CC" field. 

 

I am thinking to combine adding and removing feature in one popdown panel, and put it right to 

"CC" field. In this combined control we could have SuggestOracle (like it is the case now) and below

have the list of current reviewers with votes and without, that can be deleted.

 

But then we can delete reviewers with votes on "CC" field, that are not appear on this field. They

should be removed from "Reviewers" and "Code-Review", "Verify" and other custom labels below.

 

Other ideas?

Just some thoughts about this.

I believe the main use-case for this feature is to remove a blocking vote of a (technical) user to make the change submittable.

Removing a reviewer including all its votes was the only possibility to do this on the old change screen.

Maybe what we really want is to offer the remove action on a vote and not on the reviewer?

Ok, another use case may be that I accidentally added the wrong reviewer and I want to remove him.

If we offer the remove action on the reviewer I think it's important that at this moment you can clearly see which votes are removed by removing this reviewer.

[Thomas Swindells] I think you are right, there are two main use cases. One is to remove a reviewer who doesn’t care about the change – including removing yourself. This should possibly only be allowed if they don’t have any blocking votes.

 

The other current reason is to ignore a vote where the original voter is unable or unwilling to change it themselves (they are a machine, on holiday or just won’t back down). My preference here is that we preserve knowledge that the vote was cast and have a way of specifying why the vote is being ignored (“dodgy build machine”, “overruled by tech lead”, “they are on holiday and can’t re-review the latest patchset”). This provides better auditing, discourages abuse, and psychologically better for the person being overruled as their opinion is at least still being recorded and available for them to point back at later if it all goes wrong.

 

Presumably the third use case of removing your own vote can still be achieved by just voting again.

 

Thomas

[Edwin Kempin]

2013/8/28 Thomas Swindells (tswindel) <tswi...@cisco.com>

+1 removing / overruling of votes should be recorded and it should be possible to specify a reason when doing so

This is a long wanted missing feature.

 

 

Presumably the third use case of removing your own vote can still be achieved by just voting again.

 

Thomas

--

[David Pursehouse]

On 07/15/2013 01:46 PM, Shawn Pearce wrote:
>
> Missing features:
[...]
> - Edit commit message

Does this need to be implemented like it was in the old Change Screen,
i.e. with a button and popup from the commit message box on the main
change screen?

Or is it planned to implement it using the 'inline edit' feature?

[Steffen Gebert]

Why not linking the edit link to the codemirror screen for editing the commit message?

Steffen

[David Ostrovsky]

Am Dienstag, 10. September 2013 07:30:04 UTC+2 schrieb David Pursehouse:

On 07/15/2013 01:46 PM, Shawn Pearce wrote:
>
> Missing features:
[...]
>      - Edit commit message

Does this need to be implemented like it was in the old Change Screen,
i.e. with a button and popup from the commit message box on the main
change screen? 

The "Edit commit message" popdown is already implemented. Click on "Edit message" button

in change header or press "e".

[David Pursehouse]

Aha. Just me being blind, then :)

Thanks.

BTW are you still working on the inline series?

[David Ostrovsky]

Yes, i do.

I have to rewrite the UI part from the scratch (against CS2), though.

And Dave's ground work changes are still pending [1].

[1] https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit+branch:master+topic:inline,n,z

[Loki]

Hey good idea, to create a new change screen. I'm a little confused where all the parts are gone in ChangeScreen2, so I've created an own scratch by coping parts of the old and new one with gimp (so don't care about the contents :-)). It follows my reading workflow form left to right:

First I want to read "What are the change goals?" then "Details: contributors, project, branch, code diff, ..." or another analogy "A task card, where the contributors tokes are pined to"

On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:

My new ChangeScreen2 is mostly feature complete... so I merged it to
master where we can continue to iterate on the concept.

To use the screen modify the URL to be /#/c2/... instead of /#/c/...
it remembers this decision for the current tab only. To get back to
the old screen load /#/c/... again, if its already /#/c/... in the
address bar just reload the page in the browser.

Missing features:
    - Add reviewer(s)
    - Remove reviewer(s)
    - Edit commit message
    - Diff files in two patch sets
    - Dependencies and dependents
    - Download links by URL and action (cherry-pick, checkout, etc.)

Obviously these must be fixed to move forward with ChangeScreen2.

I am going to try to deploy tip of master to gerrit-review tomorrow. I
will reply to this thread when it is live and its easier for users to
try ChangeScreen2 in a real side-by-side comparison with the existing
ChangeScreen.

[Edwin Kempin]

2013/11/3 Loki <stoc...@gmail.com>

Hey good idea, to create a new change screen. I'm a little confused where all the parts are gone in ChangeScreen2, so I've created an own scratch by coping parts of the old and new one with gimp (so don't care about the contents :-)). It follows my reading workflow form left to right:

First I want to read "What are the change goals?" then "Details: contributors, project, branch, code diff, ..." or another analogy "A task card, where the contributors tokes are pined to"

Nice, on a first look this seems indeed easier to read :)

 

On Monday, July 15, 2013 6:46:44 AM UTC+2, Shawn Pearce wrote:

My new ChangeScreen2 is mostly feature complete... so I merged it to
master where we can continue to iterate on the concept.

To use the screen modify the URL to be /#/c2/... instead of /#/c/...
it remembers this decision for the current tab only. To get back to
the old screen load /#/c/... again, if its already /#/c/... in the
address bar just reload the page in the browser.

Missing features:
    - Add reviewer(s)
    - Remove reviewer(s)
    - Edit commit message
    - Diff files in two patch sets
    - Dependencies and dependents
    - Download links by URL and action (cherry-pick, checkout, etc.)

Obviously these must be fixed to move forward with ChangeScreen2.

I am going to try to deploy tip of master to gerrit-review tomorrow. I
will reply to this thread when it is live and its easier for users to
try ChangeScreen2 in a real side-by-side comparison with the existing
ChangeScreen.

--

[Shawn Pearce]

On Mon, Nov 4, 2013 at 4:09 AM, Edwin Kempin <edwin....@gmail.com> wrote:

2013/11/3 Loki <stoc...@gmail.com>

Hey good idea, to create a new change screen. I'm a little confused where all the parts are gone in ChangeScreen2, so I've created an own scratch by coping parts of the old and new one with gimp (so don't care about the contents :-)). It follows my reading workflow form left to right:

First I want to read "What are the change goals?" then "Details: contributors, project, branch, code diff, ..." or another analogy "A task card, where the contributors tokes are pined to"

Nice, on a first look this seems indeed easier to read :)

I also thought this was a good idea, so I tried to implement it in code:

  http://i.imgur.com/PysuuRZ.png

  https://gerrit-review.googlesource.com/51821

[David Ostrovsky]

So we are showing[1] for VRFY:

Verified

-1 Diffy Cuckoo

But all in black for CR:

Code-Review

-1 Shawn Pearce +1 Michael Zhou

Any reasons why not to have colors for CR too, at least on the numbers: green for values > 0

and red for value < 0, as one would expect (and as it was on old change screen)?

[1] https://gerrit-review.googlesource.com/#/c/50922

[Edwin Kempin]

2013/11/21 David Ostrovsky <david.o...@gmail.com>

So we are showing[1] for VRFY:

Verified

-1 Diffy Cuckoo

But all in black for CR:

Code-Review

-1 Shawn Pearce +1 Michael Zhou

Any reasons why not to have colors for CR too, at least on the numbers: green for values > 0

and red for value < 0, as one would expect (and as it was on old change screen)?

As I understand it, we use red for veto votes and green for approvals.

For the Code-Review label -2 is the veto and +2 approves the change, which is why -1 and +1 are not colored here.

 

[1] https://gerrit-review.googlesource.com/#/c/50922

[David Ostrovsky]

On Thursday, November 21, 2013 3:53:57 PM UTC+1, Edwin Kempin wrote:

2013/11/21 David Ostrovsky <david.o...@gmail.com>

So we are showing[1] for VRFY:

Verified

-1 Diffy Cuckoo

But all in black for CR:

Code-Review

-1 Shawn Pearce +1 Michael Zhou

Any reasons why not to have colors for CR too, at least on the numbers: green for values > 0

and red for value < 0, as one would expect (and as it was on old change screen)?

As I understand it, we use red for veto votes and green for approvals.

For the Code-Review label -2 is the veto and +2 approves the change, which is why -1 and +1 are not colored here.

But couldn't we use red "x" icon for CR-2 and green "V" icon for CR+2 (like it was the case on the old change screen)?

And then we could have green "+1" and red "-1" too...

I think it wouldn't hurt to have more colors/icons on the new change screen.

[Edwin Kempin]

2013/11/21 David Ostrovsky <david.o...@gmail.com>

+1
 

[Shawn Pearce]

I was trying to avoid the x and checkmark icon here, they aren't the best artwork we have and they were hard to find to begin with. And I didn't want to put too much coloring into this table. The intermediate scores are interesting but not going to move a change forward (or stop it), while the prolog rules returning reject() or ok() does. So we color that.

Of course we use red/green which isn't great for color blind people. So maybe the icons are a good idea too. But again I hate that particular set of icons that we have. 

[Will DeBerry]

Just grabbed the latest gerrit and am definitely like the reorg for sure. One thing I did notice is a new set of tabs in the right corner for related and conflicts. Getting the following error for the conflicts section and wasn't sure if something needed to be adjusted server side or if just incomplete.

400 secondary index must be enabled for conflicts:

[David Ostrovsky]

On Friday, November 22, 2013 2:08:45 PM UTC+1, Will DeBerry wrote:

Just grabbed the latest gerrit and am definitely like the reorg for sure. One thing I did notice is a new set of tabs in the right corner for related and conflicts. Getting the following error for the conflicts section and wasn't sure if something needed to be adjusted server side or if just incomplete.

400 secondary index must be enabled for conflicts:

For "conflict:"query and other new and shiny secondary index query-operators to work you have to activate Lucene index on your site and run reindex once on your data.

Check Section index in config[1].

[1] https://gerrit-review.googlesource.com/Documentation/config-gerrit.html

[David Ostrovsky]

And here the reindex link[1].

https://gerrit-review.googlesource.com/Documentation/pgm-reindex.html 

[Edwin Kempin]

2013/11/22 Will DeBerry <willd...@gmail.com>

Just grabbed the latest gerrit and am definitely like the reorg for sure. One thing I did notice is a new set of tabs in the right corner for related and conflicts. Getting the following error for the conflicts section and wasn't sure if something needed to be adjusted server side or if just incomplete.

400 secondary index must be enabled for conflicts:

The tab with the conflicting changes should not be displayed if secondary indexes are not enabled.
I've pushed a fix for this:
  https://gerrit-review.googlesource.com/51997

 

On Thursday, November 21, 2013 11:40:28 AM UTC-5, Shawn Pearce wrote:

On Thu, Nov 21, 2013 at 7:19 AM, David Ostrovsky <david.o...@gmail.com> wrote:

On Thursday, November 21, 2013 3:53:57 PM UTC+1, Edwin Kempin wrote:

2013/11/21 David Ostrovsky <david.o...@gmail.com>

So we are showing[1] for VRFY:

Verified

-1 Diffy Cuckoo

But all in black for CR:

Code-Review

-1 Shawn Pearce +1 Michael Zhou

Any reasons why not to have colors for CR too, at least on the numbers: green for values > 0

and red for value < 0, as one would expect (and as it was on old change screen)?

As I understand it, we use red for veto votes and green for approvals.

For the Code-Review label -2 is the veto and +2 approves the change, which is why -1 and +1 are not colored here.

But couldn't we use red "x" icon for CR-2 and green "V" icon for CR+2 (like it was the case on the old change screen)?

And then we could have green "+1" and red "-1" too...

I think it wouldn't hurt to have more colors/icons on the new change screen.

I was trying to avoid the x and checkmark icon here, they aren't the best artwork we have and they were hard to find to begin with. And I didn't want to put too much coloring into this table. The intermediate scores are interesting but not going to move a change forward (or stop it), while the prolog rules returning reject() or ok() does. So we color that.

Of course we use red/green which isn't great for color blind people. So maybe the icons are a good idea too. But again I hate that particular set of icons that we have. 

[David Ostrovsky]

On Friday, November 22, 2013 3:14:43 PM UTC+1, Edwin Kempin wrote:

2013/11/22 Will DeBerry <willd...@gmail.com>

Just grabbed the latest gerrit and am definitely like the reorg for sure. One thing I did notice is a new set of tabs in the right corner for related and conflicts. Getting the following error for the conflicts section and wasn't sure if something needed to be adjusted server side or if just incomplete.

400 secondary index must be enabled for conflicts:

The tab with the conflicting changes should not be displayed if secondary indexes are not enabled.
I've pushed a fix for this:
  https://gerrit-review.googlesource.com/51997

Thanks.

So i tried to document recent changes in intro-change-screen[1] and stumbled over (extended) related change tab control.

After thinking about it, i think it would be more clear to rename the original "Related Changes" tab to "Dependencies"

and put "Related Changes" above the tab control on the header section on the third column.

Here is the design suggestion: [2].

I also explained in this comment [3] why i think we should do that.

[1] https://gerrit-review.googlesource.com/#/c/51975

[2] http://i.imgur.com/yW3Cua6.png

[3] https://gerrit-review.googlesource.com/#/c/51996/1//COMMIT_MSG,cm

[David Ostrovsky]

Am Freitag, 22. November 2013 15:14:43 UTC+1 schrieb Edwin Kempin:

2013/11/22 Will DeBerry <willd...@gmail.com>

Just grabbed the latest gerrit and am definitely like the reorg for sure. One thing I did notice is a new set of tabs in the right corner for related and conflicts. Getting the following error for the conflicts section and wasn't sure if something needed to be adjusted server side or if just incomplete.

400 secondary index must be enabled for conflicts:

The tab with the conflicting changes should not be displayed if secondary indexes are not enabled.
I've pushed a fix for this:
  https://gerrit-review.googlesource.com/51997

 

Can it be that we have another problem with related changes?

At least on this change[1] same topic tab curently shows [1] on gerrit-review:

400 Unsupported query:Owner

 

[1] https://gerrit-review.googlesource.com/48254

[Edwin Kempin]

Yes, but the fix is already pushed
  https://gerrit-review.googlesource.com/51977

>
> 400 Unsupported query:Owner
>  
> [1] https://gerrit-review.googlesource.com/48254
>