Enable differential diff view to span entire width of the screen (and not be fixed width)

Closed, DuplicatePublic

Actions

This has the subtle side effect of 80+ column code looking good. Do you have any thoughts or plans here? Any cool lore I should know? :D I was thinking it could be neat to have that be a project / repository preference that gets enforced from lint to differential to diffusion.

A few pieces of context:

• 80+ Column Code
  • Today, we have filename-based width configuration options, so you can set all .java files to 120, etc. I think we ship with Java at 100 or 120. See the differential.wordwrap config directive. Differential and (I think) Diffusion respect this. So you can already do this, sort of, by setting '/.*/' => 9999999 in that map. However, this will have some other effects (I think other elements on the page may get too wide? And we'll probably force some stuff to enormous widths. See "Layout" below.) and will affect everyone and every language.
  • We could make this per-project instead of per-install, although the utility of having Erlang in project A wrap at 91 chars and Erlang in project B wrap at 97 chars is somewhat deabatable. This is probably inevitable in some installs, although you can probably fake your way through it in many cases by anchoring the regexps (e.g., @^/flib/.*\.erl@ will match Erlang only in flib/). As far as I know, not having this setting be more detailed than per-install hasn't been an issue.
  • Another possible approach is making it a view preference (see also T832), where you'd have a dropdown or option to "View unwrapped...". The downside to doing this sort of thing is that it's annoying if it's the common case, and we'll miss caches. In T832, it's more clearcut because it's obviously an uncommon case (some random file we don't guess the language of correctly), but it's less clearcut here.
• Layout
  • This task proposes making diffs "not fixed width". This is simple (delete code forcing them to specific widths) but looks awful because the left side of each diff varies. For example, you modify "A" and "B", but the longest modified line in A is only 80 characters long, while the longest modified line in B is 240 characters. This makes the right-side-of-the-diff column of line numbers start a completely different places horizontally, and obviously makes the right edge end at different places. The result is that the diff is hard to scan through (since everything is kind of all over the place) and hard to interact with (the inline comment gutter is all over the place, and the "View Options" are all over the place) and it just looks nasty. We probably want to do something more akin to forcing a min-width but not wrapping or enforcing a max-width?
  • In some cases, unwrapped code creates displays that are completely unusable. Consider cases like JS packages, where the unwrapped width is often 100,000+ characters. Notably, it becomes difficult to get to the "View Options" dropdown in this case, and if you want to leave an inline comment the gutter is somewhere in a sea of horizontal scrolling.
  • Wrapping naturally at display width without an indicator is potentially pretty bad, and can make incorrect code look correct and vice versa, especially in whitespace-sensitive languages like Python. So even if we had free-wrapping, we'd probably want to mark it visually. I'm not sure if we can do this with pure CSS in a reasonable number of browsers, or if this means a bunch of client-side JS garbage.
  • In D2062, I made the "heads up view" element (the thing at the top with the table of properties" free-width, but @vrana suggested that we should generally be limiting the width of the elements on these pages to keep text readable (i.e., the line widths of free text should be relatively short for optimal readability, and there are 92 million studies backing this up), which I mostly agree with. In theory, it's completely right; in practice, I haven't had too much difficulty with the free-width interfaces in Maniphest and Diffusion so I haven't jumped on making this change, but we probably should clamp all three tools to fixed width for comments/info elements so text has roughly book-like characters-per-line. At the very least, the current approach is awkward and inconsistent.
• Alternate Rendering
  • It would be good to split the "rendering" part out of DifferentialChangesetParser, partly to make that class less unwieldy and partly to allow us to use different renderers. Particularly: an HTML email renderer for sending HTML email, a one-column renderer for mobile, a completely-plain-text-machine-consumable renderer for writing unit tests, and possibly a three- or N-column renderer for rebases.
  • In a one-column layout, we have more freedom from some of these constraints. The reaction when we moved from one-column to two-column layouts on the web was universal approval, though, and a common criticism I hear of GitHub's tools is the lack of two-column layouts, so I would oppose defaulting to one-column. I think implementing it for mobile and making it optional on the web makes sense, though.

I'm pretty sure I sneezed earlier today and Herald added @eadler as a subscriber.