Page MenuHomePhabricator
Create Task
Maniphest T9900

Figlet does not properly render apostrophes with some characters
Open, WishlistPublic
Actions

Assigned To
None
Authored By
cspeckmim
Dec 4 2015, 10:09 AM
Tags
Referenced Files
F4290906: HoYp1cA.gif
Mar 27 2017, 4:36 PM
F1840788: pasted_file
Sep 20 2016, 5:08 PM
F1840795: pasted_file
Sep 20 2016, 5:08 PM
F1840798: pasted_file
Sep 20 2016, 5:08 PM
F1840784: pasted_file
Sep 20 2016, 5:08 PM
F1840782: pasted_file
Sep 20 2016, 5:08 PM
F1840790: pasted_file
Sep 20 2016, 5:08 PM
F1014024: figlet-test.png
Dec 4 2015, 11:02 PM
Subscribers
chad
cspeckmim
eadler
epriestley
jcox
lpriestley
tycho.tatitscheff
Tokens
"Baby Tequila" token, awarded by jcox."Cup of Joe" token, awarded by epriestley.

Description

Examine this comment:
https://secure.phabricator.com/T9360#143554
At first it appears the comment is dancing, livened by @chad's singing.

It looks like when the apostrophe immediately proceeds an 'm', 'o' or is immediately proceeded by an 's', 'm', or 'o'. There might be other characters it doesn't go well with. I haven't checked capital letters.

___ _ _ _ _ _ |_ _( ) __| | | |__ ___ ___( ) | __ ___ ___ _ __ ___ | ||/ / _` | | '_ \ / _ \ / _ \/| |/ / / _ \ / _ \| '_ \/ __| | | | (_| | | |_) | __/ | (_) | | < | (_) | (_) | |_) \__ \ |___| \__,_| |_.__/ \___| \___/ |_|\_\ \___/ \___/| .__/|___/ |_|
___ _ _ |_ _( ) __ ___ ___ | | __ | ||/ '_ ` _ \ / _ \| |/ / | | | | | | | | | (_) | < |___| |_| |_| |_| \___/|_|\_\
___ _ _ _ _ __ ___ |_ _( ) (_)___ ___ | | __ | '_ ` _ \ | ||/ | / __| / _ \| |/ / | | | | | || | | \__ \ | (_) | < |_| |_| |_|___| |_|___/ \___/|_|\_\
_ _ _ _ ( )__| | ___)_ __ _ __ (_)_ __ __ _ |/ __| |/ / | '_ \| '_ \| | '_ \ / _` | \__ \ <| | |_) | |_) | | | | | (_| | |___/_|\_\_| .__/| .__/|_|_| |_|\__, | |_| |_| |___/
_ _ _ _ ( ) __ ___ (_)___| |_ ___ _ __(_) |/ '_ ` _ \| / __| __/ _ \ '__| | | | | | | | \__ \ |_ __/ | | | |_| |_| |_|_|___/\__\___|_| |_|
_ _ _ _ _ _ _ _ ( ) _ __ ___ (_)___| |_ ___ _ __(_) __ __ ) ___ _ __ ___| |_ ___ _ __ | |__( ) ___ ___ |/ | '_ ` _ \| / __| __/ _ \ '__| | \ \ /\ / // / _ \| '_ \/ __| __/ _ \ '__| | '_ \/ / _ \ / _ \ | | | | | | \__ \ |_ __/ | | | \ V V / | (_) | | | \__ \ |_ __/ | | |_) | | (_) | (_) | |_| |_| |_|_|___/\__\___|_| |_| \_/\_/ \___/|_| |_|___/\__\___|_| |_.__/ \___/ \___/

Related Objects

Event Timeline

cspeckmim created this task.Dec 4 2015, 10:09 AM
cspeckmim updated the task description. (Show Details)
cspeckmim added subscribers: cspeckmim, chad.
epriestley added a subscriber: epriestley.Dec 4 2015, 12:36 PM

I think this is a problem only with the default font, it seems OK in other fonts:

_ _ _ _ __ ___( ) | __ _ __| |_ _ | '_ ` _ \/| |/ _` |/ _` | | | | | | | | | | | | (_| | (_| | |_| | |_| |_| |_| |_|\__,_|\__,_|\__, | |___/
### # # ### # ## ##### # # ## ## # # # # # # # # # ## # # # # # # # # # # # ###### # # # # # # # # # # # # # ###### # # ##### #
o _ /| | | _ _ _ | | __, __| / |/ |/ | |/ / | / | | | | | |_/ |__/\_/|_/\_/|_/ \_/|/ /| \|
+-+-+-+-+-+-+ |m|'|l|a|d|y| +-+-+-+-+-+-+

This could be:

  1. a bug with the font itself; or
  2. a bug with the PHP figlet library's decoding and rendering of the font.

We could distinguish between these by using the same font file with the real figlet binary. If rendering there is correct, that implicates the PHP figlet library. If rendering there is incorrect, that implicates the font. (I think (2) is much more likely.)

If this is a font problem, we could remove the font and choose a different default. If this is a library problem, we could try to fix the library. In either case, we could work around the problem by choosing a different default; this might be much easier than fixing the library.

To move forward:

  • First, we should test rendering of the default font (which should be externals/figlet/fonts/standard.flf) with the normal figlet binary to implicate the font file or the PHP figlet renderer.
  • If the renderer is at fault, we should ideally try to fix the renderer. It's only ~500 lines long with ~300 lines of real logic, and there's a C implementation to compare it to, so maybe this isn't too tricky. However, the C implementation looks like it's more in the ballpark of ~2,500 lines of real figlet logic, so it's possible that the problem with the PHP library is that it's missing 2000 lines of complex layout code.
  • If we can't fix the renderer, we could just pick a different default font.
epriestley triaged this task as Wishlist priority.Dec 4 2015, 12:37 PM
epriestley added a project: Remarkup.
epriestley renamed this task from Figlet does not properly render apostrophe's with some characters to Figlet doe's not properly render apostrophe's with some character's.Dec 4 2015, 1:03 PM
cspeckmim added a comment.Dec 4 2015, 11:02 PM

From a first glance it looks like the renderer may be the issue (tested the font in phabricator with figlet installed via homebrew):

figlet-test.png (740×1 px, 375 KB)

epriestley added a subscriber: lpriestley.Dec 16 2015, 11:52 PM
epriestley added a project: Contributor Onboarding.Aug 22 2016, 6:21 PM
Herald added a subscriber: eadler. · View Herald TranscriptAug 22 2016, 6:21 PM
epriestley assigned this task to jcox.Aug 22 2016, 8:07 PM

PhabricatorRemarkupFigletBlockInterpreter is a remarkup rule for rendering figlet fonts, invoked like this:

figlet {{{ The quick brown fox... }}}

This produces:

_____ _ _ _ _ __ |_ _| |__ ___ __ _ _ _(_) ___| | __ | |__ _ __ _____ ___ __ / _| _____ __ | | | '_ \ / _ \ / _` | | | | |/ __| |/ / | '_ \| '__/ _ \ \ /\ / / '_ \ | |_ / _ \ \/ / | | | | | | __/ | (_| | |_| | | (__| < | |_) | | | (_) \ V V /| | | | | _| (_) > < _ _ _ |_| |_| |_|\___| \__, |\__,_|_|\___|_|\_\ |_.__/|_| \___/ \_/\_/ |_| |_| |_| \___/_/\_(_)_)_) |_|

A while ago we implemented this by just running figlet, but it turns out that this is pretty hard to make safe (see T9408), so we swapped it to a PHP implementation (in externals/pear-figlet/). However, this implementation appears to have a bug with rendering apostrophes:

figlet {{{ C'thun }}}

...produces:

____ _ _ _ / ___( ) |_| |__ _ _ _ __ | | |/| __| '_ \| | | | '_ \ | |___ | |_| | | | |_| | | | | \____| \__|_| |_|\__,_|_| |_|

Uhhh, that one works fine.

_ _ _ | | | ( ) __ ___ _ __ ___ _ __ ___ | | | |/ '_ ` _ \| '_ ` _ \| '_ ` _ \ | |_| | | | | | | | | | | | | | | | | | \___/ |_| |_| |_|_| |_| |_|_| |_| |_|

Okay, that one's kinda broken. See original description for some more examples.

Either:

  • Spend a little time trying to figure this out (or trying to figure out how involved figuring this out is, or whatever else). I haven't made an attempt at this so it may be unreasonably difficult, although the total size of the implementation is not particularly large so it's conceivable that the fix is fairly easy.
  • Or, give up and close this task as not worth fixing.

If we give up, we may want to change the default $font in PhabricatorRemarkupFigletBlockInterpreter to a different one. We currently use standard, but could pick anything from externals/figlet/fonts/.

epriestley moved this task from Backlog to Intermediate on the Contributor Onboarding board.Aug 22 2016, 8:07 PM
jcox added a comment.Sep 20 2016, 5:08 PM

Alright I've been able to narrow this down a little bit more. There's some error with how external/pear-figlet/Text/Figlet.php is smushing characters. Specifically lines 339-362 as well as the _rep($r) function. The equivalent function in the C code is here. I haven't been able to find an obvious discrepancy, but I suspect there's a subtle difference in how one of the regex functions is handling things or an error in one of the regexes themselves. This error also occurs with backticks and double-quotes:

pear-figletfigletnotes
pasted_file (89×154 px, 2 KB)
pasted_file (99×158 px, 2 KB)
pear-figlet smushes "/I" into just / and replaces )_ with just )
pasted_file (95×172 px, 2 KB)
pasted_file (106×166 px, 2 KB)
The _( gets smushed into just _. Also starting on line 4, it seems like pear-figlet starts smushing the whitespace below the quotes
pasted_file (95×163 px, 1 KB)
pasted_file (97×165 px, 2 KB)
Both of the issues that the above two have combined into one. Pretty much, I don't think pear-figlet is capable of rendering _( or )_ (as well as other combinations). It needs to do that sometimes though.

Also notable, b'<ANY CHARACTER> always renders messed up. There are some other letters like that as well.

Anyway I think this is a problem that requires beer so I'm going to look more into it this weekend.

Herald added a subscriber: tycho.tatitscheff. · View Herald TranscriptSep 20 2016, 5:08 PM
epriestley awarded a token.Sep 20 2016, 5:22 PM
jcox awarded a token.Sep 20 2016, 5:28 PM
chad renamed this task from Figlet doe's not properly render apostrophe's with some character's to Figlet does not properly render apostrophe's with some character's.Mar 27 2017, 3:56 PM
chad moved this task from Backlog to Tomfoolery on the Remarkup board.Mar 27 2017, 4:02 PM
jcox removed jcox as the assignee of this task.Mar 27 2017, 4:36 PM
jcox added a subscriber: jcox.

HoYp1cA.gif (375×500 px, 220 KB)

CodeMouse92 renamed this task from Figlet does not properly render apostrophe's with some character's to Figlet does not properly render apostrophes with some characters.Mar 27 2017, 4:39 PM
epriestley added a comment.Mar 27 2017, 4:40 PM

THANKS FOR RUINING THE JOKE EVERYONE

cspeckmim added a comment.Mar 27 2017, 4:44 PM

is srs bsness mode taking over? i'm in a panic :[

epriestley mentioned this in T12681: Upcoming Support Product / End of Paid Prioritization.May 5 2017, 10:16 PM
Phabricator · Built by Phacility