401: Comments should be inserted into diff below lines being discussed, instead of in dialog box
- Fixed
- Review Board
| adam.*****@gmai***** (Google Code) (Is this you? Claim this profile.) | |
 |
|
| Dec. 27, 2008 |
I find the "Comments" dialog box very hard to work with because it obscures the diff. It appears right over the line(s) you clicked on, so the code you're writing about can't be seen. And it's usually bigger than it needs to be, obscuring more code than is necessary. I think this could be fixed by having the comment entry field be inserted into the diff itself just below the line(s) you're commenting on. The interaction would be like this: 1. Click or drag-click on the lines you want to comment on. 2. The portion of the diff just below the lines you selected slides down, revealing a 1- or 2-line text entry field that is the width of the entire diff area. This would let you see the full context of your comment while writing it. I've attached a screenshot of a mockup of how this could look. I think that the "Save Comment" and "Delete Comment" buttons wouldn't be needed at all -- any comments entered would be automatically saved when the review is submitted. The "Cancel" button could be retained (perhaps it could sit in the upper-right corner of each comment entry field. A simple "X" button would probably be sufficient, and would take up much less space).
I've considered this method when I first started, and we might revisit it for a future version, but I have no plans to change to that interface now. It doesn't work for either screenshot comments or some of the stuff I have planned for 2.0. If we moved to that UI, we'd still need the dialog for the screenshot comments and new 2.0 features, and it's not so good having to have 2 UIs for this. It also requires that you move to the top of the page and click Review in order to get to the review portion of the dialog, and that's a pain.
-
- Type-Defect - Priority-Medium + Type-Enhancement + Priority-Low + Component-Reviews
I agree with Adam - the review of source should be using a different UI to the review of images such as screenshots. Obviously when adding review comments to a screenshot the comments cannot be shown inline, but it would make it easier to use if the UI for source did allow that capability. Perhaps you could make this configurable in case some users of RB want to add comments using the popup, and allow other users to use the inline comments ?
Making the UIs configurable makes it a pain to maintain, document and support. After careful consideration, I've decided not to go the inline route, as there's a number of problems associated with it: 1) If you're reviewing a large number of lines, you have no way of seeing both the text entry and the diff lines at the same time. 2) There's no good way of indicating which lines the comment is associated with. The comment flags do a better job of this, and will do an even better job once they show line ranges (soon). 3) People don't always want to just see the comments appear. We'd need a good way of letting users show/hide all these, and that's even more unclear. Where do you go for this? Top of the page? Have a floating window somewhere? It just seems to me that it'll get in the way at times. 4) It'll appear even more like you're supposed to reply to comments by attaching a new comment to a line. This is a problem we have today, where people try to use the diff viewer to reply to reviews, which produces unexpected results. We don't want to encourage this further. What I am doing is revising the comment dialog to be smaller, appear below the comment range at a better default position, and to make it a lot more clear how to reply to existing comments and how to create new comments. I'm hijacking this bug for the new design and will be closing when the change goes in.
-
+ Started -
+ Milestone-Release1.0 -
+ chipx86
Thanks for thinking about this. I don't see how using a popup window fixes (1). If you try to arrange the popup window to be below the lines you're commenting on, then you're in the same situation you'd be in with my proposed change. If you place the popup window on top of the lines you're commenting on, then clearly you're blocking the diff from view (which is the current problem). Why should the indicators for (2) look any different for a popup window vs. an inline comment? I'm not sure exactly what you mean by (3). Are you talking about the person creating the review? Or the person reading the review? I wasn't proposing changing the interface for reading a review, only the interface for creating a review. When you're creating a review, what comments would "just appear"? (4) seems to be explicitly talking about the interface for reading a review, which I'm not proposing to change.
The key difference with the popup window is that we get intelligently place it initially but give users the freedom to move it if they're reviewing a large block of code. I assumed in this design that all inline comments would just show up automatically, rather than having comment flags. That's been the design people have recommended to me in the past when talking about an inline comment field like this.
Implemented the new, more condensed, better-aligned comment dialog in r1644. I think it's a huge improvement. We can make incremental improvements over this down the road, but for now, this is what we're going with.
-
- Started + Fixed