Expose API for page up/down

[JordanMartinez]

Is StyledTextAreaBehavior#TargetCaretOffset even needed after this (line 220)?

[TomasMikula]

targetCaretOffset is used to remember desired x-coordinate of the caret, even when the current x-coordinate is different. Consider this text, with caret represented by |:

foo bar
foo
foo bar ba|z

after pressing the Up arrow, the caret moves here:

foo bar
foo|
foo bar baz

but targetCaretOffset is used to remember the previous x-coordinate, so that when you press Up again, it goes to the end of the line:

foo bar|
foo
foo bar baz

instead of

foo| bar        // WRONG!!!
foo
foo bar baz

[JordanMartinez]

Then, shouldn't that be moved to StyledTextAreaModel now?

[TomasMikula]

It can't be in model since it is view specific. It really is pixel offset, not a logical position in text.

[JordanMartinez]

It can't be in model since it is view specific. It really is pixel offset, not a logical position in text.

Aye, just realized that....

[JordanMartinez]

I want to put the code in NavigationActions but doing so means StyledTextAreaModel needs to have it as well. On the other hand, why does STAM even implement those interfaces since it's not even public?

[TomasMikula]

That I cannot answer 😃
The whole existence of interfaces like NavigationActions, ClipboardActions, ... is somewhat dubious. NavigationActions has to be thought of as logical navigation. There are no pages there.

I would say, just put it into StyledTextArea.

[JordanMartinez]

That I cannot answer 😃

😆

If I put it into TextEditingArea, then I don't come across that issue. There are already other select-like methods there (e.g. selectRange).
I'd like to remove those interface implementations from the model in a separate PR, one focused more on code cleanup. My guess is it was carried over from when we separated all the model content in StyledTextArea from the view itself.

[TomasMikula]

Sounds good.

I think it was my attempt to distinguish what are the core methods (these are those that are abstract in those interfaces) and what are derived methods (those with default implementation in the interfaces). StyledTextArea(Model) can then focus on implementing only the core methods, inheriting the implementations of the derived methods. The goal was to make StyledTextArea code clearer, but I'm not sure that it really helped.

[JordanMartinez]

On second thought, looks like I'll need to clean it up first before exposing the page up/down API. Additionally, StyledTextAreaBehavior does not need access to the model since it can access those things through the view. The only reason to do so is to save some time by avoiding a method wrapper (view calls the same method in model).

[JordanMartinez]

Sigh... removing the interfaces then screws up the STAModel's tests.

Screw this... I'm going to just put the code in the area as opposed to the interface. Fixing the above test issue, along with cleaning up the interfaces themselves, should be something we do in a different PR.

[JordanMartinez]

Ok, now it works.

[TomasMikula]

Yay! 👍

[JordanMartinez]

I don't know whether I should open a new issue regarding this, and I don't know what I would start with in setting up a new PR for this, but what should we do to clean up the implementations?

There is value in having the model implement those interfaces because we can use it still when creating tests (as opposed to having to do that via the view). At the same time, some methods can't go into the interfaces due them being view-related. However, removing those interfaces from the model would then lead to code duplication. If we break up the interfaces into more ones, then I assume that it will get more confusing/scattered.

[TomasMikula]

When in doubt, do nothing 😃

[JordanMartinez]

When in doubt, do nothing 😃

Sounds good to me. Besides, it won't cause any extra work for the custom object PR (#314)