Issues with TextEditor: caret and wrapping

Issues:

  • caret disappears at the end of lines, both in single and multi line
  • caret goes to wrong position after a delete or undo/redo that makes the scrollbar disappear
  • when two wrapped atoms follow each other, their last / first lines get overlapped
  • the end of a wrapped atom always ends a line, even if the next one fits

Fixes:

  • getMaximumTextWidth() should be the viewport max width minus leftIndent and rightEdgeSpace (instead of plus rightEdgeSpace).
  • In checkLayout(), textRight should be the max between the viewport max width and getTextRight() + leftIndent + rightEdgeSpace, as getTextRight() is relative to the text, not the textHolder.
  • In scrollEditorToPositionCaret() and scrollToMakeSureCursorIsVisible(), getCaretRectangle() is relative to the text, not the textHolder, so it doesn’t include the indents. This is compensated in the y pos checks by subtracting / adding topIndent, but not in the x pos checks. I think it’s easier to add the indents to caretPos / caretRect directly.
  • In undoOrRedo(), scrollToMakeSureCursorIsVisible() should be after textChanged(), which calls checkLayout(), which updates the scrollbar visibility.
  • remove() lacks a checkLayout() before moveCaretTo(), as in insert(). This is a problem for cut(), because insert() does nothing, so checkLayout() gets called after all calls to repaintText() are done.
  • A wrapped atom always starts a new line. When one is found, we set tempAtom and return next(). When we find the split point, if it’s the atom’s end we call beginNewLine(), otherwise we increment lineY and set atomX to the offset for the temp’s width. This fixes the last two issues.
  • Additionally, atomX = justificationOffsetX after beginNewLine() is redundant.

Of course I may be wrong about anything. It’s all in this commit.

1 Like

Thanks for the reports and suggested fixes! These mostly look good and we’ll get them merged to the develop branch shortly.

I’m struggling to reproduce the wrapping bug though and the proposed changes seem to break the current line layout calculations. If I have a multi-line editor and enter text until it wraps, it adds a new line at the start of the atom. Do you have an example which reproduces the bug you’re seeing with the current code?

No problem! The third issue happens with centredRight justification. Write four lines of text without spaces, then add a space in the second or third line:

3rd

This is caused by

if (atomX > justificationOffsetX)
    beginNewLine();

which prevents the atom to start in a new line under the given circumstance. But a wrapped atom should always start in a new line -the first split is set to fill the whole line width, so it can’t share space with a previous atom. Now, we have those at 433-434, then we return next(), enter through the first conditional, and because we’ve just started and numChars is 0,

if (tempAtom.numChars > 0)
    lineY += lineHeight * lineSpacing;

this increment will be missed. So, that beginNewLine() is in place of this one that isn’t happening. But we don’t need to call beginNewLine() here, we just need to increment lineY -if the line is filled by this atom, there are no next atoms to include. So instead of making the newLine, returning next(), then avoiding the first increment, we can make the increment directly after reentering. Where we do need beginNewLine() is at the end of the atom, where we may have next atoms that still fit -this is a wrapped atom, a space, then a shorter one:

4th

So I replace both 433-434 and 314-315 with

if (split == numRemaining)
{
    beginNewLine();
}
else
{
    lineY += lineHeight * lineSpacing;
    atomX = getJustificationOffsetX (tempAtom.width);
}

so that we call beginNewLine() after the last split, and just increment lineY for the previous ones.

Just saw the caret stuff is done, thanks! So these are the wrapping / alignment issues, updated:

  • when the first atoms in a right aligned text are wrapped, their last / first lines get overlapped
  • the end of a right aligned wrapped atom always ends a line, even if the next one fits
  • if a glyph is too wide to fit a line, it disappears
  • for right aligned and centred text, the line after a newline gets shifted to the left

The last issue is caused by newlines having width, which messes with beginNewLine().

I get the point of justificationOffsetX now -a wrapped atom should start a new line, unless it’s already on a new line. The first issue happens because, on construction, beginNewLine() sets atomX and justificationOffsetX to the line width. After the first atom, atomX is set to atomRight, which is again the line width because of right alignment. So atomX == justificationOffsetX signals a newline that didn’t happen. Calling beginNewLine() on the last split to fix the second issue fixed the first one too, because it sets justificationOffsetX to a different value. It also inserted wrong newlines, as you saw. The third issue is caused by if (split > 0 && split <= numRemaining), which just discards a wide glyph and goes to the next atom. This doesn’t seem right, especially because some glyphs can be discarded while others aren’t.

I guess beginNewLine() could do something different for a wrapped first atom, but I think it makes more sense to use another, more purposeful way of checking if a new line already started. We only need to know if we’re at the start of the whole text, or right after a newline character. This is known in the course of next(), it doesn’t even need a member variable.

I think this works now. Sorry for the noise, it took some time to get my head around it. I really didn’t want to mess with Iterator, but I needed to fix this.

Nice work! I’ll merge something shortly. Thanks for the effort on this, it’s certainly not an easy bit of the framework to get your head around.

2 Likes

Just saw the last commits! A couple of things.

  • If shouldStartNewLine is checked after split == numRemaining, we get an additional newline when the former is false and the latter is true.
  • If split were to be clamped directly, the loop may start from 1 in the first place, but I saved split and used actualSplit separately because split == numRemaining needs to be false for split == 0. Otherwise, beginNewLine() sets atomX to bottomRight.x. (We also can’t move the width assignment after the checks, because beginNewLine() needs to see it updated).

The effect of these two can be seen here:
a b
An additional newline is added above by beginNewline() for shouldStartNewLine == false, and the "d"s are shifted to the right by beginNewLine() for split == 0. That’s why I made the checks in that order:

  • first if no new line should start (set atomX),
  • then if it’s the last split, unless split == 0 (call beginNewLine),
  • finally if it’s not the last split, or it is, but split == 0 (set both lineY and atomX).

Sorry to bother! And thanks again.

Ah, I see - thanks! I suppose we could also check shouldStartNewLine before calling beginNewLine() inside the if-clause. Something like this seems to fix those issues for me:

        int split;
        for (split = 0; split < g.getNumGlyphs(); ++split)
            if (shouldWrap (g.getGlyph (split).getRight()))
                break;

        const auto numChars = jmax (1, split);
        longAtom.numChars = (uint16) numChars;
        longAtom.width = g.getGlyph (numChars - 1).getRight();

        if (split == numRemaining)
        {
            if (shouldStartNewLine)
                beginNewLine();

            atomRight = atomX + longAtom.width;
            return true;
        }

        if (shouldStartNewLine)
            lineY += lineHeight * lineSpacing;

        atomX = getJustificationOffsetX (longAtom.width);
        atomRight = atomX + longAtom.width;

        return true;

We still have to set atomX for the other case:

        if (split == numRemaining)
        {
            if (shouldStartNewLine)
                beginNewLine();
            else
                atomX = getJustificationOffsetX (longAtom.width);

            atomRight = atomX + longAtom.width;
            return true;
        }

I think that would cover it all :slightly_smiling_face:

In that case, it might be simpler to the following:

        int split;
        for (split = 0; split < g.getNumGlyphs(); ++split)
            if (shouldWrap (g.getGlyph (split).getRight()))
                break;

        const auto numChars = jmax (1, split);
        longAtom.numChars = (uint16) numChars;
        longAtom.width = g.getGlyph (numChars - 1).getRight();

        atomX = getJustificationOffsetX (longAtom.width);

        if (shouldStartNewLine)
        {
            if (split == numRemaining)
                beginNewLine();
            else
                lineY += lineHeight * lineSpacing;
        }

        atomRight = atomX + longAtom.width;
        return true;
1 Like

Indeed! It’s much clearer.

This is on develop now.

1 Like