Seamless Page Overflow

[J4PC]

For a change, I tried implementing one of the features that I suggested #57 . I tried to make it as elegant and compact as possible, and it works, but I have not a great deal of experience, so if you want to implement it, I am very open to your suggestions.

The features include:

Automatic continuation on the next page

The obvious idea would be to make an overflow system like Word, where no matter where you type, if the page is full it overflows. But because Minecraft pages are small, it isn’t fun to format something in the middle of the page and have it constantly overflow to the next page, outside the view of the player and potentially ruining the next page. So instead, the overflow only triggers if you directly type at the end of the current page. If text overflow would occurs when typing anywhere else on the page, nothing happens. The text that overflows isn’t put on the next page; instead, an empty page is inserted where the overflowed text is put. This solves the problem of the overflow ruining the next page.

Convenience features that make it feel more connected

If you press Enter in the last line of a page, you continue on a new page after the current one.
If you press Backspace on a completely empty page, the page is removed and you are brought to the page before it.

Both of these minor things are standard staples in other text formatting programs.

Copying and pasting larger texts

Inserting large text can really benefit from the automatic overflow, so when pasting text, it overflows in the same way as if you had typed the text. This means that you can now paste longer texts than the previous one-page limit would allow.

[chrrs]

Hi! Thank you for the PR, it's really cool that you decided to try and implement it yourself :). I don't have the means to test this in-game right now, sorry, I'll do that soon! But I did read over all the code, and left some comments!

I think it's definitely a really good base, the overflow handling itself actually looks really solid, so I really don't have a lot of comments on that!

The integration with the rest of the code I think is a lot more complicated than it needs to be, a few changes in structure could really make this a lot simpler. This mostly comes down to using the code that's already there, there's a lot of extra code here that already exists in a slightly different shape, which you could probably adapt.

Also, if you're using IntelliJ, make sure to reformat and optimize imports, that'll help with making it a lot more readable. There's a few places with weird formatting, indents and imports.

Feel free to disagree with me wherever, would love to hear your thoughts!

---

One thing I'll put here, there's not really one place to make a comment about it: I think it would make a lot of sense to change scribble$pushEditCommand to something like this:

@Unique
private void scribble$pushCommand(Command command) {
    RichEditBoxWidget editBox = this.scribble$getRichEditBoxWidget();

    if (command instanceof EditCommand editCommand) {
        // .. set page, color, modifiers, etc.
    } else if (command instanceof PageOverflowCommand overflowCommand) {
        // .. set original content, etc.
    }

    // and so on..

    this.scribble$commandManager.push(command);
    this.scribble$invalidateActionButtons();
}

This way you centralise where the current state is added to the command. You can then just push all command through onHistoryPush, which should allow you to get rid of all of onCommandPush, onPageOverflow, onInsertPage and onDeletePage :).

[chrrs]

As much as I definitely like the feature, I think this one should be disabled by default, because it changes vanilla behaviour in a bit of an opinionated way. For example, if I want to add a line to the bottom of the page, I tend to just hold down enter and then hold down '-' (or similar), which I know won't overflow.

[chrrs]

What's the difference between onCommandPush and onHistoryPush? Seems like two consumers that lead to the same place, it might be better to just make onHistoryPush take a Command instead.

[chrrs]

I actually really like these two options! I honestly feel like the config option could be an enum with this:

• Disabled
• Overflow on enter/backspace
• Always overflow

[chrrs]

I skipped over this a few times: these handlers should probably always be set. Right now, in multiple places you're checking for something like onInsertPage != null to see if the config option is set. You probably want to just check the config option in place instead, makes your intent a lot clearer :).

A second reason would be for example what if a second feature would use the onInsertPage callback at some point. Also, at the end of the day, you'd expect a builder class to build exactly what you pass into it.

[chrrs]

Nitpick: you could just do something like RichEditBoxWidget editBox = listener.scribble$history$getRichEditBox(); at the start, then use that variable. A lot of the lines here (and in a few other places) will be a lot shorter and more readable.

[chrrs]

Shouldn't this go in PageDeleteCommand? I saw you did something similar for PageInsertCommand, where it's actually in the command, which makes more sense I think.

[chrrs]

I feel like if the content should potentially be trimmed, it should just happen when the command is created or executed. I think PasteCommand only get's constructed in a single place anyway.

I don't like the special casing here. I forgot I do this for edit commands too, oops. You'd probably want to switch/instanceof, and assign the current state when needed.

Also, import your classes, see above :). Also also, as mentioned, I think this function already exists as pushHistory.

Also also also, weird indentation here.

[chrrs]

Like above, I think this could just be an onHistoryPush, with originalContent assigned like how it works for EditCommands.

[chrrs]

I think the best way would just be to do + 1 when creating the command, instead of the boolean. That way, PageInsertCommand will always create a page at the given index.

For the if in rollback, I think (?) you could just compare the page index with the current page in scribble$history$deletePage, and if it's lower, switch back a page. Though I'm just saying things, I'm really not sure.

[chrrs]

I think this is where I'd make the biggest change. Right now, you're basically doing overflow handling twice: once for typing, and once for pasting. Both have slightly different logic, which also leads to some strange behaviour (for example, you handle word overflowing for typing, but not for pasting. What if you paste a single character, that should behave the same as if you typed it).

Every operation that inserts text goes through an EditCommand already, so I'd say rename this function to something like insertText, and just let this method handle everything to do with inserting text:

• Overflow the current word if needed.
• Overflow onto many pages if needed.
• Execute and push the right command (PageOverflow if it overflows, Edit if it doesn't).

Now you can replace every instance of inserting text with edit commands with calling insertText (I think it's only typing and pasting), and overflows should be handled correctly for everything. This should also make it so you can get rid of PasteCommand, and just merge it into PageOverflowCommand, since it's a lot of the same again.

[J4PC]

Sorry, accidentally closed the pull request when syncing with the 2.0 changes. Will reopen it when the revised version is ready.