Jan, Then this proposal is really different from what you presented during a meeting yesterday? I look forward to the review :-)
Regards, Wouter On Fri, 27 Mar 2015 at 10:10 Jan Heylen <[email protected]> wrote: > 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
