Page MenuHomePhabricator

Break long words in differential two-up view
ClosedPublic

Authored by sophiebits on Apr 3 2014, 7:30 AM.

Details

Reviewers
epriestley
chad
Group Reviewers
Blessed Reviewers
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP9fedd343eb18: Break long words in differential two-up view
Summary

This should prevent long lines from making the code width different between files, which can be annoying. (And of course, it stops long lines from making a giant scrollbar too.)

Test Plan

Loaded this diff in Chrome, Firefox, IE9, and IE8:

(That's a screenshot from Chrome, but it looks about the same in the other browsers.)

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sophiebits updated this revision to Diff 20597.Apr 3 2014, 7:30 AM
sophiebits retitled this revision from to Break long words in differential two-up view.
sophiebits updated this object.
sophiebits edited the test plan for this revision. (Show Details)
sophiebits added reviewers: chad, epriestley.
sophiebits added a task: Restricted Maniphest Task.

Old appearance:

sophiebits updated this revision to Diff 20599.Apr 3 2014, 7:55 AM
sophiebits edited edge metadata.

Switch to unicode 'word joiner' (essentially a non-breaking zero-width space) so there's no extra line break when a line starts a really long word. Now looks like:

{F137518}

Note that the line number in beta.txt lines up with the line. (Tested copying in Chrome, Firefox, IE9. IE8 already didn't work.)

chad edited edge metadata.Apr 3 2014, 3:01 PM

What does this do on iOS/mobile?

About the same as before, but with line breaking. Here's an example that has some red.

When you scroll over, it looks reasonable too:

chad added a comment.Apr 3 2014, 4:00 PM

i'd defer to @epriestley, not sure how purposeful it is to not break words - it would be my guess someone will dislike this behaviour change - but I might be wrong, haha.

Well, this only changes the behavior in the case of a single word that's wider than the diff window; if it can avoid breaking a word by wrapping it still will. Does your text editor not do the same? I just tested in vim, emacs, TextMate, and Atom and all of them seem to break long words when necessary.

epriestley requested changes to this revision.Apr 3 2014, 4:18 PM
epriestley edited edge metadata.

One technical issue in two places:

  • PhabricatorSourceCodeView also uses phabricator-oncopy, but it looks like it wasn't updated, so I'd expect this to break copying text in Paste -- switch the ZWS in PhabricatorSourceCodeView to Word Joiner.
  • Likewise, DiffusionBrowseFileController has a copy of this character too. Update that one.

A couple minor things inline.

Also, some browsers (Chromium only?) have unusual behavior for <zero width space> <tab literal> (see https://secure.phabricator.com/T2495#26). This might interact in those cases, but hopefully by accidentally fixing the issue.

src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php
359

Can we safely remove any of these column classes now?

441–444

Do images still display OK?

This revision now requires changes to proceed.Apr 3 2014, 4:18 PM

I can't reproduce the tab problem in Chrome beta on OS X and I don't have Chromium on Ubuntu handy, but hopefully nothing will break terribly there.

src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php
359

Yes! I meant to and forgot.

441–444

Oops, they're a little funky. I'll fix.

sophiebits updated this revision to Diff 20607.Apr 3 2014, 4:36 PM
sophiebits edited edge metadata.

Fix images, remove extraneous classes, fix copying out of pastes

epriestley accepted this revision.Apr 3 2014, 4:39 PM
epriestley edited edge metadata.

Thanks! Based on the expert strategy of "poking around for 20 seconds", I don't see this causing any display issues. This is pretty easy to adjust later if we missed something, too.

This revision is now accepted and ready to land.Apr 3 2014, 4:39 PM
epriestley closed this revision.Apr 3 2014, 4:40 PM
epriestley updated this revision to Diff 20608.

Closed by commit rP9fedd343eb18 (authored by @spicyj, committed by @epriestley).

I think this mucked up generated changes a little bit -- I get this in Safari if I click to show generated changes on a diff like D8646, for the file __phutil_library_map__.php. I can look at this when I get a chance, but you can repro locally by putting @generated in any file and diffing it if you get there first.