Community
Participate
Working Groups
I like how the comments show up as annotations on the compare editor, but the problem is that you can't address the comments there, you have to open up the file in a separate editor to address the comment, and then you end up switching back and forth between source and compare editors. I would be nice to show those comment annotations in the source editors as well as the compare editors. It would also be nice if there were an automated way to move to the next comment in the source code. There would need to be some command to load the annotations from a particular patch set.
Great suggestion. We have some support in the code for attaching annotation models to workspace editors but it's not currently enabled (requires a bunch more work). We could use the active task (review) to determine which annotations to show. One of the challenges is to only show annotations if the workspace revision matches the review revision. To simplify the process we could have a fetch action that checks out the right branch and activates the task with a pre-populated context that matches the latest patch set.
That's an interesting idea. I'm not sure that it always makes sense to activate the review rather than its "parent" task, but maybe with a pre-populated context it would. Would you display comments only from the latest patch set? I guess I would also want to be able to add and respond to comments from the workspace editors.
(In reply to comment #2) > That's an interesting idea. I'm not sure that it always makes sense to activate > the review rather than its "parent" task, but maybe with a pre-populated context > it would. We'll have to discuss how to integrate that better. I have seen projects moving away from Bugzilla and using Gerrit to collaborate on changes instead. > Would you display comments only from the latest patch set? We could probably include support for "activating" older patch sets as well but I think it's a rare use case. > I guess I would also want to be able to add and respond to comments from the > workspace editors. Yes, we would want to support that.
Sebastien, we should look at idea about actually loading the patch set as a potential addition or even solution to the issues we've been discussing in bug 400266. If we could make that work, it would certinaly simplify all of the issues w/ discovering what is in the workspace. One issue of course is time taken to load the patch set. And of course if there are existing un committed changes, it's going to fail. But even to have the option to do this could be really nice.
Take note that a lot of the functionality requested here i.e. opening files in source editor, navigating comments in editors, altering comment from workspace editors etc. already exists in R4E and should really be ported to Mylyn Review. The effort done for bug 400266 partially includes this functionality. Additional functionality is enabled in the Inline Annotation framework that is currently hosted and used in R4E, but really should be moved and adapted in Mylyn Reviews (which was the original intent anyways). From my experience in working with integrating workspace files (see bug 400266), there are a few issues to resolve for a feature like that to work completely What I tried to do there is to open the workspace file in the compare editor if the revision of the file in the review matches the one under review coming from the Gerrit server. One problem happens when the workspace file is changed (either because the user modifies it, or the workspace changes e.g. by checking out a new branch. In this case, the compare editor input for the "target" file revision changes to the new contents of the workspace file, and is thus not reflecting the file under review anymore. That could be ok if a new separate command is added to open the file in workspace rather than the file under review, which could be the same. As in R4E we can also add a decoration to the File artifact in the Gerrit Task editor to show the user whether the file under review is synchronized with the file in workspace. Another problem is that if the file under review is not synchronized with the workspace, or if we want to show anomalies from previous patch sets, then the position of the annotations could be off and mislead the user. In this case, I think it is probable desirable to open those file as read-only to avoid any potential confusion So, in short IMO we should. - Integrate R4E inline commenting framework to mylyn reviews and adapt the codebase to use this framework to gain some feature described here. - Add decorators in Gerrit Tasks editor to show user that file under review and file in workspace are synchronized or not - Add new command (and correspoding UI) to open workspace file in a compare editor (compare it with base file in review) or in source editor with review annotations. In the source editor, the file will be taken from the workspace and modifiable if in sync, or taken from the file history and read-only if not. We should leverage on the already existing R4E code in this effort As this is really an extension of the funxtionality I am working on to integrate workspace (bug 400266), I'll put this under it
Right now the solution for bug 400266 uses the ReviewBehavior object to pass in the parent repository needed to resolve the file revision from the team repository and the resolution is done when the file is opened in the compare editor. Ideally, we should add a reference to the repository in the ReviewItemSet model element and a team IFileRevision reference in the Mylyn Review IFileRevision. This would enable use to decorate the IFileItem icon in the GErrit Task editor and Review Navigator at all times. However, to make that work, we need a way to detect that the workspace file has changed to redo the resolution when the user changes his workspace (e.g. when checking out another git branch). This should be investigated and done as part of the effort for this current, which is the evolution of the solution provided in bug 400266). We should also change the Mylyn Review IFileRevision element name to IFileVersion to avoid confusion with the team API element (see bug 403393)
(In reply to comment #6) > Right now the solution for bug 400266 uses the ReviewBehavior object to pass > in the parent repository needed to resolve the file revision from the team > repository and the resolution is done when the file is opened in the compare > editor. Ideally, we should add a reference to the repository in the > ReviewItemSet model element and a team IFileRevision reference in the Mylyn > Review IFileRevision. This would enable use to decorate the IFileItem icon > in the GErrit Task editor and Review Navigator at all times. However, to > make that work, we need a way to detect that the workspace file has changed > to redo the resolution when the user changes his workspace (e.g. when > checking out another git branch). This should be investigated and done as > part of the effort for this current, which is the evolution of the solution > provided in bug 400266). +1 on all of this. I'll look at it after we get the basic persistence and cross UI notification support working.
I've pushed a very, very preliminary WIP to https://git.eclipse.org/r/#/c/38603/. This is nowhere close to working and may be completely the wrong way to do things. It's just the minimal thing you can do to get something to show up on the source files. Hopefully it provides some help in getting started.
Created attachment 249563 [details] mylyn/context/zip
(In reply to Sam Davis from comment #9) > Created attachment 249563 [details] > mylyn/context/zip Cool, going to take a look right now!
https://git.eclipse.org/r/#/c/38862/ (Some refactorings to support generic concept of active review which we'll want for this.)
