Community
Participate
Working Groups
Right now, when a file is opened in the compare editor from the Gerrit Tasks editor (or Review Navigator) it is always open as an external file, losing many benefits of integrating with Eclipse (such as navigation provided by JDT and CDT for instance). On the other hand, R4E supports opening files from the workspace if they are synchronized with the Review contents. This bug tracks the porting of the functionality from R4E to the Mylyn connector, so that both tools benefit from the workspace integration.
+1. This is gong to be a killer feature!
Navigation works on one side but not the other. It's clear technically why this is the case, but it makes the workflow feel odd. Of course, it would be useful to be able to invoke the navigation directly, and naively a user is going to wonder why it doesn't work. After all the class/method/etc.. is available in the workspace, why can't we still find it? This does work in git compare, why can't we make it work here? The confusion would be resolved a bit if we had some sort of strong visual indication that the file was also the one currently available in the workspace. See also bug 371428 comment 4.
As I said before, complete resolution wrt/ AST will not work when using the team API IFileREvision as input. However, we should be as good as the Git Compare is, so I'll need to have another look As for the functionality describe in bug 371428, this is really an extension of the work done here (a phase 2 if you will). Most of the functionality described there could be added by moving/refactoring code from R4E to Mylyn Reviews and the rest involves creating a new command to use the workspace file. See bug 371428 for more info.
(In reply to comment #3) > As I said before, complete resolution wrt/ AST will not work when using the team > API IFileREvision as input. Just to walk through the logic for the bug, can you go through a bit more of what the issue is here? > As for the functionality describe in bug 371428, this is really an extension of > the work done here (a phase 2 if you will). Most of the functionality described > there could be added by moving/refactoring code from R4E to Mylyn Reviews and > the rest involves creating a new command to use the workspace file. See bug > 371428 for more info. Makes sense to me.
(In reply to comment #4) > (In reply to comment #3) > > As I said before, complete resolution wrt/ AST will not work when using the > team > > API IFileREvision as input. > > Just to walk through the logic for the bug, can you go through a bit more of > what the issue is here? Actually this was due to a bug in my implementation. We now behave exactly like Egit.
Created attachment 227562 [details] Bug in compare I'm getting the following bug when I try to open some (but not all) files, or when I try to do a compare w/ base. There isn't a cooresponding error in error log.
(In reply to comment #6) > There isn't a cooresponding error in error log. Oh my! I would start a debug session with a Java exception breakpoint to track it down. Good luck.
Copying Steffen's comment from review "passing the repository along through the call chain isn't optimal. I would suggest that we discuss on the bug how to resolve the design issues." Sebastien and I talked through these issues and and we're pretty convinced this is neccesary now but only temporarily. The basic issues is that we don't have enough information to determine the repository *with the current design*. This, like almost everything on the project comes down to how we're going to change the airplane engines in mid-flight: In order to implement the repository discovery correctly, we first have to get the Remote working as well as improved EMF model support. (I'm hitting this in a tangenital way as well right now as part of implementing a more general replacement for the review section buttons.) Once we have that complete, we'll be able to replace the pass-through.
For reference, the review is located here: https://git.eclipse.org/r/#/c/10488/5 > Sebastien and I talked through these issues and and we're pretty convinced this > is neccesary now but only temporarily. Where can I find that discussion? I understand that we need access to the specific repository but I don't agree that it has to be exposed in the API or framework. Creation of the compare editor input could be abstracted through an interface for instance or simpler could be exposed in ReviewBehavior. A review specific instance is already passed along the call chain and could provide the editor input or an adaptable. Implementing JGit specific bits in the Gerrit connector may not be the final design we settle on but it's much better than exposing team provider specific classes in the framework component. The latter violates an important design constraint and impacts reuse of the framework. That's the type of stuff where we shouldn't compromise but rather provide more extensibility where needed.
(In reply to comment #9) > For reference, the review is located here: > https://git.eclipse.org/r/#/c/10488/5 > > > Sebastien and I talked through these issues and and we're pretty convinced this > > is neccesary now but only temporarily. > > Where can I find that discussion? > > I understand that we need access to the specific repository but I don't > agree that it has to be exposed in the API or framework. Creation of the > compare editor input could be abstracted through an interface for instance > or simpler could be exposed in ReviewBehavior. A review specific instance is > already passed along the call chain and could provide the editor input or an > adaptable. > > Implementing JGit specific bits in the Gerrit connector may not be the final > design we settle on but it's much better than exposing team provider > specific classes in the framework component. The latter violates an > important design constraint and impacts reuse of the framework. That's the > type of stuff where we shouldn't compromise but rather provide more > extensibility where needed. We discussed it but I'm afraid the discussion was not logged. In any case here are my thought on your comments 1) About passing down the repository: I agree that is not the ideal solution to pass reference to the repository around, but like you said when the compare editor gets open, we need to know if we can resolve the review contents being open to the contents available locally (in File history) so that we can fetch and use the team API IFileRevision to gain full AST resolution and navigability within the editor. Another way I tried was to add a reference to the team repository (or better yet to the team API IFileRevision) in the Mylyn Review IFileRevison element of the Review model. This reference gets populated when the Review is open in the task editor and is used later on when a compare editor is opened on this FileItem. This implies modifying the Review Model and has the drawback of having to resolve the team API IFileRevision again when the compare editor is opened because the reference might have changed in the meantime (e.g. if the user checks out another branch in his workspace). But it can be done and its avoid this passing around of the repo 2) For having the JGit references in the Reviews framework, I see what you mean and I agree with you. I need to find a way to abstract the resolving of the file details from the reviews framework. So e.g. I need to create an interface in the review framework that will have to be implemented in the Gerrit Connector to resolve the team API IFileRevision in JGit. That will keep the Reviews framework SCM-independent. I will change the code to do this. However, the dependency on the team API in the reviews framework is inevitable, but I guess that could be okay. The functionality I implemented is quite nice though and significantly enhances the use of Eclipse functionalities with the Gerrit Connector/Mylyn review so IMO there is a strong case in merging that functionality in. Ideally we should shoot for the full workspace integration, as described in bug 371428, but that can come later. Most of the functionality described in bug 371428 already exists in R4E so it's a matter of adapting/porting the code we have there. One annoying thing I noticed with my solution as it is now though is that there is no visual indication to the user that the files in the compare editor are reolved to the IFileRevison or not. So from the user's point of view, AST integration and navigability functionalities in the compare editor are not visible: Sometimes it works, and sometimes it doesn't. I don't like taht. Would you have an idea on how we can give a visual cue to the user that we are "in sync" with the workspace repo? I was thinking maybe to change the compare editor left/right side label to prepend/append some text, or to put a icon as this cue. Finally the Team API uses its own IFileRevison, as is the Mylyn Reviews framework. It gets very confusing to distinguish the two. The was we got around it in R4E is to name ours IFileVersion instead of IFileRevision to make the distinction clear. Should we change it in the Review framework as well? Let me know your thoughts, and if the solutions I mention above would be acceptable.
(In reply to comment #9) > Implementing JGit specific bits in the Gerrit connector may not be the final > design we settle on but it's much better than exposing team provider specific > classes in the framework component. The latter violates an important design > constraint and impacts reuse of the framework. That's the type of stuff where we > shouldn't compromise but rather provide more extensibility where needed. Agreed. As a meta-level comment, a Catch-22 here is that we need to have reasonably granular changes for review / smaller grained commits, but in order to do that, we have to make sub-optimal designs in order to merge in temporary solutions. Then I think we end up needing to discuss a lot of issues that won't end up being operative in the final design. So as I mentioned above and Sebastien points out in more detail, we can't do things the right way until we get the other changes in and those are dependent on stuff that I'm working on and also need to get in, that is, I have to pull out the remote stuff (bug 400356) and *then* implement a common store for the reviews. That also involves turning a lot of the editor specific bits into general UI abstractions, etc.. and handling the significant issue of Review Navigator integration. It's not even clear to me how to split that all up. So just to point out that this is a thorny issue not just for design but development methodology! We should keep this issue in mind and as discussed previously think about how we develop/evolve practices to account for it, and be willing to get some stuff in there that we know will need to be re-visited. That's riskier in some ways, but less costly than perfecting things at each step. I do think we should draw a hard-line against introducing dependencies that shouldn't be in there, but I think we should be willing to accept sub-optimal API. We should mark that, and I think this is one case where TODO or some other kind of annotation makes sense.
(In reply to comment #10) > One annoying thing I noticed with my solution as it is now though is that there > is no visual indication to the user that the files in the compare editor are > reolved to the IFileRevison or not. So from the user's point of view, AST > integration and navigability functionalities in the compare editor are not > visible: Sometimes it works, and sometimes it doesn't. I don't like taht. > Would you have an idea on how we can give a visual cue to the user that we are > "in sync" with the workspace repo? I was thinking maybe to change the compare > editor left/right side label to prepend/append some text, or to put a icon as > this cue. Yes, I think an icon just for the case where we don't have the AST integration. Perhaps the "external link" icon. > Finally the Team API uses its own IFileRevison, as is the Mylyn Reviews > framework. It gets very confusing to distinguish the two. The was we got > around it in R4E is to name ours IFileVersion instead of IFileRevision to make > the distinction clear. Should we change it in the Review framework as well? +1. I've actually never liked FileRevision as the base (original) version of the file is not by definition a revision. That's been confusing for me.
(In reply to comment #10) > 2) For having the JGit references in the Reviews framework, I see what you mean > and I agree with you. I need to find a way to abstract the resolving of the > file details from the reviews framework. So e.g. I need to create an interface > in the review framework that will have to be implemented in the Gerrit Connector > to resolve the team API IFileRevision in JGit. Take a look at ReviewBehavior, it could be sufficient to simply add a method there for now. > That will keep the Reviews > framework SCM-independent. I will change the code to do this. However, the > dependency on the team API in the reviews framework is inevitable, but I guess > that could be okay. Yes, I don't think there is any point in splitting that out at this point. Someone could make a case for reusing the annotations support without coupling to team but it's not something that I would worry about later if at all. > The functionality I implemented is quite nice though and significantly enhances > the use of Eclipse functionalities with the Gerrit Connector/Mylyn review so IMO > there is a strong case in merging that functionality in. No disagreement at all. The change just has to fit with the current architecture but I don't think we are far off at all. > Finally the Team API uses its own IFileRevison, as is the Mylyn Reviews > framework. It gets very confusing to distinguish the two. The was we got > around it in R4E is to name ours IFileVersion instead of IFileRevision to make > the distinction clear. Should we change it in the Review framework as well? That sounds good to me.
Looks the change was merged. Great to see that going in! Anything left to do here? It would be great to add a snippet to the New & Noteworthy with a screenshot.
The functionality is now integrated. For now this is complete. More functionality & a description of the evolution of the solution can be found in bug 371428, which will be implemented later to complete the work done here. What we have now is a way to use the file under review from the file repository history if it is available there, which gives us all the nice features Eclipse provide for workspace files (code navigability, tooltips etc.). As for screenshots, what we can show e.g. a compare editor with a tooltip that shows declaration information for e.g. a Java element. If you want I can do this.
This is great! Would it be possible to support navigation across files in a review? E.g. if I add a method in one file and a call to the method in another file, I'd like to be able to navigate from the call to the added method in the other file.
(In reply to comment #16) > This is great! Would it be possible to support navigation across files in a > review? E.g. if I add a method in one file and a call to the method in > another file, I'd like to be able to navigate from the call to the added > method in the other file. This should already be possible, as we now resolve the file using the team API. However you need to have the repository and project in your workspace, and the file need to have been included in at least one commit so that it's part or the git history. The missing part is to resolve the workspace file itself. This will be ported from R4E next.
(In reply to comment #17) > (In reply to comment #16) > > This is great! Would it be possible to support navigation across files in a > > review? E.g. if I add a method in one file and a call to the method in > > another file, I'd like to be able to navigate from the call to the added > > method in the other file. > > This should already be possible, as we now resolve the file using the team API. > However you need to have the repository and project in your workspace, and the > file need to have been included in at least one commit so that it's part or the > git history. The missing part is to resolve the workspace file itself. This > will be ported from R4E next. It doesn't work as I described. I have files A and B in my workspace and Git history, and I have a review where a method was added to file A and a call to that method was added to file B. At the call site in file B, if I ctrl+click on the class name A, it opens up the workspace version of file A. If I ctrl+click on the method call, nothing happens. I would like it to open up a compare editor showing the added method in class A.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > This is great! Would it be possible to support navigation across files in a > > > review? E.g. if I add a method in one file and a call to the method in > > > another file, I'd like to be able to navigate from the call to the added > > > method in the other file. > > > > This should already be possible, as we now resolve the file using the team API. > > However you need to have the repository and project in your workspace, and the > > file need to have been included in at least one commit so that it's part or the > > git history. The missing part is to resolve the workspace file itself. This > > will be ported from R4E next. > > It doesn't work as I described. I have files A and B in my workspace and Git > history, and I have a review where a method was added to file A and a call > to that method was added to file B. At the call site in file B, if I > ctrl+click on the class name A, it opens up the workspace version of file A. > If I ctrl+click on the method call, nothing happens. I would like it to open > up a compare editor showing the added method in class A. OK I see what you mean. I think this will only work when we can resolve the file directly in the workspace (not through the team API). Like I said, this functionality should be ported next from R4E.
Ok, cool.
