Welcome to the Splat alpha!

• Tags:

  +	Component-DiffViewer

I would be even MORE in love if that '80' columns was settable by either the review
author or by reviewer. In the latter case, I guess it would be just a local setting.

Would also be very nice if there was a default and per-user 'force wrap' setting at
which point the diff-viewer would wrap the line- ie I'd prefer to have an inserted
forced wrap rather than having to scroll the browser window.

• Status:

  +	Confirmed

Another idea is to cut off lines longer than some length (say 90) with an ellipsis. 
Then show a simple tooltip for the rest of the line when the user hovers over the '...'.

Looking at wrapped code can be a pain too.

Maybe the background color could be different for characters after the boundary.
Example:
<div class="line"><span class="first-80">first 80 chars...</span>trailing chars...</div>
<div class="line"><span class="first-80">only 50 chars here...(30 chars
spaces)</span></div>

div.line {background-color: blue}
div.line .first-80 {background-color: white}

I'd love to have this.  This issue is quite old - what is the chances it will be
implemented?

It would need to be done in such a way where it's 1) fast, 2) customizable without
regenerating the HTML, and 3) user-configurable.

Not everyone at a company will necessarily have the same standards, so the user would
need to be able to change this. It'd have to dynamically update the HTML. This is the
slow, complicated part. Due to syntax highlighting, the text is broken up into many
text fragments surrounded by tags. So knowing the correct place is a bit tricky.

We'd be able to hack it by adding a vertical line or something, except then
word-wrapping is an issue.

I'm all for adding this feature, but someone needs to find a good way to do this and
implement it. It's not on my todo list though.

If anyone comes to implement something, I would like it be changeable to 120 (we have 
widescreen monitors now) :) 

For me, I think just putting a div line on top would be sufficient -- even if lines 
are "wrapped" (essentially ignore the wrapped portion, and do not show the line if 
the wrapping occurs "before" the 80 column mark).

Would that work for others?

Implementation: This could be done via javascript if the option is available. The 
javascript could decide to show the line or not "on-the-fly" (depending if that 
column is available without the wrapping (measure the size of "80 columns" in pixels 
with a "hidden div" with 80 letters of the same monospaced font).

That way, the main document could stay unchanged if just the user settings is 
available somehow (external .js or additional ajax call).

Hi,

We also follow certain standards like this in our company. And we have been using review board for quite some time now.

Can the review board show margin after 80th column, or allow the admin to configure
the limit based on the file extension. It could be a thin vertical line, or a slightly different background color.

And/or, lines violating the limit could be highlighted.

Any idea of this would be implemented?

=Arun

Arun, this is one of those situations where an easy solution for one group is going to cause complaints for another. In large deployments, different teams may very well have different coding standards, and because of this we need a solution that is a bit more flexible.

The trick is to figure out *where* the 80th column is. We need something that is fast but flexible. Doing it server-side is possible and may be speedy enough, but will become part of the cached content, so depending on how we let the users customize it (whether we choose to make it an admin setting or something flexible the user can change) it may or may not be an option. If doing it client-side, we need a way to quickly find the 80th column on each row, which can be slow.

We also need to figure out the UI for showing that column. Background color could work. A thin line is a bit more difficult and looks odd on wrapped content.

If it is decided that this should be a user-level setting, why not cache the content with that setting as part of the cache key? So yes, that means a bigger cache, if there are a lot of variant of that setting and everybody is looking at everything. 

Some companies use other number of columns (ie: 100) so this should be configurable also

> The trick is to figure out *where* the 80th column is [...]

For fixed-width fonts, this determination should be easy, shouldn't it? Style a vertical line at the appropriate horizontal offset, with the css being parameterized by the user preference?

One way I did something similar was to create a hidden div with 80 letters that had the same CSS styles and measure the width. I know some JS toolkits have APIs that can perform this measurement. (I only know ExtJS)

I'm very interested in this feature, or some way of turning off line wrapping / configure wrap margin. Would be a very useful feature for me and my team.

Speaking of cached content: This is often a project or company-wide setting, so maybe this could be a global setting (turn on, turn off, column size) would work for most cases and you can benefit from computing this server-side with the same content caching as normal.

I'm not sure user customization makes sense (or at least, is as important as it is being considered). If code length is part of the review process, 'what length' is going to be a property of the project, not the reviewer. Ergo, the logical place for it to be configurable is as a repository attribute.

p.s. if you can rely on a fixed-width font, the CSS position of the margin can simply be specified in em units. If your DOM/CSS is sufficiently clever you can probably offload all the number crunching to the browser's render engine. (For that matter, if you *can't* rely on a fixed-width font, a margin is going to be ugly anyway, so...) An absolute position fixed-width div beneath the text but within the column element is probably sufficient.

Mike has some CSS that works for this:

table.sidebyside tr[line] > td:before {
  content: '';
  position: absolute;
  transform: translateX(80ch);
  border-left: 1px solid red;
  height: 18px;
}

We'll have to figure out the best way to expose this.

A few notes on the CSS, after playing with it, that we'll need to address:

1. The red lines end up overflowing from the left-hand side to the right-hand side. This can be sort of prevented by adding 'position: relative; overflow: hidden;' to the parent td, but I'm not sure that fully worked.

2. If your window width isn't wide enough to show two columns of 80 characters, you'll never see red lines and never have any indication where the wrap point is.

3. Similarly, if text wraps due to being too long to show the full block of text (such as a long variable name) on the first line, and that text hits the 80 column boundary, then it'll wrap and you won't see where the red line was supposed to be within that text.

+1 to this request. Would be great to let a company say that a given extension triggers a particular margin, since groups may vary within a company, but usually a given language has the same style throughout a company.
I’d also be happy with no configurability, but a faint line at 80, 100, 120, etc. A reviewer can notice for themselves whether code crosses whichever margin applies.