On Thu, Sep 16, 2010 at 11:58 PM, Ojan Vafai <[email protected]> wrote: > On Fri, Sep 17, 2010 at 4:39 PM, Adam Barth <[email protected]> wrote: >> >> On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler <[email protected]> wrote: >> > 1) I am happy with the review tool. I have been using it for a lot of >> > reviews. There may be no one left who prefers the old review page. >> >> Thanks. Please let me know if you have ideas for how to improve the >> tool. One thing Ojan suggested was the ability to expand the context. >> That would be tricky to implement, but it might be possible now that >> http://svn.webkit.org/repository/webkit/ has CORS enabled. > > I also want the option to see side-by-side diffs. There are some patches > where side-by-side is immensely easier to make sense of.
+1 Great work so far, though; this is so much nicer already. >> > Is there a way to preview comments before publishing? I'm a little >> > hesitant to push that button without seeing what will happen. >> As mentioned above, the "publish" button actually brings up a >> confirmation screen. My original plan was to eventually remove the >> confirmation screen, since it's fully redundant, but I can leave it if >> folks find it useful. > > I prefer avoiding the confirmation screen and instead having a preview > button. It's only confusing the first or second time you use the tool. > Whereas needing to do two clicks quickly becomes annoying. >> >> One suggestion I've received is to put the "overall comments" box on >> the toolbar so that you can accumulate overall comments as you read >> through the patch. My feeling is that might make the toolbar too >> tall, but I'd welcome other thoughts on that topic. > > This also was my suggestion. I think this would work as long as there is a > button or something to collapse the overall comments box. > >> >> > (Maybe the "Publish" button should be labeled "Preview" to >> > reduce needless nervousness.) >> >> Will do. > > Two buttons at the same time! > >> >> On Thu, Sep 16, 2010 at 7:46 PM, Ryosuke Niwa <[email protected]> wrote: >> > Yeah I often use that to get a part of patch that I posted on bugzilla. >> > e.g. I post some work in progress in bugzilla but decide to change my >> > approach. But I can still make a use of some changes in my original >> > patch, >> > so I just copy & paste from pretty diff and then remove line numbers on >> > TextMate and paste it on XCode. (Let me know if there's a better way of >> > doing this sort of stuff). >> >> The root problem here is the way the PrettyPatch DOM is structured the >> line element contains both the code and the line number. If all the >> code was in one container and all the line numbers in another >> container, you could copy the code without copying the line numbers. > > I'd love it if we changed this. > Ojan > _______________________________________________ > webkit-dev mailing list > [email protected] > http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev > > _______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
