<div dir="ltr">Jan,<br><br>Then this proposal is really different from what you presented during a meeting yesterday? I look forward to the review :-)<div><br></div><div>Regards,</div><div>Wouter</div></div><br><div class="gmail_quote">On Fri, 27 Mar 2015 at 10:10 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">On Fri, Mar 27, 2015 at 10:02 AM, Wouter Vermeiren<br>
<<a href="mailto:wouter.vermeiren@gmail.com" target="_blank">wouter.vermeiren@gmail.com</a>> wrote:<br>
> Hi,<br>
><br>
> I was wondering whether I might make a slightly different proposal for<br>
> reducing the amount of emails/notifications...<br>
><br>
> My proposal revolves around making it possible to 'edit' a comment. By<br>
> default when you are looking at a pull request and commenting on it, every<br>
> comment is saved in the database as 'draft' (like Jan's proposal). And only<br>
> when you either mark a commit as approved/rejected/... at the bottom of the<br>
> page, then the comments on this commits are converted from draft into actual<br>
> comments. The same goes for comments on the pull request. So only when you<br>
> update your status on the review, the comments are saved. I am assuming here<br>
> that people use this general status to clearly indicate there progress while<br>
> looking at the review (otherwise why do we have these nice colorful<br>
> circles).<br>
> This process can repeat itself. Suppose you have added 4 comments and you<br>
> need to go home. Then you select the 'under review' status and the 4<br>
> comments are saved. If the author then looks at the pull request he/she can<br>
> see that you are not done. The next day you add 2 additional comments and<br>
> then you reject the review. Each comment cycle is followed by a status<br>
> update thus triggering a conversion from a draft comment into an actual<br>
> comment.<br>
[jan] This is as I am implementing it :-)<br>
<br>
><br>
> - In the GUI, I would not display a comment as long as it has the draft<br>
> status. This prevents the author replying to a comment that you later want<br>
> to change/remove.<br>
[jan] This is as I am implementing it :-)<br>
<br>
however, for the moment, I don't require any edit functionality to<br>
have this workflow, so what follows is another ;-) useful discussion<br>
on the edit functionality:<br>
<br>
><br>
> - I realise that editing a comment could spark a holy war here. However my<br>
> opinion would be that this is ok as long as there is some tracability.<br>
> Therefore a comment that was edited, should always contain something like<br>
> 'Edited by <user> on <datetime>'. And this small line could be a link to the<br>
> original comment.<br>
><br>
> - Editing can happen at 2 stages in the process: a) while a comment is still<br>
> in draft status and b) while it is visible to everybody. In case of a) this<br>
> 'edited by' line does not need to be there because it wasn't visible and<br>
> thus no harm is done. In case of b) then this 'edited by' line should<br>
> definitely be there because somebody might have replied to the comment.<br>
><br>
> - GUI-wise, this means adding an edit-icon somewhere or a button next to the<br>
> delete button.<br>
><br>
> - 1 email/notification is send when the reviewer updates his status (so at<br>
> the time the comments are converted from draft into actual comments). This<br>
> will greatly reduce the amount of emails/notifications without loosing any<br>
> relevant information.<br>
><br>
> - I would also make this behaviour configurable by the admin on a per<br>
> kallithea basis by adding a checkbox in the "Admin > Settings > Emails" and<br>
> not an account specific feature.<br>
><br>
> The reason why I am a bit hesitant about accepting the proposal from Jan, is<br>
> because I don't like different buttons doing the same thing only slightly<br>
> different. For me buttons like <comment><submit all comments> next to each<br>
> other, is stepping back to the web1.0 world. They both mean the same thing<br>
<br>
[jan] There is no buttons comment anymore in the latest proposal, again:<br>
per inline comment form: save - cancel (and once saved a edit/delete)<br>
Below the general comment field you'll have the "submit all comments"<br>
<br>
> from a user perspective and without knowing the background it is confusing<br>
> which button to use. Nowadays people are used/prefer correcting a mistake<br>
> they made rather then having a tool policing them into reviewing every<br>
> comment beforehand. This kind of invasive policing reduces the mental speed<br>
> with which somebody is doing a review. Or at least he/she perceives he/she<br>
> is doing the review.<br>
> Also this extra step that is required, is not currently in a typical review<br>
> flow. You comment on code and then you mark the pull request as<br>
> approved/rejected/... At no point do I need to take into account that my<br>
> comments are only saved client side and that if my browser crash, I'd loose<br>
> all comments. That just seems bad design circa 2015...<br>
[jan] with my changes, all drafts are saved server side, as you<br>
describe here, 100% exactly the same<br>
<br>
><br>
> As always, I would be happy to create a patch. I am on holiday next week,<br>
> but I will find some time to work on it. And anyway it seems like a small<br>
> patch ;-)<br>
[jan] not that small, wait for the review ;-)<br>
<br>
><br>
> Kind Regards,<br>
> Wouter Vermeiren<br>
><br>
><br>
> On Thu, 26 Mar 2015 at 18:41 Jan Heylen <<a href="mailto:heyleke@gmail.com" target="_blank">heyleke@gmail.com</a>> wrote:<br>
>><br>
>> 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>><br>
>> > 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<br>
>> > >> 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<br>
>> > >> 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<br>
>> > >> preview<br>
>> > >> first (you are still reviewing the rest of the commit), you have to<br>
>> > >> 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 -<br>
>> > >> 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<br>
>> > > button<br>
>> > > next to the inline comment. But I guess the text on the button should<br>
>> > > 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<br>
>> > >> (no<br>
>> > >> backend change required) at once.<br>
>> > ><br>
>> > ><br>
>> > > I don't see the point in rst/md markup of comments. People should<br>
>> > > 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<br>
>> > > processing. We<br>
>> > > use<br>
>> > ><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<br>
>> > > 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<br>
>> > > 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<br>
>> > >> 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<br>
>> > >> 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 ...<br>
>> > > 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<br>
>> > > call.<br>
>> > > That means you either have to do it with javascript or change the<br>
>> > > whole diff<br>
>> > > to be one form. This PoC might thus not be step towards a real<br>
>> > > solution.<br>
>> > ><br>
>> > > /Mads<br>
</blockquote></div>