Re: Review Board 1.7.6 and whitespace
That actually toggles the visibility of the big red trailing/mixed whitespace indicators (but not the lines). It's confusing and I'm really hoping to rewrite the UI for it for 1.8. Christian On Mar 26, 2013, at 4:36, Stephen Gallagher wrote: > On 03/25/2013 05:04 PM, Christian Hammond wrote: >> Hi Stephen, >> >> Whitespace for indentation has never been shown for most files, >> regardless of that setting. That's just due to how we've always >> generated diffs. >> >> During diff generation, indentation changes are ignored. The intent was >> to prevent a block of indentation changes from showing up as a bunch of >> modified lines or, worse, deleted/added lines, distracting from the >> content of those reviews. > > Sure, I understand that, but I would have figured that's what the "Hide > whitespace changes" button would have been for. As it stands right now, > those two buttons don't appear to do anything at all. > > >> >> All other whitespace changes are then let through (trailing whitespace, >> whitespace within the line), and lines containing just that are assigned >> a class. The "Show/Hide Extra Whitespace" toggle then just changes the >> visibility of any chunks containing these classes. It doesn't regenerate >> the diff or anything. The goal of this feature is to make code review >> easier when the file contains lots of whitespace cleanup, but not to >> show indentation. >> >> So, they're noticing this for the first time. It has never been any >> different, out of the box. > > Well, this instance has only been up for about a week, so "for the first > time" happened pretty quickly. > > >> This can be disabled in an installation, though. You can include *.py >> files in "Show all whitespace for:" list in the Diff Viewer Settings in >> the Admin UI, and we'll stop ignoring leading whitespace on lines. > > I couldn't get this to work for > https://reviewboard-openlmi.rhcloud.com/r/54/diff/ > > I added "*.py, openlmi-doc-class2*" to the ignore list in the Diff > Viewer settings (and restarted the server), but it still does not > display leading whitespace changes on line 124 of that diff. > > >> Can this all be better? Yes. What I'd love is to have the diff >> generation be much smarter and say "This line hasn't changed except for >> the indentation," and then mark that up differently. There's a lot of >> work that would be needed, though. Something to put on the increasingly >> large TODO list. > > The diff generation seems to be able to identify the specific characters > that have changed within a line other than whitespace, so I'm not sure > why we can't just use that same logic. But I'll admit to having no clear > view of the internal details here. > > -- > Want to help the Review Board project? Donate today at > http://www.reviewboard.org/donate/ > Happy user? Let us know at http://www.reviewboard.org/users/ > -~--~~~~--~~--~--~--- > To unsubscribe from this group, send email to > reviewboard+unsubscr...@googlegroups.com > For more options, visit this group at > http://groups.google.com/group/reviewboard?hl=en > --- > You received this message because you are subscribed to the Google Groups > "reviewboard" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to reviewboard+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > > -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- You received this message because you are subscribed to the Google Groups "reviewboard" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
Re: Review Board 1.7.6 and whitespace
On 03/25/2013 05:04 PM, Christian Hammond wrote: > Hi Stephen, > > Whitespace for indentation has never been shown for most files, > regardless of that setting. That's just due to how we've always > generated diffs. > > During diff generation, indentation changes are ignored. The intent was > to prevent a block of indentation changes from showing up as a bunch of > modified lines or, worse, deleted/added lines, distracting from the > content of those reviews. Sure, I understand that, but I would have figured that's what the "Hide whitespace changes" button would have been for. As it stands right now, those two buttons don't appear to do anything at all. > > All other whitespace changes are then let through (trailing whitespace, > whitespace within the line), and lines containing just that are assigned > a class. The "Show/Hide Extra Whitespace" toggle then just changes the > visibility of any chunks containing these classes. It doesn't regenerate > the diff or anything. The goal of this feature is to make code review > easier when the file contains lots of whitespace cleanup, but not to > show indentation. > > So, they're noticing this for the first time. It has never been any > different, out of the box. > Well, this instance has only been up for about a week, so "for the first time" happened pretty quickly. > This can be disabled in an installation, though. You can include *.py > files in "Show all whitespace for:" list in the Diff Viewer Settings in > the Admin UI, and we'll stop ignoring leading whitespace on lines. > I couldn't get this to work for https://reviewboard-openlmi.rhcloud.com/r/54/diff/ I added "*.py, openlmi-doc-class2*" to the ignore list in the Diff Viewer settings (and restarted the server), but it still does not display leading whitespace changes on line 124 of that diff. > Can this all be better? Yes. What I'd love is to have the diff > generation be much smarter and say "This line hasn't changed except for > the indentation," and then mark that up differently. There's a lot of > work that would be needed, though. Something to put on the increasingly > large TODO list. > The diff generation seems to be able to identify the specific characters that have changed within a line other than whitespace, so I'm not sure why we can't just use that same logic. But I'll admit to having no clear view of the internal details here. -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- You received this message because you are subscribed to the Google Groups "reviewboard" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
Re: Review Board 1.7.6 and whitespace
On Tuesday, March 26, 2013 2:34:25 AM UTC+5:30, Christian Hammond wrote: > > This can be disabled in an installation, though. You can include *.py > files in "Show all whitespace for:" list in the Diff Viewer Settings in the > Admin UI, and we'll stop ignoring leading whitespace on lines. > > I think it might be helpful if we setup the "Show whitespace for: " to default to *.py. Given Python's reliance on whitespaces, defaulting the value might make it easier for reviewboard users, who will skip this setting otherwise. Regards Raja -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- You received this message because you are subscribed to the Google Groups "reviewboard" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
Re: Review Board 1.7.6 and whitespace
Hi Stephen, Whitespace for indentation has never been shown for most files, regardless of that setting. That's just due to how we've always generated diffs. During diff generation, indentation changes are ignored. The intent was to prevent a block of indentation changes from showing up as a bunch of modified lines or, worse, deleted/added lines, distracting from the content of those reviews. All other whitespace changes are then let through (trailing whitespace, whitespace within the line), and lines containing just that are assigned a class. The "Show/Hide Extra Whitespace" toggle then just changes the visibility of any chunks containing these classes. It doesn't regenerate the diff or anything. The goal of this feature is to make code review easier when the file contains lots of whitespace cleanup, but not to show indentation. So, they're noticing this for the first time. It has never been any different, out of the box. This can be disabled in an installation, though. You can include *.py files in "Show all whitespace for:" list in the Diff Viewer Settings in the Admin UI, and we'll stop ignoring leading whitespace on lines. Can this all be better? Yes. What I'd love is to have the diff generation be much smarter and say "This line hasn't changed except for the indentation," and then mark that up differently. There's a lot of work that would be needed, though. Something to put on the increasingly large TODO list. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.reviewboard.org Beanbag, Inc. - http://www.beanbaginc.com On Mar 25, 2013, at 1:53 PM, Stephen Gallagher wrote: > The OpenLMI developers discovered a bug[1] today related to whitespace > display in the diff viewer. Specifically, whitespace changes are not > being shown, regardless of the setting of "Hide Extra Whitespace" or > "Hide Whitespace changes" toggles. > > This is a pretty serious regression, particularly when dealing with > reviews of Python code, which as I'm sure the Review Board developers > know relies heavily on accurate whitespace. > > Mostly I'm bringing this to the list to raise awareness of this bug in > the hopes that it will be fixed more quickly. > > > [1] http://code.google.com/p/reviewboard/issues/detail?id=2941 > > -- > Want to help the Review Board project? Donate today at > http://www.reviewboard.org/donate/ > Happy user? Let us know at http://www.reviewboard.org/users/ > -~--~~~~--~~--~--~--- > To unsubscribe from this group, send email to > reviewboard+unsubscr...@googlegroups.com > For more options, visit this group at > http://groups.google.com/group/reviewboard?hl=en > --- > You received this message because you are subscribed to the Google Groups > "reviewboard" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to reviewboard+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > > -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- You received this message because you are subscribed to the Google Groups "reviewboard" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
Review Board 1.7.6 and whitespace
The OpenLMI developers discovered a bug[1] today related to whitespace display in the diff viewer. Specifically, whitespace changes are not being shown, regardless of the setting of "Hide Extra Whitespace" or "Hide Whitespace changes" toggles. This is a pretty serious regression, particularly when dealing with reviews of Python code, which as I'm sure the Review Board developers know relies heavily on accurate whitespace. Mostly I'm bringing this to the list to raise awareness of this bug in the hopes that it will be fixed more quickly. [1] http://code.google.com/p/reviewboard/issues/detail?id=2941 -- Want to help the Review Board project? Donate today at http://www.reviewboard.org/donate/ Happy user? Let us know at http://www.reviewboard.org/users/ -~--~~~~--~~--~--~--- To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en --- You received this message because you are subscribed to the Google Groups "reviewboard" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
