Add set-selection-offsets function to TextInput, TextEdit, and LineEdit

[BrandonXLF]

The function accepts two arguments that specify the start and the end of the text to select.

Fixes #4164

[CLAassistant]

All committers have signed the CLA.

[ogoffart]

Thanks a lot for this PR!
This is great to see a polished PR with docs and tests and everything.

We're concerned that exposing selection as utf-8 code unit might not be ideal, but IMHO, this is probably the most practical way to do it.
We just should make sure that this works ok-ish if value within code point are selected. So the test ideally should include that.

Another point is weather a function is the right thing, as opposed to exposing the cursor and anchor position as in-out properties.
We could still have both in the future, and the function acts as a convenience, but exposing the properties might be actually better so we can also get the selection.
@tronical : what do you think?

[ogoffart]

The other functions on the TextInput (select-all, copy, ...) use ItemMemberFunction. Is there any reason not to use that? Is that because it has arguments maybe and that's not supported by the code generator right now?
IMHO it would have been better to use ItemMemberFunction because it would probably be simpler, potentially supporting different function signature. But If that's not easy, then it's fine to have it like that.

[BrandonXLF]

Yes it's because of the arguments. I couldn't find an easy way to use ItemMemberFunction while still supporting arguments.

[tronical]

I agree with Olivier. I understand the desire to have this (it makes sense), so I'm in favor of a pragmatic solution: a low-level api for now that leaves the door open for a high-level api later. Low-level means utf-8 offset based.

I'd agree that would be ideally property based - that facilitates reading and writing in one go.

But I have questions:

What if somebody sets wrong values? After setting, should reading them return the wrong value set (symmetry) or should there be a setter behind the scenes that "corrects" it?

If we correct the value at setting time, it becomes more complicated to implement.

If we do the sanity checking only in the implementation at rendering time, then anyone reading the properties must sanity check the values before using them. That makes the api even harder to use.

For example, to advance the cursor to the right, you can't just read the cursor and add one (let's ignore multi-byte for a moment). If somebody recently changed the string, the cursor index might now be completely out of bounds.

So my feeling is that if we offer a low-level api with utf-8 offsets, then IMO at least reading the properties should be "safe" and not require additional work.

[tronical]

Hi @BrandonXLF . Thanks again for the PR :). Olivier and I just discussed this a little further and we'd like go go ahead with your PR. There are changes we'd like to make on top:

1. Add a test that verifies that nothing bad happens when setting invalid utf-8 offsets or and offsets out of range of the text (not a screenshot test).
2. Rename select(start, end) to set-selection-offsets(start, end), to make it clearer that this API takes utf-8 byte offsets.
3. Document that the parameters are utf-8 byte offsets in the current text.

Would you be interested in making these changes? Alternatively, we can also do them. In any case, thanks again for pushing this forward - it's been requested quite a few times.

[BrandonXLF]

Hi @BrandonXLF . Thanks again for the PR :). Olivier and I just discussed this a little further and we'd like go go ahead with your PR. There are changes we'd like to make on top:

1. Add a test that verifies that nothing bad happens when setting invalid utf-8 offsets or and offsets out of range of the text (not a screenshot test).
2. Rename select(start, end) to set-selection-offsets(start, end), to make it clearer that this API takes utf-8 byte offsets.
3. Document that the parameters are utf-8 byte offsets in the current text.

Would you be interested in making these changes? Alternatively, we can also do them. In any case, thanks again for pushing this forward - it's been requested quite a few times.

Sounds good, I'll make the changes shortly.

[BrandonXLF]

@tronical It looks like the byte offsets are accessed using selection_anchor_and_cursor which makes sure the offsets it returns are valid. Should I still write tests specifically for set-selection-offsets?

[tronical]

Thanks for making the changes.

@tronical It looks like the byte offsets are accessed using selection_anchor_and_cursor which makes sure the offsets it returns are valid. Should I still write tests specifically for set-selection-offsets?

It's true, the accessors try to make sure of that, but for example the call to set_cursor_position passes an offset that's not checked, while the implementation passes the offset straight to the renderer for the call to text_input_cursor_rect_for_byte_offset.

I think it would be best to run both values through safe_byte_offset, then we're on the safe side :)

[BrandonXLF]

Thanks for making the changes.

@tronical It looks like the byte offsets are accessed using selection_anchor_and_cursor which makes sure the offsets it returns are valid. Should I still write tests specifically for set-selection-offsets?

It's true, the accessors try to make sure of that, but for example the call to set_cursor_position passes an offset that's not checked, while the implementation passes the offset straight to the renderer for the call to text_input_cursor_rect_for_byte_offset.

I think it would be best to run both values through safe_byte_offset, then we're on the safe side :)

Makes sense. I added the calls to safe_byte_offset and added tests for out-of-range offsets and invalid UTF-8 offsets.

[tronical]

Looks good to me. Thanks a lot :)