save multiple comments at once

[Jan Heylen]

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".
>
>> 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.

I would only use the 'preview' logic to define the set of comments
that will be submitted when the user pushes the "all comments"
button,or would you even post all "open" forms? The fact that you can
also see the rst/md markup is out of the scope of this discussion.

>
>> 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.

Indeed, that is the first thing that should be added. Even without any
change on the comment system that would be an improvement for not
loosing comments...

>
>
>> 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?

Yes, as said, it's only a POC, and I needed to stop the recursing
somehow indeed.

>
> I would guess you could use something like
> $('code-difftable form').each(function(){$(this).submit();});
> or perhaps even
> $('code-difftable form').submit();
>
If I understand this code snippet correctly, that would submit all
"form" under the code-difftable?

> 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.
In my POC, I submit all the forms in separate api calls using
javascript indeed, so I don't see why that is not a step towards a
real solution. This step in the solution would allow us, by only
changing a bit in the frontend javascript code to see if users would
like this feature of submitting all comments in one step instead of
one comment at a time. The way it is implemented in communicating
towards the backend can be changed if this would be a good step
(change into one form, or something else,...) And if it isn't well
received, we can easily change it back?
Or is this not what you wanted to say, and did I misunderstood?

>
> /Mads

Jan