<div dir="ltr">Hi,<br><br><div>I was wondering whether I might make a slightly different proposal for reducing the amount of emails/notifications...</div><div><br></div><div>My proposal revolves around making it possible to 'edit' a comment. By default when you are looking at a pull request and commenting on it, every comment is saved in the database as 'draft' (like Jan's proposal). And only when you either mark a commit as approved/rejected/... at the bottom of the page, then the comments on this commits are converted from draft into actual comments. The same goes for comments on the pull request. So only when you update your status on the review, the comments are saved. I am assuming here that people use this general status to clearly indicate there progress while looking at the review (otherwise why do we have these nice colorful circles).</div><div>This process can repeat itself. Suppose you have added 4 comments and you need to go home. Then you select the 'under review' status and the 4 comments are saved. If the author then looks at the pull request he/she can see that you are not done. The next day you add 2 additional comments and then you reject the review. Each comment cycle is followed by a status update thus triggering a conversion from a draft comment into an actual comment.</div><div><br></div><div>- In the GUI, I would not display a comment as long as it has the draft status. This prevents the author replying to a comment that you later want to change/remove.</div><div><br></div><div>- I realise that editing a comment could spark a holy war here. However my opinion would be that this is ok as long as there is some tracability. Therefore a comment that was edited, should always contain something like 'Edited by <user> on <datetime>'. And this small line could be a link to the original comment.</div><div><br></div><div>- Editing can happen at 2 stages in the process: a) while a comment is still in draft status and b) while it is visible to everybody. In case of a) this 'edited by' line does not need to be there because it wasn't visible and thus no harm is done. In case of b) then this 'edited by' line should definitely be there because somebody might have replied to the comment.</div><div><br></div><div>- GUI-wise, this means adding an edit-icon somewhere or a button next to the delete button.</div><div><br></div><div>- 1 email/notification is send when the reviewer updates his status (so at the time the comments are converted from draft into actual comments). This will greatly reduce the amount of emails/notifications without loosing any relevant information.</div><div><br></div><div>- I would also make this behaviour configurable by the admin on a per kallithea basis by adding a checkbox in the "Admin > Settings > Emails" and not an account specific feature.</div><div><br></div><div>The reason why I am a bit hesitant about accepting the proposal from Jan, is because I don't like different buttons doing the same thing only slightly different. For me buttons like <comment><submit all comments> next to each other, is stepping back to the web1.0 world. They both mean the same thing from a user perspective and without knowing the background it is confusing which button to use. Nowadays people are used/prefer correcting a mistake they made rather then having a tool policing them into reviewing every comment beforehand. This kind of invasive policing reduces the mental speed with which somebody is doing a review. Or at least he/she perceives he/she is doing the review.</div><div>Also this extra step that is required, is not currently in a typical review flow. You comment on code and then you mark the pull request as approved/rejected/... At no point do I need to take into account that my comments are only saved client side and that if my browser crash, I'd loose all comments. That just seems bad design circa 2015...</div><div><br></div><div><span style="line-height:1.5;font-size:13.1999998092651px">As always, I would be happy to create a patch. I am on holiday next week, but I will find some time to work on it. And anyway it seems like a small patch ;-)</span><br></div><div><br></div><div>Kind Regards,</div><div>Wouter Vermeiren</div><div><br></div></div><br><div class="gmail_quote">On Thu, 26 Mar 2015 at 18:41 Jan Heylen <<a href="mailto:heyleke@gmail.com">heyleke@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Mads,<br>
<br>
any comments on the issue discussed below?<br>
<br>
I'll restart the thread:<br>
<br>
I've been working on this some more and as I see it now:<br>
<br>
For inline comments you'll have 2 buttons: save - cancel<br>
Below the general comment field you'll have the "submit all comments"<br>
and the "cancel all" button<br>
<br>
So the save button would store the comment as draft (on the server),<br>
and the "submit all comments" will change all those drafts into<br>
comments and send out the notification.<br>
<br>
I would leave out the 'direct' comment button from the inline-comments<br>
as I think we should encourage a workflow where users review at least<br>
whole the changeset before committing comments, instead of submitting<br>
each comment separately. Even better would be to be able to do this<br>
for the whole pull request at once (save draft comments on all<br>
changesets, and then submit all comments at once).<br>
<br>
br,<br>
<br>
Jan<br>
<br>
On Tue, Mar 17, 2015 at 8:36 PM, Jan Heylen <<a href="mailto:heyleke@gmail.com" target="_blank">heyleke@gmail.com</a>> wrote:<br>
><br>
> On Mon, Jan 26, 2015 at 7:35 PM, Mads Kiilerich <<a href="mailto:mads@kiilerich.com" target="_blank">mads@kiilerich.com</a>> wrote:<br>
> > On 01/26/2015 06:37 PM, Jan Heylen wrote:<br>
> >><br>
> >> Hi,<br>
> >><br>
> >> I've been working on a change that a user would allow to open multiple<br>
> >> comments on a commit, and 'post' (save) them all at once to the<br>
> >> kallithea database. Currently this is client side only change, but<br>
> >> with the proper changes on the server side end, this could result in a<br>
> >> one-mail-per-commit-review instead of a one-mail-per-comment setup<br>
> >> now.<br>
> >><br>
> >> But the main reason I was working on this is the fact that if you add<br>
> >> 2 or more comments to a commit, and you choose to save them as preview<br>
> >> first (you are still reviewing the rest of the commit), you have to go<br>
> >> over all the comments and press the save button.<br>
> >><br>
> >> Where I want to take this is even one step further:<br>
> >><br>
> >> We have now:<br>
> >>    For inline comments:  save - cancel - preview<br>
> >>    And below the commit: approved/rejected/close/... and comment - preview<br>
> >><br>
> >> I would change this to:<br>
> >>    For inline comments: comment - cancel<br>
> >>    Below the commit: submit all comments - cancel all<br>
> ><br>
> ><br>
> > I think it could be annoying if we don't have a comment/save/submit button<br>
> > next to the inline comment. But I guess the text on the button should change<br>
> > to something with "all comments".<br>
> ><br>
><br>
> I've been working on this some more and as I see it now (almost the<br>
> same as above, but a bit more explanation):<br>
><br>
> For inline comments you'll have 2 buttons: save - cancel<br>
> Below the general comment field you'll have the "submit all comments"<br>
> and the "cancel all" button<br>
><br>
> So the save button would store the comment as draft (on the server),<br>
> and the "submit all comments" will change all those drafts into<br>
> comments and send out the notification.<br>
><br>
> I would leave out the 'direct' comment button from the inline-comments<br>
> as I think we should encourage a workflow where users review at least<br>
> whole the changeset before committing comments, instead of submitting<br>
> each comment separately. Even better would be to be able to do this<br>
> for the whole pull request at once (save draft comments on all<br>
> changesets, and then submit all comments at once).<br>
><br>
> br,<br>
><br>
> Jan<br>
><br>
><br>
> >> The inline 'comment' buttons would actually do what is now a preview<br>
> >> (and may still indicate comment preview), cancel remains the same.<br>
> >> The 'submit all comments' would do an AJAXPost of all the comments (no<br>
> >> backend change required) at once.<br>
> ><br>
> ><br>
> > I don't see the point in rst/md markup of comments. People should comment on<br>
> > content, not spend time making it look fancy. We only saw people being<br>
> > annoyed by their code snippets being obfuscated by the markup processing. We<br>
> > use<br>
> > <a href="https://bitbucket.org/Unity-Technologies/kallithea/commits/09286e5ca064de6930d5bdefb9df6708eda19976" target="_blank">https://bitbucket.org/Unity-<u></u>Technologies/kallithea/<u></u>commits/<u></u>09286e5ca064de6930d5bdefb9df67<u></u>08eda19976</a><br>
> > and do thus not really have any need for preview. We could perhaps move in<br>
> > that direction here upstream and avoid the problem you mention.<br>
> ><br>
> >> What do you think?<br>
> ><br>
> ><br>
> > +1 to the general idea and your thoughts.<br>
> ><br>
> > Also, we should introduce a "don't navigate away from this page with pending<br>
> > comments" before we encourage users to not commit immediately.<br>
> ><br>
> ><br>
> >> I've made a little POC patch that does the submit-all-open-comments<br>
> >> thing, it does it for open comments for know, but probably not that<br>
> >> difficult (and even better, it won't require the remove-class hack<br>
> >> probably) to change that to a for all preview comments way of working.<br>
> >> I'll inline the POC patch for your reference, but forgive me my basic<br>
> >> javascript knowledge...<br>
> >><br>
> >> regards,<br>
> >><br>
> >> Jan<br>
> >><br>
> >> the patch:<br>
> >><br>
> >> diff -r bfc304687f1c kallithea/public/js/base.js<br>
> >> --- a/kallithea/public/js/base.js Wed Jan 21 17:35:11 2015 +0100<br>
> >> +++ b/kallithea/public/js/base.js Mon Jan 26 18:36:00 2015 +0100<br>
> >> @@ -667,6 +667,9 @@<br>
> >>       $form.submit(function(e){<br>
> >>           e.preventDefault();<br>
> >><br>
> >> +        if(!$tr.hasClass('form-open'))<u></u>{<br>
> >> +          return;<br>
> >> +        }<br>
> >>           if(lineno === undefined){<br>
> >>               alert('Error submitting, line ' + lineno + ' not found.');<br>
> >>               return;<br>
> >> @@ -694,6 +697,10 @@<br>
> >>                   'line': lineno<br>
> >>           };<br>
> >>           ajaxPOST(submit_url, postData, success);<br>
> >> +        //proof of concept, preemptive remove the form from the open<br>
> >> list, not good, but only for POC<br>
> >> +        $tr.removeClass('form-open');<br>
> >> +        var allOpenForms = $( '.form-open' ).next();<br>
> >> +        allOpenForms.find( ".inline-form" ).submit();<br>
> >>       });<br>
> >><br>
> >>       $('#preview-btn_'+lineno).<u></u>click(function(e){<br>
> ><br>
> ><br>
> > It is not obvious to me what is going on with this form-open. Or ... you are<br>
> > recursing and using this to make sure it terminates?<br>
> ><br>
> > I would guess you could use something like<br>
> > $('code-difftable form').each(function(){$(this)<u></u>.submit();});<br>
> > or perhaps even<br>
> > $('code-difftable form').submit();<br>
> ><br>
> > But in the next step you will submit multiple comments in the same api call.<br>
> > That means you either have to do it with javascript or change the whole diff<br>
> > to be one form. This PoC might thus not be step towards a real solution.<br>
> ><br>
> > /Mads<br>
</blockquote></div>