Community
Participate
Working Groups
Comments by other users should stand out from comments made by you. For example, the comment icon could be coloured differently or have some additional decoration (e.g. yellow person) when the comment is by you.
That sounds like a great idea!
Great idea! I'll implement this as part of bug 334967.
Miles, are you going to be working on this or should we assign back to mylyn-triaged? This was about the comment icon in the compare editor.
Couldn't resist this low-hanging fruit after bug 410673. :) https://git.eclipse.org/r/#/c/13838/ Sam, note this doesn't do what you want it to do yet. It only shows in the Review Navigator. But it should be pretty easy to adapt to the comapre editor as well. The issue there is whether we want to change the comment icon to a person...because we'd need to do that for it to make sense. (I personally am not keen on the glasses thing anyway..)
And actually, the compare editor "extensibility" makes it much more involved to do then it would otherwise be. Looks like we'd have to create multiple annotation types believe it or not, as the icon is specified in org.eclipse.ui.editors.markerAnnotationSpecification and there doesn't seem to be anyway to programmatically override it. That one's for another day, I think. :)
https://git.eclipse.org/r/#/c/13838/3 adds support for compare editor. Note that this also changes the icon from "glasses" to the standard "person" icons. Personally, I think it looks a lot better..thoughts?
Wecan probably get this in for 2.0.1, any reason not to?
I thought the decision about what to include in 2.0.1 was going to require more discussion. Whatever goes in, we want to be sure it works.
(In reply to comment #8) > I thought the decision about what to include in 2.0.1 was going to require more > discussion. Whatever goes in, we want to be sure it works. Well, yeah. ;) I'm posing the question because in this case it's not clear what we want to do. I could go either way, given that 2.0.1 isn't really about usability improvements but OTOH this change isn't likely to break anything since it just involves a few presentation details. (knock on wood)
We would need a justification to include enhancements in 2.0.1 rather than making a case to not include a new feature in a SR. This certainly sounds like a cool feature but from reading this bug I don't see any indication that this remove a significant impediment (an example of that may be the lack of Gerrit 2.6 support for example) or another justification for including it?
(In reply to comment #10) > We would need a justification to include enhancements in 2.0.1 rather than > making a case to not include a new feature in a SR. This certainly sounds like a > cool feature but from reading this bug I don't see any indication that this > remove a significant impediment (an example of that may be the lack of Gerrit > 2.6 support for example) or another justification for including it? Not really. The only justification I can see for including it now is not having it hanging around in my open Reviews. ;)
We can still merge the change but just not cherry-pick it to the SR branch, right?
(In reply to comment #12) > We can still merge the change but just not cherry-pick it to the SR branch, > right? See previous discussion. I was sort of kidding about not wanting to leave it hanging. I'd just as soon make master track 2.0.1 for now. That's going to make it much easier to do final acceptance testing and so on and make sure we don't miss anything.
Miles, are you planning on doing this for 3.10?
(In reply to Steffen Pingel from comment #14) > Miles, are you planning on doing this for 3.10? Yep, I was just waiting for the rest of Tomek's changes to come through and then merge and rebase. He's just got two more to go it looks like..
It might be out of scope but it could be a good idea to also indicate somehow the number of comments on a line, or at least indicate if there is more than one, perhaps using an icon with multiple persons.
Unfortunately the change didn't make the cut off for 3.10. I have set the milestone to the next release since it looks like the change is almost ready to be merged.
No biggie.. :) I think that Sam's changes have potential to increase scope quite a bit to support caching of patchset compares (at least to base) so we'll need to decide what to do w/ that.
Merged: https://git.eclipse.org/r/#/c/13838
Thanks Miles.
I am no longer able to view draft comments I just posted. I have to close and reopen the review and compare editor (the comments are also decorated with a blue person until I do this but that's less important). We should consider reverting this change if we can't fix that.
(In reply to Sam Davis from comment #21) > I am no longer able to view draft comments I just posted. I have to close > and reopen the review and compare editor (the comments are also decorated > with a blue person until I do this but that's less important). We should > consider reverting this change if we can't fix that. I'd suggest backing it out for now at least. It isn't important enough to cause a regression. I'll try to revisit it when I have a chance.
The only thing that could possibly break is that we've changed the comment model. But even if that created issues for existing comments, if shouldn't effect newly comments. Otherwise I'm at a loss to why this change would cause that issues. Nothing in error log?
No, but now I can't reproduce this. The comments are blue until I reopen the compare editor, but I have no trouble viewing them. It was probably unrelated to this change as it isn't uncommon that the inline comment popup won't open. I think I jumped the gun in reopening this, so I'm closing it again but I'll watch for this. Sorry for the false alarm.
There's a known limitation that the yellow person icon is not shown when clicking compare with base. > In the compare with base editor, we're comparing an item set that isn't part of > the review itself. Instead it's collected from Gerrit API. So that review item > set isn't actually connected to a review and we have no (immediate/obvious) way > to discover who the repository user ("me") is! Now, we could attach the compare > item set to a review when we create it -- I tested it and that works fine -- but > then the issue is that we have a review item set for the compare that is going > to be persisted with the review -- we can't do that as it would be a resource > leak. Now, it would be lovely to support caching of these item sets, but that's > a little out of scope.. ;) OTOH, we could dispose it when the compare editor is > disposed, but there isn't any obvious way to do that with the compare editor > 'API'. Ugh. > > What it comes down to is that we're looking at a lot of work to get the yellow > icon on the base compare, unless there is an approach I haven't thought of.
