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 support for pasting text into a textbox #2281

Merged
merged 7 commits into from Sep 22, 2022

Conversation

ConnectedSystems
Copy link
Contributor

@ConnectedSystems ConnectedSystems commented Sep 18, 2022

Support pasting text into a textbox.

Fixes #2266

Type of change

Delete options that do not apply:

  • New feature (non-breaking change which adds functionality)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@ffreyer
Copy link
Collaborator

ffreyer commented Sep 18, 2022

I was a bit worried this would paste multiple times even on press but it seems fine to me. It behaves pretty much the same as the textbox on github does for me, i.e. paste - pause - repeat pastes with ctrl+v held down. It might be good to check on windows and mac as well though.

For testing we could do something similar to the camera and mouse state machine tests in https://github.com/MakieOrg/Makie.jl/blob/master/test/events.jl. I.e. create a figure with a textbox, trigger some events to select it, fill the clipboard with something (you can pass content to it) trigger down+release of ctrl+v and check if the contents end up in textbox.

@ConnectedSystems
Copy link
Contributor Author

ConnectedSystems commented Sep 18, 2022

Hi @ffreyer

For the test I'm using a mouse click to simulate the textbox being selected.
Seems I have to display a window for the click position to be recognized correctly. Is this okay, given none of the other tests seem to display a window?

Another issue is that I clear the figure and recreate the scene to test the right control + v combination. But the test fails for this attempt, and I don't know why: it always evaluates the stored_string in the second textbox to be nothing.

Would you mind taking a look?
(please also feel free to move the test to somewhere more appropriate)

@ffreyer
Copy link
Collaborator

ffreyer commented Sep 18, 2022

I don't think display should be necessary. Maybe you need remove either the tb.focused = true or the mouse selection bit?

@ConnectedSystems
Copy link
Contributor Author

ConnectedSystems commented Sep 18, 2022

Yes, the focused flag could be removed but without the mouse selection, the textbox will not be selected.

Without display(), the window size will be (0,0), and so the textbox selection (via mouse selection) will not work either, as it relies on clicking the textbox location.

Is there a more straightforward way to selecting widgets programmatically that I'm not aware of?

@ffreyer
Copy link
Collaborator

ffreyer commented Sep 19, 2022

I cleaned up the tests, they should be working now.

Without display(), the window size will be (0,0), and so the textbox selection (via mouse selection) will not work either, as it relies on clicking the textbox location.

There are multiple sources for the window size which can be used. events.window_area is what a backend reports when/after it creates a window. I.e. if there is no window this will report 0 size. There is also scene.px_area which initially reports what size a scene wants to be and then gets resized to match the window, layouting etc. This is what is actually used for most mouse events in makielayout. (Technically they can still depend on point picking but I removed most of these cases a while ago for better performance.)

@ffreyer
Copy link
Collaborator

ffreyer commented Sep 19, 2022

no clipboard command found, please install xsel or xclip

Is it worth setting one of those up to test this?

@ConnectedSystems
Copy link
Contributor Author

no clipboard command found, please install xsel or xclip

Is it worth setting one of those up to test this?

I guess it would be good for completeness, but I'll have to leave this up to you as I don't run *nix at the moment. Sorry!

if is_allowed(char, tbox.restriction[])
insertchar!(char, cursorindex[] + 1)
else
no_err = false
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really seem like an error case, so I'd rather call this all_chars_allowed or something like that.
Also, we should add a catch case and rethrow, no? Or at least warn...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excuse the clumsy implementation.

My thinking here was that it would be obvious that the copy/paste stopped.
I take your point, though, it would make more sense if I check for disallowed characters in the string first, and just abort the paste if any is found. Will make changes in the morning

Copy link
Member

Choose a reason for hiding this comment

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

No worries, this is great :) Maybe just use:

if all(char-> is_allowed(char, tbox.restriction[]), content)
   foreach(char-> insertchar!(char, cursorindex[] + 1), content)
   return Consume(true)
else
    return Consume(true)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

That second Consume returns false instead though, correct? But yes, I get the idea.

Warn when pasting fails
Check all characters are allowed before pasting, otherwise abort.
@SimonDanisch SimonDanisch merged commit 27aa070 into MakieOrg:master Sep 22, 2022
@SimonDanisch
Copy link
Member

Thanks :)

@ConnectedSystems ConnectedSystems deleted the copy-paste-textbox branch November 20, 2022 03:32
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.

Pasting text into a textbox?
4 participants