On Fri, Mar 27, 2015 at 10:02 AM, Wouter Vermeiren <[email protected]> wrote: > Hi, > > I was wondering whether I might make a slightly different proposal for > reducing the amount of emails/notifications... > > 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). > 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. [jan] This is as I am implementing it :-)
> > - 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. [jan] This is as I am implementing it :-) however, for the moment, I don't require any edit functionality to have this workflow, so what follows is another ;-) useful discussion on the edit functionality: > > - 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. > > - 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. > > - GUI-wise, this means adding an edit-icon somewhere or a button next to the > delete button. > > - 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. > > - 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. > > 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 [jan] There is no buttons comment anymore in the latest proposal, again: per inline comment form: save - cancel (and once saved a edit/delete) Below the general comment field you'll have the "submit all comments" > 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. > 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... [jan] with my changes, all drafts are saved server side, as you describe here, 100% exactly the same > > 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 ;-) [jan] not that small, wait for the review ;-) > > Kind Regards, > Wouter Vermeiren > > > On Thu, 26 Mar 2015 at 18:41 Jan Heylen <[email protected]> wrote: >> >> Hi Mads, >> >> any comments on the issue discussed below? >> >> I'll restart the thread: >> >> I've been working on this some more and as I see it now: >> >> For inline comments you'll have 2 buttons: save - cancel >> Below the general comment field you'll have the "submit all comments" >> and the "cancel all" button >> >> So the save button would store the comment as draft (on the server), >> and the "submit all comments" will change all those drafts into >> comments and send out the notification. >> >> I would leave out the 'direct' comment button from the inline-comments >> as I think we should encourage a workflow where users review at least >> whole the changeset before committing comments, instead of submitting >> each comment separately. Even better would be to be able to do this >> for the whole pull request at once (save draft comments on all >> changesets, and then submit all comments at once). >> >> br, >> >> Jan >> >> On Tue, Mar 17, 2015 at 8:36 PM, Jan Heylen <[email protected]> wrote: >> > >> > On Mon, Jan 26, 2015 at 7:35 PM, Mads Kiilerich <[email protected]> >> > wrote: >> > > On 01/26/2015 06:37 PM, Jan Heylen wrote: >> > >> >> > >> Hi, >> > >> >> > >> I've been working on a change that a user would allow to open >> > >> multiple >> > >> comments on a commit, and 'post' (save) them all at once to the >> > >> kallithea database. Currently this is client side only change, but >> > >> with the proper changes on the server side end, this could result in >> > >> a >> > >> one-mail-per-commit-review instead of a one-mail-per-comment setup >> > >> now. >> > >> >> > >> But the main reason I was working on this is the fact that if you add >> > >> 2 or more comments to a commit, and you choose to save them as >> > >> preview >> > >> first (you are still reviewing the rest of the commit), you have to >> > >> go >> > >> over all the comments and press the save button. >> > >> >> > >> Where I want to take this is even one step further: >> > >> >> > >> We have now: >> > >> For inline comments: save - cancel - preview >> > >> And below the commit: approved/rejected/close/... and comment - >> > >> preview >> > >> >> > >> I would change this to: >> > >> For inline comments: comment - cancel >> > >> Below the commit: submit all comments - cancel all >> > > >> > > >> > > I think it could be annoying if we don't have a comment/save/submit >> > > button >> > > next to the inline comment. But I guess the text on the button should >> > > change >> > > to something with "all comments". >> > > >> > >> > I've been working on this some more and as I see it now (almost the >> > same as above, but a bit more explanation): >> > >> > For inline comments you'll have 2 buttons: save - cancel >> > Below the general comment field you'll have the "submit all comments" >> > and the "cancel all" button >> > >> > So the save button would store the comment as draft (on the server), >> > and the "submit all comments" will change all those drafts into >> > comments and send out the notification. >> > >> > I would leave out the 'direct' comment button from the inline-comments >> > as I think we should encourage a workflow where users review at least >> > whole the changeset before committing comments, instead of submitting >> > each comment separately. Even better would be to be able to do this >> > for the whole pull request at once (save draft comments on all >> > changesets, and then submit all comments at once). >> > >> > br, >> > >> > Jan >> > >> > >> > >> The inline 'comment' buttons would actually do what is now a preview >> > >> (and may still indicate comment preview), cancel remains the same. >> > >> The 'submit all comments' would do an AJAXPost of all the comments >> > >> (no >> > >> backend change required) at once. >> > > >> > > >> > > I don't see the point in rst/md markup of comments. People should >> > > comment on >> > > content, not spend time making it look fancy. We only saw people being >> > > annoyed by their code snippets being obfuscated by the markup >> > > processing. We >> > > use >> > > >> > > https://bitbucket.org/Unity-Technologies/kallithea/commits/09286e5ca064de6930d5bdefb9df6708eda19976 >> > > and do thus not really have any need for preview. We could perhaps >> > > move in >> > > that direction here upstream and avoid the problem you mention. >> > > >> > >> What do you think? >> > > >> > > >> > > +1 to the general idea and your thoughts. >> > > >> > > Also, we should introduce a "don't navigate away from this page with >> > > pending >> > > comments" before we encourage users to not commit immediately. >> > > >> > > >> > >> I've made a little POC patch that does the submit-all-open-comments >> > >> thing, it does it for open comments for know, but probably not that >> > >> difficult (and even better, it won't require the remove-class hack >> > >> probably) to change that to a for all preview comments way of >> > >> working. >> > >> I'll inline the POC patch for your reference, but forgive me my basic >> > >> javascript knowledge... >> > >> >> > >> regards, >> > >> >> > >> Jan >> > >> >> > >> the patch: >> > >> >> > >> diff -r bfc304687f1c kallithea/public/js/base.js >> > >> --- a/kallithea/public/js/base.js Wed Jan 21 17:35:11 2015 +0100 >> > >> +++ b/kallithea/public/js/base.js Mon Jan 26 18:36:00 2015 +0100 >> > >> @@ -667,6 +667,9 @@ >> > >> $form.submit(function(e){ >> > >> e.preventDefault(); >> > >> >> > >> + if(!$tr.hasClass('form-open')){ >> > >> + return; >> > >> + } >> > >> if(lineno === undefined){ >> > >> alert('Error submitting, line ' + lineno + ' not >> > >> found.'); >> > >> return; >> > >> @@ -694,6 +697,10 @@ >> > >> 'line': lineno >> > >> }; >> > >> ajaxPOST(submit_url, postData, success); >> > >> + //proof of concept, preemptive remove the form from the open >> > >> list, not good, but only for POC >> > >> + $tr.removeClass('form-open'); >> > >> + var allOpenForms = $( '.form-open' ).next(); >> > >> + allOpenForms.find( ".inline-form" ).submit(); >> > >> }); >> > >> >> > >> $('#preview-btn_'+lineno).click(function(e){ >> > > >> > > >> > > It is not obvious to me what is going on with this form-open. Or ... >> > > you are >> > > recursing and using this to make sure it terminates? >> > > >> > > I would guess you could use something like >> > > $('code-difftable form').each(function(){$(this).submit();}); >> > > or perhaps even >> > > $('code-difftable form').submit(); >> > > >> > > But in the next step you will submit multiple comments in the same api >> > > call. >> > > That means you either have to do it with javascript or change the >> > > whole diff >> > > to be one form. This PoC might thus not be step towards a real >> > > solution. >> > > >> > > /Mads _______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
