Community
Participate
Working Groups
Create a common navigator to browse the contents of the active code review.
Since I assume, that the navigator operates on a common review model I assume we have to extend that?
I think there would be a content provider for the common model (if we decide to provide that). The common navigator framework allows very extensible and flexible structuring of content though so the navigator wouldn't be limited to supporting that model.
Let me if we have a driver to do this for 0.7. I don't think the Gerrit connector will require this until later.
*** Bug 390366 has been marked as a duplicate of this bug. ***
Scheduled for next round of ui improvements.
Scope is expanded to include generic participants support as well.
Please see Gerrit: https://git.eclipse.org/r/#/c/8732/ I'd like to focus on basic implementation issues at this point, while I think there are a few niceties in there, there are many additional cool things we can do, but for now I'd like to get a working implementation in. Let's open up subtasks for UI ideas/nits, etc..
That sounds good to me and the current iteration already looks promising. Can you describe the minimal feature set that we are aiming for? From my point of view it would be nice to be able to: * See files and comments of the active patch set * Open the compare editor for a file. Multiple presentations and fancy label providers are nice to have but it might be easier to leave that for a later review.
(In reply to comment #8) > That sounds good to me and the current iteration already looks promising. > > Can you describe the minimal feature set that we are aiming for? From my > point of view it would be nice to be able to: > * See files and comments of the active patch set > * Open the compare editor for a file. Those are completed. We might want some way to highlight or filter for items in the last patch set. > Multiple presentations and fancy label providers are nice to have but it > might be easier to leave that for a later review. Those are all feature complete at this point in any case.
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 forgot that that review will break, because the existing UI can't compile against it. So we'll have to keep these on the same review.
Why does the change break the UI? Can we make an incremental change that doesn't or update the relevant parts of the UI? It's important that reviews are broken down into consumable chunks.
We renamed a usage. But yes, I guess we could include just that fix.
High level comments from looking at the first portion of the review: * I'm not convinced that the "Review Comments" content extension is needed. I would strongly prefer if we removed it to simplify the UI and discuss alternatives to using a content extension to satisfy the underlying use case (whichever that is). * I found the use of the server icon confusing. It makes sense to have a separate icon for generated comments but the server doesn't seem right since it's used for an entirely different purpose in other contexts. There are a number of classes that are not used such as ReviewsSorterDateable. I don't know what purpose they serve but unless there is a concrete use of these classes they should be removed from the review.
(In reply to comment #14) > High level comments from looking at the first portion of the review: > > * I'm not convinced that the "Review Comments" content extension is needed. I > would strongly prefer if we removed it to simplify the UI and discuss > alternatives to using a content extension to satisfy the underlying use case > (whichever that is). I feel strongly that this feature is helpful. In fact, in real world usage I've found it much more helpful and useful than the tree view. The thing that I find most broken about the current UIs is the inability to see inline comments and general comments in a common presentation, or even to be able to scan comments at all. Without this feature, we're still forcing users to drill down into each artifact to see comments, even if all they wanted to do was get an overview of what issues had been addressed and what hadn't. Also, when the view is not able to be presented flat, there are sorting options that can't be supported sensibly. Note that this feature is not at all unusual. For example, imagine how ponderous the Search view would be without the ability to show search results as a flat list. I agree that we could improve the underlying code and perhaps not expose this as a content extension, but I don't think it would be worth losing what I think is pretty cool functionality just for that reason. > * I found the use of the server icon confusing. It makes sense to have a > separate icon for generated comments but the server doesn't seem right since > it's used for an entirely different purpose in other contexts. Makes sense. Other ideas. > There are a number of classes that are not used such as ReviewsSorterDateable. I > don't know what purpose they serve but unless there is a concrete use of these > classes they should be removed from the review. These are there for future extensibility. I've been working with them as we develop the functionality, and I'd like to maintain them especially given differing potential approaches to e.g. bug 396921.
(In reply to comment #15) > I agree that we could improve the underlying code and perhaps not expose this as > a content extension, but I don't think it would be worth losing what I think is > pretty cool functionality just for that reason. Okay you convinced me that it makes sense to go with that. Part of what threw me off were the different column layouts in the presentation which made it look like it's separate widgets. I agree that columns are sensible for the navigator but usability wise it's already difficult to manage one set of columns. Having different columns in the various presentations isn't manageable. It would seem most obvious to merge the File and Location column. Have you tried that? > > * I found the use of the server icon confusing. It makes sense to have a > > separate icon for generated comments but the server doesn't seem right since > > it's used for an entirely different purpose in other contexts. > > Makes sense. Other ideas. Let's just not differentiate them from other comments for now. We don't have a good way to detect that anyways other than hard coding comment patterns which isn't right. > > There are a number of classes that are not used such as ReviewsSorterDateable. > I > > don't know what purpose they serve but unless there is a concrete use of these > > classes they should be removed from the review. > > These are there for future extensibility. I've been working with them as we > develop the functionality, and I'd like to maintain them especially given > differing potential approaches to e.g. bug 396921. Okay, that's fine to do that but they shouldn't be part of the review as I don't want to merge as long as they are work in progress. Sorry, if that's a bit more overhead to maintain in terms of other pending changes but it will speed up merging changes and we don't end up with dead code in master.
(In reply to comment #16) > It would seem most obvious to merge the File and Location column. Have you tried > that? Here's the problem there: We want to display location right aligned. If we put it immediatly after the file name, it won't be aligned. If we put it before the file name, we need to determine how much left padding to put it in. (Fixed value? Max line count across all files. :O) And it looks weird because there is a gap there. (I tried a bunch of permutations in the non-table version of all of this. BTW, I just noticed is hard to differentiate smack up against Last Change which also displays a number. Note that we can't use Styled Text for the location as I initially did, since you need to do a lot of hacks to make StyledText right aligned. Basically it comes down to whether you think it's more important to have these in a single column or to be able to easily compare file line numbers. Unless I've missed another way to do this. Also, I considered keeping file in the same column for both views, that is sharing the column in the flat view as it is shared in the tree view, but that would require adding a separate row for each file, and it wouldn't really be that much different from a resource tree structure then. One other possiblity for cleaning things up a bit would be to move the Author column in the flat presentation so that it is immediately before "Last Change". (That would solve the line number issue above.) I'd put Author where it was because it seemed much more natural to have author appear closest to the comment itself. But perhaps it would be a better tradeoff to move it? > Let's just not differentiate them from other comments for now. We don't have a > good way to detect that anyways other than hard coding comment patterns which > isn't right. Okay, I've removed the implementation (commented out though) from current patch. > Okay, that's fine to do that but they shouldn't be part of the review as I don't > want to merge as long as they are work in progress. Sorry, if that's a bit more > overhead to maintain in terms of other pending changes but it will speed up > merging changes and we don't end up with dead code in master. Removed dead code in current review. Let me nkow if I seeme to have missed anything.
(In reply to comment #17) > One other possiblity for cleaning things up a bit would be to move the Author > column in the flat presentation so that it is immediately before "Last Change". > (That would solve the line number issue above.) I'd put Author where it was > because it seemed much more natural to have author appear closest to the comment > itself. But perhaps it would be a better tradeoff to move it? Okay, I just tired that and remember now why I didn't like it. It's subtle so bear with me here.. Even though I'd really like to, we can't make comments span multiple columns, without significant SWT / StyledString hackery. That means that we get comment text cut off at the end of each column. That looks fine when there is always text in the next column, but it looks like crap when there isn't. There is always an author, but for global comments there isn't always a file. So by having the author column where it is we improve the look considerably.
(In reply to comment #17) > (In reply to comment #16) > > It would seem most obvious to merge the File and Location column. Have you > tried > > that? > > Here's the problem there: > > We want to display location right aligned. If we put it immediatly after the > file name, it won't be aligned. If we put it before the file name, we need to > determine how much left padding to put it in. (Fixed value? Max line count > across all files. :O) And it looks weird because there is a gap there. (I tried > a bunch of permutations in the non-table version of all of this. Having the line number right aligned has a benefit but I doubt that it's particularly important. If you look at the Markers view it doesn't have that either. Personally, I never look at the line number anyways since it doesn't mean anything, the logical element or artifact that a comment or marker is referring to is relevant to me, not the location within a file. > Basically it comes down to whether you think it's more important to have these > in a single column or to be able to easily compare file line numbers. Unless > I've missed another way to do this. It's much more import to me to have a single column layout and fewer columns are better. > Also, I considered keeping file in the same column for both views, that is > sharing the column in the flat view as it is shared in the tree view, but that > would require adding a separate row for each file, and it wouldn't really be > that much different from a resource tree structure then. Why? You could show the file as part of the location. I think *very* important that we share the columns otherwise switching presentations is like opening a whole different view. > One other possiblity for cleaning things up a bit would be to move the Author > column in the flat presentation so that it is immediately before "Last Change". > (That would solve the line number issue above.) I'd put Author where it was > because it seemed much more natural to have author appear closest to the comment > itself. But perhaps it would be a better tradeoff to move it? I like the current position. > Okay, I just tired that and remember now why I didn't like it. It's subtle so > bear with me here.. Even though I'd really like to, we can't make comments span > multiple columns, without significant SWT / StyledString hackery. That means > that we get comment text cut off at the end of each column. That looks fine when > there is always text in the next column, but it looks like crap when there > isn't. There is always an author, but for global comments there isn't always a > file. So by having the author column where it is we improve the look > considerably. That assumption isn't correct, there can be comments that have no author: java.lang.NullPointerException at org.eclipse.mylyn.internal.reviews.ui.providers.ReviewsLabelProvider.getImage(ReviewsLabelProvider.java:115) at org.eclipse.mylyn.internal.reviews.ui.providers.TableStyledLabelProvider.getColumnImage(TableStyledLabelProvider.java:57) at org.eclipse.mylyn.internal.reviews.ui.providers.ReviewsFlatLabelProvider.getColumnImage(ReviewsFlatLabelProvider.java:41) at org.eclipse.mylyn.internal.reviews.ui.providers.TableStyledLabelProvider$1.getImage(TableStyledLabelProvider.java:86) Anyways, let's try a simple approach before we add several column layouts. We can iterate over the column order later. The current ordering seems fine to me.
In the interest of avoiding two much back and forth on the presentation design I would like to make the following suggestion: * We go with a simple single column layout and combine the existing label provider implementations and aim to merge the change before the holidays. * We postpone the decision to change the code, do a UI review with a larger audience on the next Mylyn call in the second week of January and decide how to proceed then. Any preferences?
(In reply to comment #19) > Having the line number right aligned has a benefit but I doubt that it's > particularly important. If you look at the Markers view it doesn't have that > either. Personally, I never look at the line number anyways since it doesn't > mean anything, the logical element or artifact that a comment or marker is > referring to is relevant to me, not the location within a file. Yeah, you make a good case; Sam was saying the same thing. But where I think it does matter is ordering, since we generally work through the items one at a time. But that's implicit in the layour anyway. Honestly, for me it was more a matter of it looking messy, but OTOH, it allows the use of styled text for the line number. > It's much more import to me to have a single column layout and fewer columns are > better. I'm not convinced that it matters -- once you have columns, why limit the number? If you look at a view like the Git History or Problems, they have multiple items and I don't think it is an issue. > > Also, I considered keeping file in the same column for both views, that is > > sharing the column in the flat view as it is shared in the tree view, but that > > would require adding a separate row for each file, and it wouldn't really be > > that much different from a resource tree structure then. > > Why? You could show the file as part of the location. I think *very* important > that we share the columns otherwise switching presentations is like opening a > whole different view. Yeah, I can see your point that it can be confusing. In a way, I'm trying to have our cake and eat it to, because it seemed pretty clear that we did not want to have multiple views for this, which was my original preference. Let me see what I can come up with. > I like the current position. +1 > > there is always text in the next column, but it looks like crap when there > > isn't. There is always an author, but for global comments there isn't always a > > file. So by having the author column where it is we improve the look > > considerably. > > That assumption isn't correct, there can be comments that have no author: Whoops. What review did you see that on?
(In reply to comment #20) > In the interest of avoiding two much back and forth on the presentation design I > would like to make the following suggestion: > > * We go with a simple single column layout and combine the existing label > provider implementations and aim to merge the change before the holidays. > * We postpone the decision to change the code, do a UI review with a larger > audience on the next Mylyn call in the second week of January and decide how to > proceed then. I'd like to get in what we have, or very near to it. FWIW, I built a single column layout to begin with, so that's still there to play around with. Here's my POV: I'd *really* like to get this in before the holidays so that people can take a crack at it in the real world. There is definitely some fine-tuning to do or even perhaps whole sale changes, but the point is that now we have the infrastructure in place to easily move things around. So I'd love to have a UI review, but I also want to see this committed as soon as we have the remaing code issues in place. In the meantime, I'll try to work in some changes along the lines of what you and others have suggested. Does that seem reasonable?
I've just committed a number of changes that I hope will address at least some of the issues. I've consolidated the columns, made them size to max, and made a number of minor improvements. Most importantly, I've gone ahead an abstracted and simplified a number of the implementation details. It should now be quite easy to change things around/experiement with different layouts. The one thing that I have retained is splitting the columns for items into separate comment and artifact columns for the flat view. I hope when you see the latest iteration that it will make sense to you. I really thing combining the columns is not optimal, but in any case I'd like to put that discussion off for a a more comprehensive UI review in January. My hope is that we can merge this as it is now % any new code issues that might surface.
Thanks for making your efforts, Miles. Based on today's UI review these are the items I would prefer to have changed before submitting: * Revert label provider presentation for review editor to use full path name for files to make it easier to see the related items based on the path hierarchy. * Make artifact column the first column in the flat presentation and use something like "Global" if the artifact name is empty. It feels like there was something else but I don't remember now.
Okay, I've made those changes. w/ the "global" node it looks much better. (I tried using the actual change name in place of "Global" but it just created more text w/o any real benefit.) I can't rmemeber the other thing either. :)
Thanks for hanging in there, Miles! I have merged the review into master. There are still a few open tasks that need to be addressed and should be tracked on separate bugs: * The Gerrit editor now creates all controls eagerly to download all patch set content. This can have noticeable performance impact when opening the review editor depending on the size of the review and needs to be improved. * The Gerrit editor spawns a job per patch set which can slow down the UI for a reviews with many (> 20) patch sets. It would be better if data was cached more lazily and if there was a single job per review to do that. * The Gerrit UI plug-in is directly coupled to the navigator. That should not be the case. All navigator concerns should be handled by the framework. Miles, it would be great if you could open tasks for these.
(In reply to comment #26) > There are still a few open tasks that need to be addressed and should be tracked > on separate bugs: Okay, before I do that let me clarify these a bit more.. > * The Gerrit editor now creates all controls eagerly to download all patch set > content. This can have noticeable performance impact when opening the review > editor depending on the size of the review and needs to be improved. 1. So the concern here is the controls, not the patch set loading itself -- which is a separate issue (treated below)? > * The Gerrit editor spawns a job per patch set which can slow down the UI for a > reviews with many (> 20) patch sets. It would be better if data was cached more > lazily and if there was a single job per review to do that. 2. So what we're talking about here is sort of batching these up then, right? > * The Gerrit UI plug-in is directly coupled to the navigator. That should not be > the case. All navigator concerns should be handled by the framework. 3. Right, I think this was unavoidable for the most part in the the current implementation because of the coupling between the Gerrit UI and the the Gerrit API. I think we can untangle all of that. In fact, my thinking in all of this is that these concerns will be addressed as a part of the EMF Edit and Remote API work. (Don't have detailed bugs to refer to yet because we're in the midst of more detailed planning.) The current implementation is far from ideal, but should make a lot more sense when we're able to support a truly unified model, with for example updating of the patch set contents not happening simply because the user opened the editor.
I'm finding the caching of all patch sets as soon as the review is opened to be a major problem even with just 3 large patch sets. I have to go to the web UI if I want to actually see the inline comments. It seems like the most recent should be fetched first rather than fetching them all in parallel.
(In reply to comment #27) > > * The Gerrit editor now creates all controls eagerly to download all patch set > > content. This can have noticeable performance impact when opening the review > > editor depending on the size of the review and needs to be improved. > > 1. So the concern here is the controls, not the patch set loading itself -- > which is a separate issue (treated below)? Yes, particularly on Gtk just creating controls is slow (at least much slower than on Windows, nor sure about Mac). > > * The Gerrit editor spawns a job per patch set which can slow down the UI for > a > > reviews with many (> 20) patch sets. It would be better if data was cached > more > > lazily and if there was a single job per review to do that. > > 2. So what we're talking about here is sort of batching these up then, right? Yes, just doing things more sequentially rather than hitting the server with x parallel requests. > > * The Gerrit UI plug-in is directly coupled to the navigator. That should not > be > > the case. All navigator concerns should be handled by the framework. > > 3. Right, I think this was unavoidable for the most part in the the current > implementation because of the coupling between the Gerrit UI and the the Gerrit > API. I think we can untangle all of that. The review behavior class should already provide sufficient abstraction. It would simply need to be created through the connector rather than directly in the command handler. The other thing that is missing are proper event handlers for the model that update the Navigator view instead of relying on explicit refreshes from the Gerrit editor page. I don't think any of this requires larger framework changes.
(In reply to comment #29) > Yes, particularly on Gtk just creating controls is slow (at least much slower > than on Windows, nor sure about Mac). Good to know. There is 0 effect on Mac -- e.g. opening the editor for the common navigator review with 32 patch sets doesn't take any longer that opening a bugzilla review. I'll keep that in mind w/ future changes. > The review behavior class should already provide sufficient abstraction. It > would simply need to be created through the connector rather than directly in > the command handler. > > The other thing that is missing are proper event handlers for the model that > update the Navigator view instead of relying on explicit refreshes from the > Gerrit editor page. Right, that was the point of my comment below: >...The current > implementation is far from ideal, but should make a lot more sense when we're > able to support a truly unified model, with for example updating of the patch > set contents not happening simply because the user opened the editor. > I don't think any of this requires larger framework changes. It doesn't *require* them, my point is that the framework changes will ideally render them moot. I'll create the bugs so that we make sure that we're addressing those concerns.
(In reply to comment #26) > * The Gerrit editor now creates all controls eagerly to download all patch set > content. This can have noticeable performance impact when opening the review > editor depending on the size of the review and needs to be improved. bug 399697 > * The Gerrit editor spawns a job per patch set which can slow down the UI for a > reviews with many (> 20) patch sets. It would be better if data was cached more > lazily and if there was a single job per review to do that. bug 399699 > * The Gerrit UI plug-in is directly coupled to the navigator. That should not be > the case. All navigator concerns should be handled by the framework. bug 399700
