Bug 344108 - [editor] support new and replying to global review comments
Summary: [editor] support new and replying to global review comments
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: unspecified   Edit
Hardware: PC Linux
: P3 enhancement (vote)
Target Milestone: 2.0   Edit
Assignee: Miles Parker CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed, noteworthy
: 389931 (view as bug list)
Depends on:
Blocks: 390367
  Show dependency tree
 
Reported: 2011-04-28 06:56 EDT by Benjamin Muskalla CLA
Modified: 2012-12-19 18:45 EST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Muskalla CLA 2011-04-28 06:56:05 EDT
The reply button on review comments doesn't work. I think this should open the post comments dialog and prefill with comment with a quoted version of the comment in question.
Comment 1 Steffen Pingel CLA 2011-05-07 20:38:59 EDT
Replying is not yet supported but would be a very useful addition.
Comment 2 Miles Parker CLA 2012-09-20 20:02:33 EDT
*** Bug 389931 has been marked as a duplicate of this bug. ***
Comment 3 Miles Parker CLA 2012-12-06 17:38:42 EST
Correct me if I'm wrong, but it appears to be the case that the only way global commenting is supported now is through publish comment. I'd like to go ahead and add a New Comment box to the task editor as well, unless anyone objects..
Comment 4 Miles Parker CLA 2012-12-06 17:45:56 EST
Hmm... this begs the question of whether we want a global "Review" or "Submit" action (and Actions section?) that would trigger publishing of the comments dialog (and any draft comments..?) If we don't support this, the Reply to Functionality doesn't really make sense either, as we don't have any where to put the replied comment or to trigger it's being submitted. (We could just take the value and put it in the publish comments dialog, which is what I'll do for a first pass, but that workflow is pretty weird / nonintuitive.
Comment 5 Miles Parker CLA 2012-12-06 19:35:31 EST
https://git.eclipse.org/r/#/c/9089/
Comment 6 Steffen Pingel CLA 2012-12-07 10:47:37 EST
This is directly related to bug 370645. Gerrit would need to integrate with the standard editor submit functionality and provide a comment attribute as proposed in the review. At least the publish action would need to be triggered through the editor to support this (which may be a reasonable effort overall).
Comment 7 Miles Parker CLA 2012-12-07 15:26:28 EST
Right, on submit button, I was thinking we could just provide the PublishCommentDialog. As a variation, we could assume that the comment was what they wanted, and provide a simple confirm dialog. I'd like to do away with the confirm altogether, but I gather that isn't easy to do under the current design..we would also miss the "opportunity" to inform user of number of drafts, but I don't think that's neccessary and isn't provided w. Gerrit Web Ui in any case. Thoughts? Worth doing for this bug or should we hold on it?
Comment 8 Miles Parker CLA 2012-12-07 16:12:04 EST
(edited summary to reflect widening of scope and distinguish between this and replying to comments in artifact editors.)
Comment 9 Steffen Pingel CLA 2012-12-13 05:48:21 EST
We should remove the confirmation dialog. Currently it supports voting which would need to be integrated with the editor first. It's also possible to publish comments for older patch sets which is something we should support as well. I would propose to polish the current review, merge it and track all further improvements under bug 370645.
Comment 10 Miles Parker CLA 2012-12-13 13:22:42 EST
(In reply to comment #9)
> We should remove the confirmation dialog.

I assume you mean "eventually" not "now" as part of https://git.eclipse.org/r/#/c/9089/? (We can't do so now as as you point out below that would leave the user no way to vote.)

> Currently it supports voting which
> would need to be integrated with the editor first. It's also possible to
> publish comments for older patch sets which is something we should support
> as well. I would propose to polish the current review, merge it and track
> all further improvements under bug 370645.

+1 So unless I misunderstood I'm going to address the tow other things you noticed on the review and we can hopefully leave it at that for now.
Comment 11 Steffen Pingel CLA 2012-12-19 18:45:16 EST
I have merged the latest patch set into master. Thanks very much for the contribution, Miles!
Comment 12 Steffen Pingel CLA 2012-12-19 18:45:29 EST
Marking resolved.