Community
Participate
Working Groups
Created attachment 178059 [details] Initial model from Alvaro and the R4E team
The model looks good, however as we are going to provide some components in our framework project we have to make some adaptions. Currently in the review editor with inline commenting(for source files) is in the scope, so this editor has to interpret IFileContext and its deltas. At the moment the Delta has a reference to any Object, which is not really a good contract. It would be better that the Delta should at least contain the attributes position, original and patched contents. Roughly the same thing is with the Location class, it should contain a reference to the FileContext and the position and I'm not quite sure, why we need a reference from the deltas to the locations. I'm not quite sure yet, how to handle non-source files like emf models. For the task based reviews an extension of the emf location would work, but that's not the case for other review. Since basically all information is present in Item, I'm not sure if we need the Changeset,Patch and Resource classes. Regarding the reviews of a Resource(full file), is there only one Delta per FileContext or are there several Deltas (one per line/previous changesets)?
Created attachment 178167 [details] data model diagram updated to address comments from Kilian.
See the embedded R4E team comments below: 1) At the moment the Delta has a reference to any Object, which is not really a good contract. Object has been used in few places to allow the user application to determine the variant, depending of the content type e.g. Models, Code, Diagrams, etc.. 2) It would be better that the Delta should at least contain the attributes position, original and patched contents. Point taken, see updated attachment (Sept. 3), The Location in the previous diagram was missing a reference to FileVersion in order to make it a true Location entity, which in addition includes a new attribute position. The position remains an Object to allow for extensions e.g. for a source file it will be a line range, for a UML Model a element id, etc... the Delta has been updated with two references to Content (base and target), and content itself includes a reference to Location 3) Roughly the same thing is with the Location class, it should contain a reference to the FileContext and the position Point taken, the update above is capturing this item with the slight modification that instead of referencing FileContext, Location refers to a FileVersion since the location is applicable to one file. Position was added as an attribute in the Location class. 4) I'm not quite sure, why we need a reference from the deltas to the locations. The new diagram hopefully makes it clearer, the delta is associated to two Contents (base and target) and each content must reference a Location (which includes position) 5) I'm not quite sure yet, how to handle non-source files like emf models. For the task based reviews an extension of the emf location would work, but that's not the case for other review. That's why we left the Content and position as Objects to allow for specialisations e.g. diagrams, models, etc.. 6) Since basically all information is present in Item, I'm not sure if we need the Changeset,Patch and Resource classes. Point taken, we have removed these items and added a note instead. 7) Regarding the reviews of a Resource(full file), is there only one Delta per FileContext or are there several Deltas (one per line/previous changesets)? You are right, this was a typo, Item may contain multiple Deltas for Patch and ChangeSet and for a Resource one Delta would be the normal case but representing a multiplicity of 0..* covers all. /Sebastien, Jacques, Alvaro
Created attachment 178662 [details] Ecore diagram update
Created attachment 179326 [details] data model
An updated model included, The file itself contains the description of the changes.
Created attachment 180201 [details] updated data model
Created attachment 180994 [details] reduced model
Thanks for iterating on this, Alvaro. I am having a bit of trouble understanding the need for FileContext, Delta, Content and the proposed abstraction. It would be helpful if you could provide more background as to why you added these to the model and share the discussion around the changes. I assume that this was driven by the existing model for R4E. Do you have a list of requirements for the model such as per user serialization, representation of deltas that would help to understand it better? On a higher level, my suggestion would be to start with concrete use cases and a model that is scoped to address these while keeping future extensibility in mind. I would aim to make the model as concrete as possible. The more information the base model has, the more functionality can be provided by the framework. Once an initial model is in place I would extend it as new use cases are implemented. As an example, I would look at the Mylyn Review integration and start with a model sufficient to cover the current functionality + line based commenting. My impression is that the Oct 15 model provides most of that. What would need to be added is a representation for patches and line-based locations. My sense is that it's easier to drive the model through implementation and revising it frequently rather than trying to agree on a complex model beforehand.
One minor suggestion: I would rename Task to TaskReference and simply use an EString to identify a task. Each Mylyn task has a unique string handle that can be used for that.
Created attachment 181090 [details] Data model sketch from Vancouver's meeting
Created attachment 181339 [details] Agreed meeting on October 20th
The first version of the data model for Mylyn Reviews framework is now available in the git repository, http://git.eclipse.org/c/mylyn/org.eclipse.mylyn.reviews.git/ The model reflects the structure agreed in the Mylyn Reviews meeting (October 20th)
I have added two wiki pages to capture some of the discussion that happened in Vancouver: http://wiki.eclipse.org/Reviews/Features http://wiki.eclipse.org/Reviews/Design
I have pushed an org.eclipse.mylyn.reviews.core and org.eclipse.mylyn.reviews.ui plug-ins which extract the model and common UI components from the Gerrit plug-ins. The main difference is that I added concrete class such as FileItem, FileRevision and LineLocation. I saw that there are similar class in R4E. We could try to find a common abstraction or otherwise consider moving these additional class to a separate model. Alvaro, can you take a look and let me know what your thoughts are?
Hi all, it's been so long since this initial work, that I'm wondering if we should close this one to capture work to date, and continue with bug 389853 or if we should mark that one as a dup of this?
*** Bug 389853 has been marked as a duplicate of this bug. ***
From dup bug: We'd like to enrich the current Reviews model with enhancements that have been developed as part of R4E effort. At the same time, we'll look at other model enhancements and generalizations. The aim will be to have a model that is general, rich and designed ot be extensible for tools specific concerns, but that is itself free of such tool specific concerns. I'm just starting to explore that now working with R4E. For the first phase, we'll just be re-integrating the R4E model to make it dependent on the common reviews pieces.
Please see gerrit's for initial changes. (It's awkward, but we're spanning repos so we have to use multiple reviews. We'll need to work hard to keep the merges to master in Sync.) I'd like to follow up with these on gerrit as we can track all of the comments to specific changes. Let's keep the general comment stream on the reviews (not reviews.r4e) gerrit. https://git.eclipse.org/r/7844 https://git.eclipse.org/r/7845
Created attachment 221277 [details] mylyn/context/zip
One thing we have to be careful in merging the Gerrit and Review Frame models is that there are a few transient fields in the Reviews frame models that might not be used in Gerrit but are in R4E. These should be preserved as R4E rely on them, due to the fact that R4E subdivides some model elements so that there are serialized per user, to minimize write contention/merging problems.
(In reply to comment #21) > One thing we have to be careful in merging the Gerrit and Review Frame models is > that there are a few transient fields in the Reviews frame models that might not > be used in Gerrit but are in R4E. These should be preserved as R4E rely on > them, due to the fact that R4E subdivides some model elements so that there are > serialized per user, to minimize write contention/merging problems. I believe that those all still exist in the core. Note btw what I'll be referring to as the reviews.core model, also happens to be used for Gerrit as well. This reconciled model is really a proper superset of the "frame" (Current R4E base) model. It might make sense to break out a specific Gerrit model, but my thinking is probably not. The additional (e.g. not used in r4E) classes provide lone number functionality for Location that is general and I think appropriate for all kinds of Reviews. Sebastien and I discussed today the possiblity of preserving full backward compatibility with existing R4E models. I looked at that, but it doesn't seem to be an option, as the namespace for the models will change even if we make no other breaking changes. So while models *will* be fully mappable, they will not be file compatible and we'll need some simple mechanism for converting them. See bug 390065.
Here is proposed plan for working with the complexities of completing a common model design. As background, we've been doing these on Gerrit, but the granularity is way too low for the scale and complexity of the changes proposed, and as a non-comitter I am really hampered by 250-line limit. 1. I do a full round of re-factoring, aligning and rationalizing the existing models on a github branch. 2. I discuss key issues with all participants and put together a Wiki page summarizing any areas that need clarification or decision and asking for input.* 3. We have scheduled meeting(s) to discuss any of these issues. * We'll try to do these as part of regular Mylyn meetings but that might be practical -- so if anyone wants to participate in these discussion, please let me know and I'll make sure that you are included. 4. Iterate 1-3 as needed. 5. When all parties sign-off *, submit CQ for github changes. 6. While waiting for 5 and in parallel with the other stuff, I can continue to do work on R4E and Reviews to try to earn my committer status. :) If I can get that status in time, we won't need CQ. *Perhaps we could even capture the whole thing as an R4E review, which would be excellent dog-fooding. Does this seem workable to everyone?
Steffen brings up question about why I've made EMF classes extend EObject. See: https://git.eclipse.org/r/#/c/8054/. There are a number of reasons for this: 1. Practically speaking, it would mean modifying many many lines of R4E code to perform unsafe coercions to EObjects. 2. EObject provides nothing but support for standard persistence, reflection and related object references. It is analogous to making the assumption that we are extending Object for a POJO. It simply doesn't make sense to not have these services available for a managed object. 3. As a side-point, EMF interfaces have a somewhat different role from Java interfaces. Because the code is generated, it is expected that the entire contract is defined in the model itself. In that sense, the contract is actually specified in .ecore and realized in the Java interface. Interfaces don't buy you quite as much as they do in a common Java API. (That might be why they are not by default generated as "I" types, but I've kept the current usage in that case.) In addition to separating specification from implementation, they do facilitate multiple-inhiertance, implementation inter-operability and other nice things. 4. We will be leveraging EMF enormously throughout the Reviews implementations. The basic contract for a Reviews model object is that it is implemented in EMF. I don't think we can or should anticipate someone implementing a reviews model that is not using EMF. All of our UI and persistence mechanisms depend on it. 5. This is the standard EMF practice. I know of no API EMF that don't do things this way though of course it would be interesting to see counter-examples. In short, it's a useless and actually harmful pretense to assume that our IReviews objects are not in fact EObjects. ;)
BTW, I should add that I don't mean to short-circuit discussion about whether it is overall a good thing to make all API that leverages EMF in any way exposed as common Java API. I'm not convinced we *should*. I'm just saying that if we want to provide generic API support, we should be clear that we are doing that. In fact, that's a good argument for not generating the code in the I[ClassName] form. As we've discussed before, it's not clear that EMF and the standard Eclipse approach to API exposure are a very good fit. Arguably, to the extent that we are exposing these as API, they probably shouldn't also be the EMF generated objects but perhaps say a higher-level API wrapper or other services. If we do want to provide extendible API, then we could do something like generating the "I" interfaces for some subset as appropriate (perhaps in a separate genmodel) and then manually extending those interfaces from the Ecore model. We would then treat the generated EMF interface objects as non-API artifacts except for the case where we are extending them with other EMF derived implementations. For the purposes of non-EMF based extensions, consumers would need to treat those classes as essentially final classes, similar to say Mylyn TaskData. But unfortunately there aren't any Eclipse supported API enforcement mechanisms that would be able to make these kinds of subtle distinctions. :)
(In reply to comment #24) > 1. Practically speaking, it would mean modifying many many lines of R4E code to > perform unsafe coercions to EObjects. We certainly don't want to introduce unnecessary casts. Can you provide a typical example where R4E is using EObject API when accessing the model? > 2. EObject provides nothing but support for standard persistence, reflection and > related object references. It is analogous to making the assumption that we are > extending Object for a POJO. It simply doesn't make sense to not have these > services available for a managed object. The EObject API is very lean but I still consider it an implementation detail whether EMF is used or not. > 3. As a side-point, EMF interfaces have a somewhat different role from Java > interfaces. Because the code is generated, it is expected that the entire > contract is defined in the model itself. In that sense, the contract is actually > specified in .ecore and realized in the Java interface. Interfaces don't buy you > quite as much as they do in a common Java API. (That might be why they are not > by default generated as "I" types, but I've kept the current usage in that > case.) In addition to separating specification from implementation, they do > facilitate multiple-inhiertance, implementation inter-operability and other nice > things. We are just following Eclipse convention here. > 4. We will be leveraging EMF enormously throughout the Reviews implementations. > The basic contract for a Reviews model object is that it is implemented in EMF. > I don't think we can or should anticipate someone implementing a reviews model > that is not using EMF. All of our UI and persistence mechanisms depend on it. Agreed. > 5. This is the standard EMF practice. I know of no API EMF that don't do things > this way though of course it would be interesting to see counter-examples. AFAIK, the e4 API does not expose EMF. (In reply to comment #25) > We would then treat the generated EMF interface objects as non-API artifacts > except for the case where we are extending them with other EMF derived > implementations. For the purposes of non-EMF based extensions, consumers would > need to treat those classes as essentially final classes, similar to say Mylyn > TaskData. But unfortunately there aren't any Eclipse supported API enforcement > mechanisms that would be able to make these kinds of subtle distinctions. :) That is something I have been wondering about. I would like to annotate all generated interfaces as @noextend and @noimplement (while still leaving an option to extend the model). The default templates don't seem to support adding annotations though and it would discourage any extensibility. Miles, you make a number of good arguments why it doesn't cause harm to have EObject in the API but I still don't see the benefit look at it from the framework angle. Do you know if it's possible to generate the framework model suppressing EMF types while exposing them in the R4E model?
(In reply to comment #26) > We certainly don't want to introduce unnecessary casts. Can you provide a > typical example where R4E is using EObject API when accessing the model? There are two cases. 1. Because the R4E model assumes that interfaces are EObjects, all of the generated code fails. What this implies is that EMF genmodel doesn't actually support the case where a model doesn't extend EObject but an extending model does! So we'd also need to change the R4E model to match, a change that I've been reluctant to make but that might be warranted. 2. There are ~50 cases where EObject API is used. However, in most if not all of those cases, that's required by the resource model. These generally are supporting the fine-grained record control that R4E provides, and are locating the resource that an object resides in but also support the R4E UI. Whether that is something that should be generally supported is arguable, but in this case there is a practical matter of a lot of manual code changes. > The EObject API is very lean but I still consider it an implementation detail > whether EMF is used or not. That's an interesting issue. At some level, everything is an implementation detail. :D (Consider that Java objects imply thread semantics.) OTOH, I also see interfaces as implying/enforcing contracts and sometimes that means erring on the side of over-specification. IOTW, if someone tried to implement a non-EMF IReview, should that be supported? (And if it's noextend, then it becomes a moot-point.) > We are just following Eclipse convention here. Yep, I'm just making the case that Eclipse convention and EMF reality aren't always aligned. > AFAIK, the e4 API does not expose EMF. Ha! As soon as I submitted my last comment I said to myself, "geez, I bet e4 doesn't expose that".. Though again here, E4 would make the assumption that noone else should be managing the life-cycle of it's objects. That's the real dividing line here -- do we want to "allow" implementors to control their object's life-cycles. In the case of R4E as consumer, that decision has already been made, the question is whether we want to reify that or not. > That is something I have been wondering about. I would like to annotate all > generated interfaces as @noextend and @noimplement (while still leaving an > option to extend the model). The default templates don't seem to support adding > annotations though and it would discourage any extensibility. I think you could do that through customizing the templates. I can look into how compicated that would be, but not right now. :) > Miles, you make a number of good arguments why it doesn't cause harm to have > EObject in the API but I still don't see the benefit look at it from the > framework angle. Do you know if it's possible to generate the framework model > suppressing EMF types while exposing them in the R4E model? Bottom-line, it's one of these things where the purist argument would say we need to have a very good reason to extend, and the practical argument would say "why, if it makes no difference to anyone?". I can be as much of a purist as anyone else, but can we think of any real-world issues that extending EObject would cause? Because not having it *will* create a number of headaches. At some point the persistence mechanism for R4E may evolve to the extent that many of these casts are no longer necessary, but basically, we're going to have to go and make a bunch of manual changes to code that will introduce possiblity for errors. I'll add that I've run into other cases where having the interfaces extending eobject has been important, but those aren't relevant to this specific case.
(In reply to comment #27) > 1. Because the R4E model assumes that interfaces are EObjects, all of the > generated code fails. What this implies is that EMF genmodel doesn't actually > support the case where a model doesn't extend EObject but an extending model > does! So we'd also need to change the R4E model to match, a change that I've > been reluctant to make but that might be warranted. Ok, if having different settings for the genmodels isn't an option then it's most sensible to change the (much smaller) framework model. > That's an interesting issue. At some level, everything is an implementation > detail. :D (Consider that Java objects imply thread semantics.) OTOH, I also see > interfaces as implying/enforcing contracts and sometimes that means erring on > the side of over-specification. IOTW, if someone tried to implement a non-EMF > IReview, should that be supported? (And if it's noextend, then it becomes a > moot-point.) No, it shouldn't. At the same time EObject exposes details about change notifications, containment etc. that I'm not convinced should be part of the API contract. > Ha! As soon as I submitted my last comment I said to myself, "geez, I bet e4 > doesn't expose that".. Though again here, E4 would make the assumption that > noone else should be managing the life-cycle of it's objects. That is the very same assumption that I make about the Mylyn frameworks (builds, tasks, reviews): Persistence and life-cycle of framework objects is a framework concern that shouldn't be exposed to connectors. That said, R4E isn't a connector but a full-on review tool implementation that reuses framework components and we haven't built persistence into the Reviews framework. > That's the real dividing line here -- do we want to "allow" implementors to > control their object's life-cycles. In the case of R4E as consumer, that > decision has already been made, the question is whether we want to reify that or > not. In the interest of moving forward it seems reasonable to change the framework design in this case. While I am generally in favor of richer frameworks that encapsulate common concerns we have to strike a balance here and R4E already contains a ton of that stuff that we don't want to move or change at this point. > I think you could do that through customizing the templates. I can look into how > compicated that would be, but not right now. :) Yes, it's possible to do that but I don't really want to maintain customized templates.
(In reply to comment #28) > No, it shouldn't. At the same time EObject exposes details about change > notifications, containment etc. that I'm not convinced should be part of the API > contract. Yes, I agree that this is not ideal. (To be clear, it exposes that there *is* a containment reference, not what that containment is.) > That is the very same assumption that I make about the Mylyn frameworks (builds, > tasks, reviews): Persistence and life-cycle of framework objects is a framework > concern that shouldn't be exposed to connectors. That said, R4E isn't a > connector but a full-on review tool implementation that reuses framework > components and we haven't built persistence into the Reviews framework. Right. As we've discusses across R4E and Reviews teams, hopefully the R4E finer-grain locking schemes can eventually be replaced with more generic EMF solutions, which are supported in Reviews proper or even at a lower level, but that's a long way off. Unfortunately, part of the trade-off is that we'll then have to wait for a major version shift if we want to take those EObject dependencies back out. It might be worthwhile adding a note to that effect to the I[Name] docs. > Yes, it's possible to do that but I don't really want to maintain customized > templates. Totally agree.
In order to support the R4E multiple file-based approach we're going to need to enable proxiesfor containment relationships. (Confusingly enough, even when contaimnet refrenes are marked for resolve proxies in genmodel, this value is overridden by model level setting by default.)
Steffen, one other interesting issue that came up as I was working through manifest API issues. Because we have "Supresss EMF Metadata" set to true, the IReviewsPackage is not generated. This forces consumers to use the internal ReviewsPackage class whenever they need to use that metadata. This is actually a big deal for many usages, as it is those class and feature Literals that provide most of the EMF magic, by effectively turning Ecore classes and accessors into first class objects. Without that, consumers are always going to be doing an end-run around internal API restrictions. Thoughts?
See Gerrit for actual IP tracing, but note: I authored 100% of the content that I am contributing. I have the rights to donate the content to Eclipse. I contribute the content under the EPL.
(In reply to comment #31) > Without that, consumers are always going to be doing an > end-run around internal API restrictions. Thoughts? That is correct. Consumers who would like to use the EMF specific APIs of the model will need to rely on internals.
Marking as done now that R4E is also merged.
