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. > > > 2) It’s kind of crazy that the review tool’s URL is > "...&action=prettypatch". It was nice of you to leave the old review tool > unchanged, at least in part to placate me, but you’re squatting on another > feature’s territory! I think we want a plain old formatted diff for other > purposes. We need to get your tool out of there! > > > > 3) I suggest you make your review tool the default at > "...&action=review" and move the old style review tool to another URL; we > can put a link in yours. > > Ok. I might need some help modifying bugzilla to make that happen, > but hopefully Julie Parent will be willing to point me in the right > direction. > Yup, this should be easy. See http://trac.webkit.org/changeset/59265 for an example of adding a new action and associated template. Feel free to ping me if you have any questions. > > I haven’t tried to use it on an iPad yet. > > The tool has some problems on iPad. The issue is the bottom toolbar > uses position: fixed, which seems to be frozen to the initial viewport > on iPad. When you scroll, the bar stays in the middle of the page. I > presume there's some way to fix this. It's on my list. > > On Thu, Sep 16, 2010 at 6:36 PM, Alexey Proskuryakov <[email protected]> > wrote: > > It's only now that I realized there's a new review tool at > action=prettypatch :-) > > Hopefully that means I didn't disrupt your use of action=prettypatch. :) > > > 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. > > > One thing I'd love to see added is a back-link to a bug. I find myself > using that fairly often currently. > > A couple other folks requested this as well. The complication here is > that navigating away from the tool will lose your in-progress edits. > My plan was to implement auto-save using localStorage first so that > the tool can restore your comments when you return. > > On Thu, Sep 16, 2010 at 6:48 PM, Maciej Stachowiak <[email protected]> wrote: > > I do really like the layout of the new page. Seems like it will be really > > good for reviews. > > 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. > > > (Maybe the "Publish" button should be labeled "Preview" to > > reduce needless nervousness.) > > Will do. > > 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. > > Adam > _______________________________________________ > 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
