Community
Participate
Working Groups
We'll be adding some features to core model mostly to support some common navigator and presentation features but that could also be more generally useful.
Copying comment from Miles from https://bugs.eclipse.org/bugs/show_bug.cgi?id=334967#c10: See review https://git.eclipse.org/r/#/c/9175/ I've proposed the following changes to the model to support the UI changes: •Create TopicContainer to encapsulate all items that (yes) can contain topics, that is Reviews and ReviewItems. •Created a derived "allComments" reference for TopicContainers. This represents all comments contained (reachable) from the container, e.g. Reviews contain all PatchSets comments, which contain all Artifact comments. •Move createTopicComment operation to TopicContainer, so Review instances provide this behavior. •The topics reference is no longer derived. This may break R4E functionality, but is a change we'll need to make at some point in any case. •Created an opposite reference from Comment "parentTopic" to Topic "comments". •Created an "owner" entry for Reviews. •Added a reference to FileItem (file) from File Revision. •Added rangeMin and rangeMax derived values for line locations. •Created an Orderable interface to allow items to place themselves in an arbitrary ordered space. (e.g. linenumber for comments) •Created a Dateable interface to support all classes that have creation and update dates. •Created a Dateable derived value "lastChangeDate" to return the last date the object was modified. (For case where updateDate is null, but creationDate is not.)
I'm slightly confused regarding the design of IComment and ITopic (which is an IComment itself). Are the following assumptions correct? * An ITopic is contained in a review. * Several review items (file revisions) can reference the same topics (not supported in Gerrit but in the model/R4E). * The ITopic instance is the first comment of a thread. * Referenced IComment instances are replies to the topic or replies to replies. The "replies" and "comments" fields of ITopic seem to overlap in a confusing way. I'm also confused by the fact that an ITopic can have several locations but one IReviewItem. Miles, with the change to allowing multiple locations per topic would it make more sense to tie the item to the location? I believe I avoided this complication in the framework model originally since there wasn't a use-case in the Gerrit connector.
(In reply to comment #2) > I'm slightly confused regarding the design of IComment and ITopic (which is > an IComment itself). Are the following assumptions correct? > > * An ITopic is contained in a review. > * Several review items (file revisions) can reference the same topics (not > supported in Gerrit but in the model/R4E). > * The ITopic instance is the first comment of a thread. > * Referenced IComment instances are replies to the topic or replies to > replies. > > The "replies" and "comments" fields of ITopic seem to overlap in a confusing > way. > > I'm also confused by the fact that an ITopic can have several locations but > one IReviewItem. Miles, with the change to allowing multiple locations per > topic would it make more sense to tie the item to the location? I believe I > avoided this complication in the framework model originally since there > wasn't a use-case in the Gerrit connector. Some clarifications, from an R4E perspective: * ITopics are indeed contained in a Review * The ITopic instance is indeed the first comment of a thread. In R4E we extend ITopic with R4EAnomaly. * In R4E, the Topics (R4EAnomalies) are not directly linked to the Review Items themselves. What happens is that there is a mapping done to link Topics with File Versions, but only in the UI. The way this is done is at the opening of the review (when UI model data structures are populated) by comparing the File Version the Topic (Anomaly) is written on, and compare it with each file version under all the Review items. If a match is detected, then the Topic (Anomaly) is added under this File version in the UI model only. In the core model, Topics are treated independently and only refer to their parent review. So the bottom line if we do use the Review Item references to Topics (at least as of now). The Review element is the sole "Topic Container" * IComments are child comments of an ITopic i.e. replies to it. In R4E, we extend the IComment with R4E. We also allow only 1 level of replies to an ITopic (for now anyways), but in Gerrit the are no limits on replies to other replies. I agree that the "comments" field in Topic seems superfluous, as Topic is a Comment, which already has the "replies" filed for the same purpose. For ITopic having several locations, we had put that in the model at the beginning to support having the same comment at various locations. However, the do not use it right now, since this would mean that the anomalies at all the various locations will really be the same one and cannot be tracked independently. Right now, we clone the Topic (R4EAnomaly) every time, so we only use 1 location per Topic. I'm actually not sure if we should keep the multiple locations per topic, or only support one? Regarding Comment positioning, I think the actual approach of relying on line locations is a little bit too restrictive. For instance, in R4E we can define comments down to the character position (e.g. put a comment on part of a line). It would be good to support this kind of granularity in the model. Also, later on when e.g. supporting model reviews, the location will not necessarily be kept in terms of line numbers. So having a generic Location is perfectly OK, but maybe the extension of the this generic location i.e. LineRange should maybe have startPosition and length members, on top of the current start and end member it has (See R4ETextPosition on how this is done in R4E). Also in R4E, since anomalies only belong to parent Reviews, we extended the Location so that the Anomaly (Topic) has a direct reference to the specific File Version (file) it is written on. This is also needed to map anomalies to the correct files (as I explained above). Now regarding Miles changes, some comments: The new TopicContainer element change is OK, It won't really affect R4E for now, as we only use the Review element as Topic container anyways. However, removing the derived attribute from the Topic will indeed badly break R4E right now. The reason the Topic is derived now in R4E has to do with the fact that the Comments are actually serialized per user, and not in the Review resource file itself. This was done to minimize the risk of collisions/merge occurrences when serializing the information to file. As we move towards supporting offline reviews, we are planning to remove this user division (which will then be obsolete, and then it will be OK to remove the derived. We would prefer this is not touched now if it can be avoided. Otherwise we will have to manually hack the code in the R4E Model to manually emulate the derived behavior for topics, which is not pretty. All other changes are OK, but see my comments above on line locations and position tracking in general.
(In reply to comment #2) > I'm slightly confused regarding the design of IComment and ITopic (which is an > IComment itself). Darn, I was sort of assuming that you knew waht was going on here, because I'm confused about the distinction as well. > * An ITopic is contained in a review. Yes, for *global* comments, where the relationship is 1:*. No for comments on file revisions. We need to be careful about what we mean by "containment", because we haven't actually specified containment relationships yet, as doing so would cause issues with current R4E persistence mechanism. But yes, TopicContainer topics would be a containment relationship if we could define it that way now.) > * Several review items (file revisions) can reference the same topics (not > supported in Gerrit but in the model/R4E). No, I think that topics should be referenced by one and only one review item. This should be a containment reference, but can't be until we > * The ITopic instance is the first comment of a thread. That's what I had thought. And this is why I added in support for "all comments". > * Referenced IComment instances are replies to the topic or replies to replies. Yes. (This came up when discussing this review https://git.eclipse.org/r/#/c/7688/) > The "replies" and "comments" fields of ITopic seem to overlap in a confusing > way. Totally! What I found really odd in the existing code was that a user created comment actually creates two comments; a parent "topic" and a child "comment", both with the same creation date, author, message, etc.. Can you think of any reason why this would have made sense? To my eye, topics serve as a sort of anchor for, well a topic of conversation, and that could support multiple locations in code (as R4E does), generic subject threads, etc.. My take is that a Topic shouldn't be a Comment in the first place. So for example when a user creates a comment for I think this is all complicated by the fact that none of this has actually been supported in the UI (you couldn't reply to comments anyway!) so this hasn't really been exercised fully. But I just saw Sebatien's response and that clarifies things. > I'm also confused by the fact that an ITopic can have several locations but one > IReviewItem. Miles, with the change to allowing multiple locations per topic > would it make more sense to tie the item to the location? I believe I avoided > this complication in the framework model originally since there wasn't a > use-case in the Gerrit connector. From Sebastien's response it looks like rather than make locations * cardinality, we should have changed it to single cardinality for R4E. If we "assume" one and only one location per topic, then it still makes sense to tie this to item, but otherwise it is sort of a mess. If you wanted to support this properly, I think you would actually want a "TopicLocation" class, holding a reference to a topic, file item, and whatever arbitrary coordinate (e.g. line number, offset, model item) that an implementation provides. But I'm loath to make a large change like that at this point. I think we could legislate for now that the locations are meant to refer to locations within the one and only one file referenced from the topic. Personally, I see a lot of advantage in the idea of allowing topics to refer to multiple locations and even file revisions, but that will be a lot easier to manage when we have Edit support and other mechanisms to ensure internal consistency. Otherwise the whole thing is kind of scary. To summarize, here are the options as I see them: 1. Consider modifying Topics so that they no longer extend Comments. (But I'd like to avoid this change for now, if we can.) 2. a) Consider creating a TopicLocation class as described above. b) Return Topic location cardinality to 1:1. c) Leave Topic as it is for now, even though we recognize that having one file item reference and multiple locations doesn't actually make much sense. My feeling is is that we should probably leave things as they are for the time being, but plan to come back to this as part of a new iteration of model improvements once we have a better handle on what features model consumers actually need.
(In reply to comment #4) > > * Several review items (file revisions) can reference the same topics (not > > supported in Gerrit but in the model/R4E). > > No, I think that topics should be referenced by one and only one review item. > This should be a containment reference, but can't be until we Whoops, petered out there.. The point here is that review items aren't simply FileItems. They can also be ReviewItemSet (e.g. Gerrit Patchsets). Therefore, a topic could be contained at any of these levels. This is what really needs to be worked out eventually, because there are actually two issues here: 1. Where a topic is *contained*. That is, who really "owns" the topic? I might create a topic that refers to just a single file, or one that refers to files within a patchset, or one that refers to files across patchsets. (Or even across reviews, but let's not even go there!) 2. What artifacts a topic *refers* to. Right now in Gerrit at least, those two concerns are elided. But that is a limiting assumption. In any case for now we should probably simply focus on ensuring that we support the Gerrit and R4E use cases properly as they are. The current model doesn't make perfect sense, but I think it at least works.
> > * An ITopic is contained in a review. > > Yes, for *global* comments, where the relationship is 1:*. No for comments > on file revisions. For R4E all comments (global and linked to file revisions) are currently linked to reviews only. In my previous comments, I explain how in R4E we manage to map the comments to their respective file versions. But there is no explicit relationship between Topic and Review Items / File Items that is used in R4E. This is done to minimize merges as I state above > > To summarize, here are the options as I see them: > > 1. Consider modifying Topics so that they no longer extend Comments. (But > I'd like to avoid this change for now, if we can.) Please no :-) > 2. a) Consider creating a TopicLocation class as described above. > b) Return Topic location cardinality to 1:1. > c) Leave Topic as it is for now, even though we recognize that having one > file item reference and multiple locations doesn't actually make much sense. > > My feeling is is that we should probably leave things as they are for the > time being, but plan to come back to this as part of a new iteration of > model improvements once we have a better handle on what features model > consumers actually need. My vote is for option 2c) only, although 2b) could be added too. Options 1 would be problematic so I prefer not and 2a) would in theory be OK, but I'm afraid is could break the R4E serialization. As for the change of the topic being derived in Reviews etc. we need to discuss this offline, as this has major implications for the current R4E.
(In reply to comment #6) > > 1. Consider modifying Topics so that they no longer extend Comments. (But > > I'd like to avoid this change for now, if we can.) > > Please no :-) Do you mean "please no, that's a terrible idea", or "please no, let's not complicate things further right now"? Because I don't think that Topics should be Comments ultimately. > My vote is for option 2c) only, although 2b) could be added too. Agreed. > As for the change of the topic being derived in Reviews etc. we need to discuss > this offline, as this has major implications for the current R4E. Yep, one of the options we've been considering is to create some kind of non-derived clientTopics reference. Then we could implement the derived value for Gerrit as simply returning the clientTopics. Ugly as hell, but we'd replace it with simply topics when we do away with the serialization mechanisms currently in R4E. This might make things ugly for other consumers, but I think we have to recognize that there are going to be a number of changes over the next little while...
(In reply to comment #7) > (In reply to comment #6) > > > 1. Consider modifying Topics so that they no longer extend Comments. (But > > > I'd like to avoid this change for now, if we can.) > > > > Please no :-) > > Do you mean "please no, that's a terrible idea", or "please no, let's not > complicate things further right now"? Because I don't think that Topics > should be Comments ultimately. > In R4E topics are extended as anomalies, with state tracking etc.. so we would like to keep the structure as it is now for the time being.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > > 1. Consider modifying Topics so that they no longer extend Comments. (But > > > > I'd like to avoid this change for now, if we can.) > > > > > > Please no :-) > > > > Do you mean "please no, that's a terrible idea", or "please no, let's not > > complicate things further right now"? Because I don't think that Topics > > should be Comments ultimately. > > > > In R4E topics are extended as anomalies, with state tracking etc.. so we would > like to keep the structure as it is now for the time being. I can't remember why we ended up adding a comment in Gerrit instead of specifying attributes on the topic. It looks to me that we are using the model in a different way then intended so this should be fixed in the framework / Gerrit. Having topic and comment separated could make the model easier to understand but I'm okay keeping the current structure where topic is also a comment.
(In reply to comment #9) > I can't remember why we ended up adding a comment in Gerrit instead of > specifying attributes on the topic. It looks to me that we are using the model > in a different way then intended so this should be fixed in the framework / > Gerrit. Having topic and comment separated could make the model easier to > understand but I'm okay keeping the current structure where topic is also a > comment. I think there are clear topic concerns that are relevant to all comments and that conversely topics aren't really comments at all. By analogy, this closest to the distinction between a thread and a post on an internet forum. I'd really like to revisit this if we have an opportunity down the line.
"clear topic concerns that are relevant to all comments " -> "clear topic concerns that are *not* relevant to all comments" e.g. location, task reference, title...
(In reply to comment #5) > (In reply to comment #4) > > > * Several review items (file revisions) can reference the same topics (not > > > supported in Gerrit but in the model/R4E). > > > > No, I think that topics should be referenced by one and only one review item. > > This should be a containment reference, but can't be until we Okay, > > Whoops, petered out there.. > > The point here is that review items aren't simply FileItems. They can also be > ReviewItemSet (e.g. Gerrit Patchsets). Therefore, a topic could be contained at > any of these levels. This is what really needs to be worked out eventually, > because there are actually two issues here: > > 1. Where a topic is *contained*. That is, who really "owns" the topic? I might > create a topic that refers to just a single file, or one that refers to files > within a patchset, or one that refers to files across patchsets. (Or even across > reviews, but let's not even go there!) Let's discuss that separately. For Gerrit it's only semi relevant at the moment (see https://git.eclipse.org/r/#/c/7688/2/org.eclipse.mylyn.reviews.core/model/reviews.ecore). > In any case for now we should probably simply focus on ensuring that we support > the Gerrit and R4E use cases properly as they are. The current model doesn't > make perfect sense, but I think it at least works. We changed the cardinality of topic - locations to 1:* under the assumption this was blocking converging on a common model that could be consumed by R4E. Since that doesn't appear to be the case and the change complicates the model and the code handling topics I'm in favor of reverting that change and going back to a 1:1 relation. The later we make that change the more expensive it's going to be. At the moment it might be as simple as reverting a few commits. Miles, if you could make a good use case to allowing multiple locations for a topic we could also go down that route but we would need to add the notion of a TopicLocation or extend location to reference the review item because the current structure doesn't make sense, i.e. supporting multiple locations within the same review item. In summary I'm suggesting the following changes: * Remove comments attribute in ITopic (and use replies where appropriate). I think this might have been added originally to enable topics to contain their child comments but that there are likely beter solutions to that problem. * Change framework to use the topic to store the data for the first comment. * Revert change that added support for multiple locations per topic.
(In reply to comment #12) > We changed the cardinality of topic - locations to 1:* under the assumption this > was blocking converging on a common model that could be consumed by R4E. Since > that doesn't appear to be the case and the change complicates the model and the > code handling topics I'm in favor of reverting that change and going back to a > 1:1 relation. The later we make that change the more expensive it's going to be. > At the moment it might be as simple as reverting a few commits. Yeah, and eating a bit of crow on my part. ;) https://git.eclipse.org/r/#/c/7908/ http://dev.eclipse.org/mhonarc/lists/mylyn-reviews-dev/msg00636.html Sorry, Robert. ;) But actually, R4E team have a lot of code that makes the assumption that there are multiple locations. Guys, are you up to making the changes neccesary on your end? Note that the merge won't be straightforward given the other changes. > current structure doesn't make sense, i.e. supporting multiple locations within > the same review item. Clearly the way it's set up doesn't make much sense. > In summary I'm suggesting the following changes: > > * Remove comments attribute in ITopic (and use replies where appropriate). I > think this might have been added originally to enable topics to contain their > child comments but that there are likely beter solutions to that problem. Note that those comments will have to be contained somewhere. I actually think that topics is a good place to keep them. > * Change framework to use the topic to store the data for the first comment. To be clear, we want to replace the current approach of creating an essentially duplicate comment with one where we have a head comment. Should be straightforward since some of the code has already been isolated, but I'm concerned that there might be some hidden assumptions that will be overlooked. But my inclination especially given the issue of Topics vs. Comments above is to wait on this change until we revisit all of this functionality. Is there anything that would drive making that change now? > * Revert change that added support for multiple locations per topic. R4E team, +1 on that?
(In reply to comment #13) > (In reply to comment #12) > > * Remove comments attribute in ITopic (and use replies where appropriate). I > > think this might have been added originally to enable topics to contain their > > child comments but that there are likely beter solutions to that problem. I'm pretty convinced that we don't want to remove the comments entry. It's used in a number of places in both the Gerrit and R4E code. I also think it could be the most appropriate place for the containment relationship. Again, referencing https://git.eclipse.org/r/#/c/7688/2/org.eclipse.mylyn.reviews.core/model/reviews.ecore replies is a complex tree structure, and I think that using Reviews or ReviewItemSet would be too high level. If you consider a threaded conversation, you often want to be able to access all of the items in that list as if they were flat. That's exactly what we do in the content provider, where it wouldn't be appropriate to have replies listed as children. Of course, one could make this a derived rollup but I don't think that is right either. One thing to be clear on: is there one and only one root comment that all other comments reply on, or should it be possible to have multiple root level comments? If the former and if we also make Topics no longer extend comments, the question is where to put the first comment? We need something to build the tree off of. My proposal is to have a cardinality 1 "primaryComment"/"initialComment" or similar for that role. I think we could consider have a cardinality 1
To answer the questions directed toR4E: >But actually, R4E team have a lot of code that makes the assumption that there >are multiple locations. Guys, are you up to making the changes neccesary on your >end? Note that the merge won't be straightforward given the other changes. I checked today. It's actually not that bad in R4E. A few small bits of code to modify. We just have to synchronize so that our changes are committed soon after the common model changes are. For the Topics vs. Comments I agree let's wait for that, as it has bigger impacts for R4E. In R4E the "root" comment is a special one (we call it an Anomaly). It has all the information a normal comment has, but with more info on top of it, so the current structure is actually pretty well aligned with that. Currently, R4E only allows 1 level of subsequent comments, as I stated above. So the current R4E structure is Anomaly (Topic) Comment 1 Comment 2 Comment 3 etc. So it is a pretty flat structure. However, we can easily change that for a multi-level structure i.e. replies to replies to comments etc. to align with Gerrit. This should basically be viewed as an email thread or forum post thread. And yes it would be good to have the ability to display it in an hierarchical way, as well as in a flat list. However let's hold off for this.
(In reply to comment #13) > > * Remove comments attribute in ITopic (and use replies where appropriate). I > > think this might have been added originally to enable topics to contain their > > child comments but that there are likely better solutions to that problem. > > Note that those comments will have to be contained somewhere. I actually think > that topics is a good place to keep them. Okay, then let's keep it if it's not terrible from a modeling point of view. > > * Change framework to use the topic to store the data for the first comment. > > To be clear, we want to replace the current approach of creating an essentially > duplicate comment with one where we have a head comment. Yes. > Should be > straightforward since some of the code has already been isolated, but I'm > concerned that there might be some hidden assumptions that will be overlooked. True, but the overall code handling comments should still be reasonably limited. > But my inclination especially given the issue of Topics vs. Comments above is to > wait on this change until we revisit all of this functionality. Is there > anything that would drive making that change now? My main concern is that the Gerrit behavior is inconsistent with R4E which would make it difficult to share UI parts. I'm fine with delaying that change until the common navigator is merged but it's something that I would like to converge on. (In reply to comment #14) > One thing to be clear on: is there one and only one root comment that all other > comments reply on, or should it be possible to have multiple root level > comments? Gerrit has one root comment and one level of replies. > If the former and if we also make Topics no longer extend comments, > the question is where to put the first comment? We need something to build the > tree off of. My proposal is to have a cardinality 1 > "primaryComment"/"initialComment" or similar for that role. That works for me. I'll let you hash that out since to implement what makes the most sense in the light of R4E consuming the framework model.
I just had a talk with Alvaro here about the cardinality of locations within Topic and there is actually a good reason for having multiple locations per topic. This has to do with having the code ready to support model reviews, as in models a Comment (Topic) can refer to a group of elements together i.e. multiple locations. So, please let's forget about changing the cardinality of locations in topic and let's keep it to 1..* as it is now.
(In reply to comment #7) > (In reply to comment #6) > > As for the change of the topic being derived in Reviews etc. we need to > discuss > > this offline, as this has major implications for the current R4E. > > Yep, one of the options we've been considering is to create some kind of > non-derived clientTopics reference. Then we could implement the derived value > for Gerrit as simply returning the clientTopics. Ugly as hell, but we'd replace > it with simply topics when we do away with the serialization mechanisms > currently in R4E. This might make things ugly for other consumers, but I think > we have to recognize that there are going to be a number of changes over the > next little while... Okay, I've discussed this with Sebastien, and there seems to be no good way around this. So we're going to create a directTopics non-derived reference from TopicContainer now and revisit it when serialization gets replaced in R4E.
(In reply to comment #17) > So, please let's forget about changing the cardinality of locations in topic and > let's keep it to 1..* as it is now. Okay, we can leave this in the model for now. If it turns out that there is no use or consumer of this functionality within the release cycle I may reopen the discussion to remove the functionality.
https://git.eclipse.org/r/9175
The model changes were reviewed on https://git.eclipse.org/r/#/c/9175/ and merged as b26108c7b04aa412183a64b29bc0111d8697d0f1. Thanks for driving this, Miles!
