Page MenuHomePhabricator

In revisions, replace "o" with "πŸ‘"
AbandonedPublic

Authored by epriestley on Feb 17 2019, 5:18 PM.

Details

Summary

Ref T12822. This is an objectively good change, although I'm not planning to pursue it to completion right now.

This roughly demonstrates how we can improve "\t", ZWS, etc., behavior in the future by showing one thing and copying something else: we can show something that's easy to read, and copy the authentic original text.

There are a few issues with this, and a real implementation would be a bit messier:

  • This puts the replacement text on the <td>, but you can select one single character and the <td> won't (I think?) be part of your selection. But if you just copy some whitespace or a single sheep or whatever, it's probably fine that your clipboard ends up with exactly what you copied -- it's probably only really important/valuable to preserve the original text when you're copying larger blocks.
  • You can copy half a line, and we'll currently put the entire line on your clipboard. This gets even trickier when the original and replacement text have different numbers of characters (e.g., "\t" -> multiple spaces).
  • When you copy part of a line, it might be at the beginning or end of your selection, so when we're putting part of a replacement text onto the clipboard, we need to figure out if we're supposed to take from the beginning or end of the replacement text.

Anyway, this is on the table in the future for dealing with these sorts of issues, although I don't plan to pursue it for now.

Test Plan

Copied "closed" out of a diff, got "clπŸ‘sed" on my clipboard.

Diff Detail

Repository
rP Phabricator
Branch
sorcery6x
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 22049
Build 30124: Run Core Tests
Build 30123: arc lint + arc unit

Event Timeline

epriestley created this revision.Feb 17 2019, 5:18 PM
epriestley requested review of this revision.Feb 17 2019, 5:19 PM
epriestley abandoned this revision.Feb 17 2019, 5:25 PM

Objectively great diff, but not planning to move forward with it for now.

I suppose we could resolve most of these issues simply and very surgically by putting <span></span> tags around exactly the replaced text, e.g. helic<span data-copy-text="πŸ‘">o</span>pter.

When we replace "\t" with " " you'd still get a "\t" if you select at least one of the spaces, but that seems perfectly fine, and everything else should basically "just work".

I suppose we could resolve most of these issues simply and very surgically by putting <span></span> tags around exactly the replaced text, e.g. helic<span data-copy-text="πŸ‘">o</span>pter.

This approach seems pretty reasonable: D20194.