Community
Participate
Working Groups
When viewing patch sets, the editor should indicate whether each file is modified, deleted, or renamed. The web UI does this and it's very useful.
Agreed, that is currently missing and would be very useful to have. It should be reasonably straight forward to add that as a decoration on the icon in the list of files. For renames it could also help to have a tooltip that shows the previous filename.
Hello Sam and Steffen, I started a review for this feature(?) here https://git.eclipse.org/r/#/c/9414/ Hope you've had a good new year!
Thanks Lei! It's great to see some progress on this.
Created attachment 228504 [details] wrong path shown As highlighted in the screenshot, the new path is shown on the base revision of a renamed file. Can that be fixed as part of this?
I'll defer to Lei, but personally I'd rather see all of the rename stuff a new task so that we can get these improvements in for EclipseCon demo. :)
Just to make sure I understand the screenshot, its supposed to say "com/tasktop/client/ui" on the highlighted part? Also that doesn't look like the same part I'm working on so it might take a bit.
(In reply to comment #6) > Just to make sure I understand the screenshot, its supposed to say > "com/tasktop/client/ui" on the highlighted part? > Also that doesn't look like the same part I'm working on so it might take a bit. Yeah, as I say, let's just go ahead and merge the current changes as is and work on the rename part afterward. We can hold this bug open rather than rename it. :) Okay?
Sounds fine to me.
(In reply to comment #6) > Just to make sure I understand the screenshot, its supposed to say > "com/tasktop/client/ui" on the highlighted part? That's right.
I will continue to work on the rename portion of this.
Thanks for your contributions so far, Lei!
Great work, Lei!
Thank you :D
*** Bug 402782 has been marked as a duplicate of this bug. ***
This a small thing, but we should consider not decorating /COMMIT_MSG with a + as it doesn't really make sense.
(In reply to comment #15) > This a small thing, but we should consider not decorating /COMMIT_MSG with a > + as it doesn't really make sense. Additionally, the file icon should also be decorated when it has been reviewed using a little checkmark, like the Gerrit WebUI does.
What determines whether a file has been reviewed? For me, just opening a file doesn't mean I've reviewed it. If this status is going to be shown, I would want to be able to mark a file as reviewed or not reviewed, like you can in the Gerrit WebUI.
(In reply to comment #17) > What determines whether a file has been reviewed? For me, just opening a > file doesn't mean I've reviewed it. If this status is going to be shown, I > would want to be able to mark a file as reviewed or not reviewed, like you > can in the Gerrit WebUI. When using the Gerrit webUI, opening the file shows a checkbox that is used to mark the file as reviewed. Similarly, in R4E, we provide a contextual command on the file icon in the R4E Review Navigator to the user that can be used to manually mark the file as reviewed. According to how the editor works in eclipse, we could either provide an compare editor contextual menu item checkbox to mark the file as reviewed, or use a contextual menu in the Gerrit task editor/Review navigator icon in a similar way R4E does it. Or both.
https://git.eclipse.org/r/#/c/11699/ started here.
Should we also indicate copied files like the web ui does?
Sure. But I've never actually noticed that.. Do you have an example review?
I just looked at the review again and re: our comments about hard-code string. This might be overkill, but I'm wondering if we should having all of the logic for this in the model. We'd add a ChangeType enum attribute to IFileItem and process it at retrieval time -- this would also allow us to take care of the special case where COMMIT_MESSAGE isn't counter as an "Add". https://git.eclipse.org/r/#/c/11699/1/org.eclipse.mylyn.reviews.ui/src/org/eclipse/mylyn/internal/reviews/ui/providers/ReviewsLabelProvider.java
That makes sense to me. (In reply to comment #21) > Sure. But I've never actually noticed that.. Do you have an example review? I only just noticed it myself, which is why I posted. I don't have a public example, but I'll post a screenshot.
Created attachment 229782 [details] copied files in Gerrit web ui
Wow, I wonder how they even detect that and what we'd check in the file item to infer it? My inclination is to say this is sort of an edge case that we can safely ignore but OTOH if we do this in model it would be an important case to at least represent. Thanks for pointing this one out, Sam!
BTW, I had one other thought about all of this. We're assuming like Gerrit assumes that all files under review actually *have* changed in some way. (The commit message seems to sort of a special case in Gerrit.) In fact with R4E and perhaps other review systems, you might add items into a ReviewSet that haven't been modified at all. How would you represent that? Sebastien, and thoughts?
(In reply to comment #26) > BTW, I had one other thought about all of this. We're assuming like Gerrit > assumes that all files under review actually *have* changed in some way. > (The commit message seems to sort of a special case in Gerrit.) In fact with > R4E and perhaps other review systems, you might add items into a ReviewSet > that haven't been modified at all. > > How would you represent that? Sebastien, and thoughts? Basically, in R4E you can manually add files to a given review. However R4E does not work like Gerrit because the review creator has to add the review content manually anyways, whereas in Gerrit a Review is always linked to a specific "change" and this change is automatically included with its parent review at creation. In R4E adding manually a file assumes that the "change" is actually the whole file. You can also add specific parts of files. However this is a moot point in Gerrit. Now for the copied files in Gerrit, we'll have to see how it does that. If this information is included or can be inferred from the Gerrit model data, then we could represent it as well. We could use a decoration on the file icon, add content to tooltip etc. I think that this should be tackled as a separate effort i.e. write another bug for this. This one is getting crowdy.
I guess Gerrit must infer copying based on file content (or perhaps a hash of the content). I don't know if it considers a file copied if you copy it and then change one byte. If this is available in the model, it could be useful to show it using a small version of the standard copy icon. It is an unusual case and may represent a mistake (i.e. maybe the copy should have been a move). As for unchanged files, perhaps they could be decorated with the glasses icon or similar to indicate that they have been explicitly added, but it sounds like this might not make sense in R4E and it isn't supported in Gerrit, so maybe that case can be ignored for now.
(In reply to comment #28) > As for unchanged files, perhaps they could be decorated with the glasses icon or That's a nice idea. In any case, I agree that we should hold on this one. What about idea of pushing this to model?
+1
I think I am going to start to work on the other features that Sebastien mentioned, some hints are appreciated (:
Lei, can you summarize what is left to do? I would suggest that we open a separate bug for decorating and managing files that have been reviewed. This will possibly require model changes and back-end communication to communicate which files have been reviewed back to Gerrit. Thanks very much for your great contributions so far!
Thank you Steffen (: I think what is left to do is: * remove graphic decoration from commit messages * Decorate file icons with a checkmark when it has been reviewed like in the Gerrit web-ui All of Sam's original suggestions are finished I think.
(In reply to comment #33) > I think what is left to do is: > * remove graphic decoration from commit messages That makes sense. Do you already have an idea how to implement that? > * Decorate file icons with a checkmark when it has been reviewed like in the > Gerrit web-ui Unless there is an easy way to do this with the current model let's defer that to a separate bug.
Good morning Steffen! For the first, I tried by checking if the image being retrieved was for the commit message in the getImage method with a hard-coded String, and I think Miles said to defer that to the bug. For the second one, I am not sure.
(In reply to comment #35) > Good morning Steffen! > > For the first, I tried by checking if the image being retrieved was for the > commit message in the getImage method with a hard-coded String, and I think > Miles said to defer that to the bug. Thanks, just found the conversation in comment #22 :). > We'd add a ChangeType enum attribute to IFileItem and > process it at retrieval time -- this would also allow us to take care of the > special case where COMMIT_MESSAGE isn't counter as an "Add". That makes sense to me. Miles, is that something you could do?
(In reply to comment #34) > (In reply to comment #33) > > I think what is left to do is: > > * remove graphic decoration from commit messages > > That makes sense. Do you already have an idea how to implement that? > > > * Decorate file icons with a checkmark when it has been reviewed like in the > > Gerrit web-ui > > Unless there is an easy way to do this with the current model let's defer > that to a separate bug. I agree with Steffen. For the reviewed file issue, a separate bug should be filed
(In reply to comment #36) > > For the first, I tried by checking if the image being retrieved was for the > > commit message in the getImage method with a hard-coded String, and I think > > Miles said to defer that to the bug. > > Thanks, just found the conversation in comment #22 :). > > > We'd add a ChangeType enum attribute to IFileItem and > > process it at retrieval time -- this would also allow us to take care of the > > special case where COMMIT_MESSAGE isn't counter as an "Add". > That makes sense to me. Miles, is that something you could do? > I agree with Steffen. For the reviewed file issue, a separate bug should be > filed There are a number of things we could do to enhance the model to support user cues like this. Another one if to differentiate between "humans" and "bots" for messages and to identify the user to support providing a Yellow icon. But yes, these should all go ina s eperate bug, like "enhance model to support user interface" or something..
(In reply to comment #38) > There are a number of things we could do to enhance the model to support user > cues like this. Another one if to differentiate between "humans" and "bots" for > messages and to identify the user to support providing a Yellow icon. But yes, > these should all go ina s eperate bug, like "enhance model to support user > interface" or something.. I was more thinking along a modification type for each file as you suggested, e.g.: ModificationType { Added, Modified, Deleted, Unchanged }.
ModificationType { Added, Modified, Deleted, Renamed, Unchanged }.
(In reply to comment #39) > I was more thinking along a modification type for each file as you suggested, > e.g.: ModificationType { Added, Modified, Deleted, Unchanged }. Right, that would nicely simplify the code. I'm just suggesting we could wrap all of these model changes together.
(In reply to comment #41) > (In reply to comment #39) > > I was more thinking along a modification type for each file as you suggested, > > e.g.: ModificationType { Added, Modified, Deleted, Unchanged }. > > Right, that would nicely simplify the code. I'm just suggesting we could wrap > all of these model changes together. The reason I'm suggesting to make this change now is because it's simple. Gerrit's Patch.getChangeType() method already returns exactly what we are looking for: ChangeType { ADDED, MODIFIED, DELETED, RENAMED, COPIED }. It should be straight forward to set that on the model.
(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #39) > > > I was more thinking along a modification type for each file as you > suggested, > > > e.g.: ModificationType { Added, Modified, Deleted, Unchanged }. > > > > Right, that would nicely simplify the code. I'm just suggesting we could wrap > > all of these model changes together. > > The reason I'm suggesting to make this change now is because it's simple. > Gerrit's Patch.getChangeType() method already returns exactly what we are > looking for: ChangeType { ADDED, MODIFIED, DELETED, RENAMED, COPIED }. It > should be straight forward to set that on the model. Yes, we should do that no question. My priority right now is getting the persistence stuff finished for RC1. If I can, I'll squeeze a quick change in for these before then. Basically, I'd like to get all of the model changes in at once as it is much simpler to bundle them up together. We don't have to do the UI side in the same change.
Since Sam's original suggestions are completed, I am now going to resolve this as closed, and I've made a new Task: bug 407836
