On Aug 29, 2010, at 4:39 PM, Adam Barth wrote: > On Sun, Aug 29, 2010 at 4:16 PM, Maciej Stachowiak <m...@apple.com> wrote: >> I'm not sure who objects to new features being added to Review Patch, but I >> don't like this change: > > There's a tention between folks who like line-by-line comments and > (mostly) Darin, who likes the old Review Patch tool. Darin likes the > giant textbox with the whole diff. I don't really understand, but I > certainly have no wish to impede his use of the tools. > >> 1) I'm used to having the "click to add a comment" feature in Review Patch, >> and would miss it if it was gone. >> 2) I think overloading "Formatted Diff" is wrong - it should remain a >> read-only view. >> >> I think the main remaining problems with the Review Patch page are the >> inability to give comments with multiple lines of context, and the excessive >> amount of space dedicated to things that are not the patch. > > The other problem is that reviews get hard to follow really fast when > folks reply to review comments and the existing line-by-line editing > requires about twice as many clicks as it needs. I don't have mockups > to show, but what I had in mind was the following: > > 1) Use the whole page for the diff (basically remove the bottom half > of the "Review Patch" screen). > 2) When you're done writing line-by-line comments, show a lightbox > that has the content that used to be at the bottom of the screen. > That gives you a chance to enter high-level comments, read over your > comments, and adjust the various flags. > 3) The tools will then store the review in a comment, as before, which > generates an email to the author of the patch. > 4) The patch author can either read your review as a bugzilla comment, > or they can look at the patch and the tool will show your comments > inline in the diff where you wrote them. The author can respond by > adding more comments inline, which again are stored as bugzilla > comments, etc. > > In this approach, you can also imagine integration with the style > checker and the EWS bots. The tools can show the style errors inline > in the diff, as well as the compiler failures. The view is more that > the diff is a "living document" with a bunch of layers (some of which > are editable).
That sounds like a good design to me. But it doesn't sound like a replacement for a read-only view of just the diff. If you add the ability to show and hide the lightbox at any time, it might even be a reasonable replacement even for people who like to type their review comments by hand. > >> If changing the Review Patch page as needed would be too disruptive for some >> reason, I suggest using a new page instead of overloading "Formatted Diff". > > We can certain add these features under a new name, if you think that > would be the least disruptive. Sam already emailed me privately > explaining how I broke one of his uses of the Formatted Diff, but I > think I can fix that fairly easily by learning slightly more jQuery. > > I'm happy to build this off in a silo and then convince everyone how > awesome it is once it actually is more awesome than the current tools. I suggest you start by making it a new page, and then we can decide whether to remove any of the existing pages in favor of the new one. Regards, Maciej _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
