save multiple comments at once
Wouter Vermeiren
wouter.vermeiren at gmail.com
Fri Mar 27 05:37:24 EDT 2015
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 <heyleke at gmail.com> wrote:
> On Fri, Mar 27, 2015 at 10:02 AM, Wouter Vermeiren
> <wouter.vermeiren at gmail.com> 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 <heyleke at gmail.com> 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 <heyleke at gmail.com> wrote:
> >> >
> >> > On Mon, Jan 26, 2015 at 7:35 PM, Mads Kiilerich <mads at kiilerich.com>
> >> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20150327/3c1a20c4/attachment-0001.html>
More information about the kallithea-general
mailing list