Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add entry.Insert API to insert text at current cursor position #5334

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

dweymouth
Copy link
Contributor

Description:

Fixes #3445

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style and have Since: line.
  • Any breaking changes have a deprecation path or have been discussed.
  • Check for binary size increases when importing new modules.

@coveralls
Copy link

Coverage Status

coverage: 59.671% (+0.02%) from 59.65%
when pulling 0b2a001 on dweymouth:3445-entry-insert
into 0afa6a9 on fyne-io:develop.

@andydotxyz
Copy link
Member

I think the review comments were removed by the force push.

@dweymouth
Copy link
Contributor Author

I think the review comments were removed by the force push.

I actually think conversation had only been on the linked issue so far and not the PR. But noted on the force push, will use merge commit next time :)

@andydotxyz
Copy link
Member

I think the review comments were removed by the force push.

I actually think conversation had only been on the linked issue so far and not the PR. But noted on the force push, will use merge commit next time :)

Ah, that makes sense, sorry for the confusion.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - just one note about potentially unexpected behaviour...

return // Nothing to paste into the text content.
}

if !e.MultiLine {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code is specific to clipboard so maybe it should be outside of the new insert API?

If you think it does belong here please document it in the API doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matwachich Do you have an opinion on whether the InsertAtCursor API should also reformat multiline text when inserting into a single-line entry (like paste does)? I feel like it maybe shouldn't since it'd be the developer who controls what is being inserted, vs a user action like paste.

@matwachich
Copy link
Contributor

matwachich commented Jan 3, 2025 via email

@dweymouth
Copy link
Contributor Author

Default behaviour could be to convert line breaks to spaces Le ven. 3 janv. 2025 à 20:01, Drew Weymouth @.***> a

There won't be a default behavior but a single, unchangable behavior since the API doesn't have any other arguments for options. For that reason I'm leaning toward inserting the text exactly as-is, and documenting it as such. It's more flexible since the programmer is in control of the text passed to the API, so if they want to remove newlines, or process the string in any way, they can do so. But if we process the string, there would be no way to disable it.

@matwachich
Copy link
Contributor

matwachich commented Jan 3, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants