CodeEditorComponent Windows, Linux broken 6.0.8 -> 6.1 including in Projucer and DemoRunner

[baconpaul]

Hi

On Linux and Windows, between 6.0.8 and 6.1, the CodeEditorComponent no longer respects start-of-line spaces in the document. It seems commit e1366361ed99bcf238327b91e7c19e1b12c60b52 broke this.

Here’s the Surge lua modulator in 6.0.8 on ubuntu 20.04

and here it is at 6.1

On Windows we see the same thing; on macOS we don’t.

The 6.1 version also has the problem that pressing space in the document at start of line doesn’t introduce spaces.

On macOS it is fine at both 6.0.8 and 6.1.

I’ll try and get closer to the commit that breaks it this weekend and will update here, but wanted to know if this rings any bells?

Oh and just linking this to our GitHub: Formula Editor: indents not displayed · Issue #4925 · surge-synthesizer/surge · GitHub

Thanks

[baconpaul]

Did a git bisect while drinking my morning coffee.

e1366361ed99bcf238327b91e7c19e1b12c60b52 is the first bad commit
commit e1366361ed99bcf238327b91e7c19e1b12c60b52
Author: ed <eddavies95@gmail.com>
Date:   Wed Jul 7 09:40:40 2021 +0100

    TextLayout:  Skip whitespace tokens in TokenList::createLayout() to fix misalignment when using horizontally centred justification

that’s the commit that broke it between 6.0.8 and 6.1 @ed95 FYI

will see if I can figure out why

[baconpaul]

Oh also, when you go mid line and press space, the space gets put in in the wrong spot (basically ignoring the stripped whitespace). So seems this commit has totally broken the CodeDocumentEditor on linux.

No idea why it only breaks linux and not mac. will dig some more this weekend unless someone shares a thought here.

[baconpaul]

This code path in this diff is called on linux but isn’t even called on macOS so that explains why this diff breaks linux but not macOS.

[baconpaul]

The problem is entirely in the choice to skip whitespace tokens in the glyph construction (which matches what we see of course). This diff “fixes” the problem but I think it unfixes whatever the intent of the diff was

diff --git a/modules/juce_graphics/fonts/juce_TextLayout.cpp b/modules/juce_graphics/fonts/juce_TextLayout.cpp
index 70547a564..74f659ad3 100644
--- a/modules/juce_graphics/fonts/juce_TextLayout.cpp
+++ b/modules/juce_graphics/fonts/juce_TextLayout.cpp
@@ -354,7 +354,7 @@ namespace TextLayoutHelpers
 
                 if (newGlyphs.size() > 0)
                 {
-                    if (! t.isWhitespace && ! t.isNewLine)
+                    //if (! t.isWhitespace && ! t.isNewLine)^M
                     {
                         currentRun->glyphs.ensureStorageAllocated (currentRun->glyphs.size() + newGlyphs.size());
                         auto tokenOrigin = t.area.getPosition().translated (0, t.font.getAscent());

[baconpaul]

Note that code in the DemoRunner displays the same problem.

[baconpaul]

Here’s how code editor renders a C++ file in projucer

and with patch above applied

[baconpaul]

Fyi i forked juce and am now running surge on 6.1.0 with the above commit reverted and our Linux and windows users report things work. Will undo the fork when the issue is resolved but no urgency for our dev cycle. Thanks and let me know if you need anything else to reproduce!

[eyalamir]

Bumping this post so it doesn’t get lost.

[ed95]

Thanks for the report. We’ve reverted the problematic commit in 2be72f6. This is on develop now and there will be a minor bugfix release in the next couple of weeks so it’ll make it’s way onto the master branch soon.

[baconpaul]

Wonderful thanks

[kamedin]

This effectively unfixes the issue with centred text though: tooltips are off-centre again. As there’s currently no way to get the size of a layout without trailing whitespace, I’ll resort to using GlyphArrangement with a fixed width.

[ed95]

Yes, I’m looking into a more permanent solution for that issue but for the time being we need to revert the change.

[kamedin]

No problem, I just mentioned in case it went unnoticed. I’d try to help but I can’t reproduce the bug on Windows, either with \n or \r\n, either in the Projucer, the DemoRunner or a minimal example.

[baconpaul]

Surge at head, if you unfork juce, gives a reliable repo on windows and Linux, if you want I can give you directions for that to test.

The projucer screenshots above are just building projucer from source on ubuntu 20 at 6.1 and running it on a new project.

[ed95]

On Windows and macOS the default is to use the native DirectWrite/CoreText layout APIs, respectively, so you need to force the JUCE TextLayout code path to see the bug. Commenting out this line will do it.

[kamedin]

Of course, sorry. Completely forgot about that.

[baconpaul]

FYI: Surge showed the problem on windows also but we are using a memory-loaded TTF font; from reading the windows code it seems like the that case may fall through to the same codepath as linux maybe?

[ed95]

Sounds likely. The Windows implementation for createNativeLayout() has a few checks that may fail for a custom font:

juce-framework/JUCE/blob/4f01392000965f6c5e957329220d5ad06ca174c4/modules/juce_graphics/native/juce_win32_DirectWriteTypeLayout.cpp#L451

[baconpaul]

Yeah I never could figure out custom fonts from ttf in vstgui (which uses dw2d). But the fact that you fall back explains another tiny regression in our port. Thanks!