TextEditor bug


#1

Hi Julian,

TextEditor triggers a failed assertion on CTRL+<- when the text it contains has leading whitespace.

Here’s how I fixed it:

(juce_TextEditor.cpp:2408)

        const int type = getCharacterCategory (t [position - 1 - startOfBuffer]);

=>

        int type = 0;
        if (position != 0)
            type = getCharacterCategory (t [position - 1 - startOfBuffer]);

Thanks.


#2

Wow - that’s a dumb one! Thanks very much!


#3

By the way,

I’m finding it a bit confusing that the cursor stops at the beginning of a word when CTRL-LEFTing and at the end of a word when CTRL-RIGHTing ?

My browser’s text input browses word starts only. So does my Visual Studio.

It tried to disable whitespace skipping in TextEditor::findWordBreakBefore and TextEditor::findWordBreakAfter, and it feels quite nice, actually.

But I agree, this is plain hair-splitting :slight_smile:


#4

yes, it’s hair-splitting, but these little things matter! I’ll take a look.


#5

Great, thanks.

Also, while playing with the TextEditor, I got some strange numbers, performance-wise.

I am trying to use a read-only TextEditor as a text output console, repeatedly calling TextEditor::insertTexAtCursor().

When adding more than a thousand lines, things start getting very expensive and unbearably slow.

A little bit of profiling showed that the undo manager gets very busy while inserting lines. Disabling when in read-only mode improved things.

I noticed that 90% of the cpu time spent in TextEditor::insert is sucked up by moveCursorTo (which tries to reposition the viewport to make the cursor visible, and therefore recomputes a lot of text metrics).

And I’m pretty much stuck here.

Am I using the wrong Component for this task?


#6

Yes, I’ve never optimised it for really large amounts of text. But it does go massively faster in release mode than debug, because there’s a huge number of assertions in there, so try a release build before you spend too long worrying about its performance!


#7

I wouldn’t have posted without having tried in release mode, of course :slight_smile:

It’s really a matter of doing too many things, here, not of doing them too slowly:

1 000 calls to TextEditor::insertTextAtCursor generates

2 000 calls to ViewPort::setViewPosition, which in turn generates

3 000 calls TextEditor::updateTextHolderSize, which amounts to

4 000 000 calls to TextEditorIterator::next

The 1000 lines is just the sequence “1\n” -> “1000\n”.


#8

Yes - like I say, I’ve never tried optimising it!

But thinking about what you found there, I’ve just checked in a couple of quick and simple optimisations that might make a big difference - give it a try…


#9

Cool :slight_smile:

It’s about 3 times faster with my use case, but still blocks the message thread for a loooong time.

It also seems to overflow the message queue when making more than 10000 insertTextAtCursor calls.

I’ll try to be more gentle with TextEditors in the future :slight_smile:

Thanks a lot!


#10

You’re doing 10000 inserts programatically?? That’s nuts! No wonder it’s messing up - better to create your string beforehand and insert it in a single operation, of course.

I’m still a bit surprised that it’d break the message queue though - any idea which bit of it is responsible for that?


#11

Yeah, I know, I’m a crazy person :slight_smile:

Trouble is, the TextEditor is used like a console output, and I have no way of predicting what console writers will do with it. I guess a Timer and some buffering will help.

I’ll take a closer look at the message queue breaking and let you know.

Thanks.


#12

Ah, I see - that’s a pretty reasonable use for it! But it’ll never be very quick to insert text, because of all the undo/redo stuff that’s required, so you’re better off batching the insertions together if possible.


#13

Okay, thanks :slight_smile:

I did not manage to reproduce the message queue overflow…
Maybe your performance tweaks fixed it.
I don’t have time to further investigate, though :frowning:

Another thought:

Is there a reason for the TextEditor to catch the return keypresses with ANY modifiers ? Isn’t it possible to let it catch only naked ones?

I’d like to bind CTRL-RETURN on the parent component, and have that binding active when the user types text in the TextEditor.

The required change is just a one-liner:

else if (key == KeyPress::returnKey)

instead of

else if (key.isKeyCode (KeyPress::returnKey))

in juce_TextEditor.cpp:1886.

Thanks!


#14

Yes - a good suggestion there, I’ll change that - thanks!


#15

textEditor.setScrollToShowCursor(false);
// No scrollbars.

textEditor.setText(someVeryLongStringCausingScrollbarsToBeDisplayed);
// Scrollbars shown.

textEditor.clear();
// :-(
// Scrollbars still there.

textEditor.setText(String::empty);
// Yeah, scrollbars gone.

Is this normal ?


#16

You’re really hammering that TextEditor, aren’t you! How about this:

void TextEditor::clear() { clearInternal (0); updateTextHolderSize(); undoManager.clearUndoHistory(); }


#17

Wonderful!

Sorry for the hammering… but I really like this TextEditor thingy :slight_smile:

And I’m afraid I have another request about it:

applyFontToAllText() seem to always move the caret back to (0,0) because clearInternal() calls moveCaret() with a position that don’t exist anymore.

So I did this in applyFontToAllText:

    const int originalCaretPosition = caretPosition;
    clearInternal (0);
    insert (oldText, 0, newFont, findColour (textColourId), 0, originalCaretPosition);

But maybe there’s a better way…


#18

Actually, that method needed a bit of a spring-clean. I’ve checked in a much more efficient version now.


#19

Cool! Thanks!

But…

        textColourId             = 0x1000201, /**< The colour that will be used when text is added to the editor. Note
                                                   that because the editor can contain multiple colours, calling this
                                                   method won't change the colour of existing text - to do that, call
                                                   applyFontToAllText() after calling this method.*/

This doesn’t hold true anymore. And I am relying on it…


#20

doh! Yes, of course. Try it again now…