Seamless Page Overflow V.2#123
Conversation
chrrs
left a comment
chrrs
left a comment
There was a problem hiding this comment.
Hi, thanks for reworking the PR! It definitely looks a lot better than the last one, though I feel like there's still a lot of unnecessary abstractions here that make it a lot more complicated than it needs to be.
I haven't looked too much at the details of the implementation yet, for now mostly some more high-level structure comments. Again, like in the previous PR, this is definitely a good base, but there's a lot of places here where there's existing code that you could definitely reuse, instead of writing a similar thing twice.
| * | ||
| * @return a new RichText with all carriage returns removed. | ||
| */ | ||
| public RichText filterCarriageReturns() { |
There was a problem hiding this comment.
Shouldn't this just be filtered out the raw clipboard text when pasting, instead of after having processed everything already?
|
|
||
| void setPageContent(int page, RichText content); | ||
|
|
||
| void refreshPages(); |
There was a problem hiding this comment.
Most of these methods feel unneccessary, this can all be done using switchAndFocusPage, see also EditCommand:
getTotalPagesfeels misplaced? It's never used in history commands.getPageContent->textField.getRichText.setPageContent->textField.setValueWithoutUpdatingortextField.setValue.refreshPages->textField.sendUpdateFormat
I might be missing something here, do tell me, but I'd rather avoid having multiple ways to do the same thing.
|
|
||
| //region History | ||
| public void pushCommand(Command command) { | ||
| this.pushCommand(0, command); |
There was a problem hiding this comment.
I'd rather always specific pageOffset, instead of silently passing a 0 here. Accidentally calling this with for example an EditCommand could lead to some sneaky bugs.
| public Value<Boolean> copyFormattingCodes = value(true); | ||
| public Value<Integer> editHistorySize = value(32) | ||
| .range(8, 128, 1); | ||
| public Value<Boolean> overflowWhenTyping = value(false); |
There was a problem hiding this comment.
Like mentioned in the previous PR (#92 (comment)), I think it would be nice to have an enum here with three options:
- Don't overflow ever.
- Overflow only on enter / backspace (so you don't accidentally enter a new page while just typing / you can hold down something like
-to fill up the last line). - Always overflow when running out of room.
| public Value<Integer> editHistorySize = value(32) | ||
| .range(8, 128, 1); | ||
| public Value<Boolean> overflowWhenTyping = value(false); | ||
| public Value<PasteBehavior> pasteBehavior = value(PasteBehavior.DENY); |
There was a problem hiding this comment.
The name "Paste behaviour" doesn't really indicate its connection to overflowing. Maybe "Overflow when pasting" to match the other option?
| import java.util.function.Consumer; | ||
|
|
||
| @NullMarked | ||
| public class TextOverflowHandler { |
There was a problem hiding this comment.
Two things here:
- Why isn't this just in
ScribbleBookEditScreen? - The name
TextOverflowHandleris really confusing, given there's the completely unrelated interfaceOverflowHandler.
| public void execute(HistoryListener listener) { | ||
| int newPages = after.size() - 1; | ||
| for (int i = 0; i < newPages; i++) listener.insertPageAt(page + 1 + i, null); | ||
| for (int i = 0; i < after.size(); i++) listener.setPageContent(page + i, after.get(i)); |
There was a problem hiding this comment.
See the comment in HistoryListener, why does this use setPageContent, and not just pass the text to insetPageAt directly?
| setCursor(listener.switchAndFocusPage(page), before.getLength()); | ||
| } | ||
|
|
||
| private void setCursor(RichMultiLineTextField tf, int pos) { |
There was a problem hiding this comment.
I think it might just be clearer if this is done inline where it's called, since this just hides the behaviour behind a function, and it's only two lines.
| int maxPages = 100 - listener.getTotalPages() + 1; | ||
| if (cursor != text.getLength() || maxPages < 1) return false; | ||
|
|
||
| // Truncate insert to max pages × ~400 chars/page (conservative estimate) |
| @NullMarked | ||
| public class TextOverflowHandler { | ||
| private static final int PAGE_WIDTH = 114; | ||
| private static final int LINE_LIMIT = 14; |
There was a problem hiding this comment.
Don't these already exist in other places: textField.width and textField.lineLimit?
2 participants
Back in february I reworked the old pull request (#92 ) from the ground up, but never got around to submitting it. Suffice it to say I believe it is a lot better, but I am not sure if even this improved version is remotely close to the rest of the mod.
The features include more customisability on how you want the page to overflow.
I liked your suggestion with the enum and went a bit further: you now have two settings:
"Overflow when typing" and Overflow when Pasting ("Paste Behavior").
The overflow when typing is just a normal boolean toggle which is off by default and when turned on it automatically continues on the new page when typing and also you then get the enter and Backspace shortcuts from before.
Then for the second setting "Paste Behavior" here the default is deny if it doesn't fit (so the vanilla behaviour), the new option here being fill page: When you paste something it will fill the page until full but not continue on the next pages.
And then lastly the Overflow to next page behaviour which is just the same except it continues on the following pages.
Personally thought about changing the default paste behaviour to fit page because I think it is more intuitive that way, it changes vanilla behaviour but so does the entire mod. I left the default in the pull request on deny if it doesn't fit but I am curious what your thoughts on that are.
On the topic of synergies I now consolidated the way the paste and typing overflow system work so they go through one function which leads to the same behaviour when pasting or typing a single character. I know this implementation is probably not what you had in mind when you cited the binary search as too complex but I decided to keep it and just give it the same word wrapping as typing because when testing without the binary search there was a noticeable lag/freeze when pasting a bit more and the game could crash when pasting for example moby dick in (even though it cuts off at 100 pages).
Also while testing I noticed that some websites have a next line symbol which appears when pasting the contents in the book so in this new version they get filtered out.
#57