Minimal clipboard handling #95

ilidemi · 2023-09-11T09:18:28Z

ilidemi commented

2023-09-11 09:18:28 +00:00

Handling the basic case of copy-pasting text with platform hotkeys without the need for permissions.

On ctrl+v/cmd+v/shift+ins, runtime generates an extra OC_EVENT_CLIPBOARD_PASTE event. Guest can call oc_clipboard_get_string() while in the event handler (but doesn't have to). Outside the event handler, the call is a no-op that emits a warning.
On ctrl+x/ctrl+c/..., runtime opens a 1 second window for the guest to call oc_clipboard_set_string(). After the window passes, the call is a no-op that emits a warning.

Handling the basic case of copy-pasting text with platform hotkeys without the need for permissions. * On `ctrl+v`/`cmd+v`/`shift+ins`, runtime generates an extra `OC_EVENT_CLIPBOARD_PASTE` event. Guest can call oc_clipboard_get_string() while in the event handler (but doesn't have to). Outside the event handler, the call is a no-op that emits a warning. * On `ctrl+x`/`ctrl+c`/..., runtime opens a 1 second window for the guest to call `oc_clipboard_set_string()`. After the window passes, the call is a no-op that emits a warning.

ilidemi commented

2023-09-11 09:21:02 +00:00

The change introduces a bit of new organization, feedback and nitpicks are welcome.

The new clipboardArena seems very special case but we can't use frameArena because it gets reset at begin_frame(), and clipboard text will likely come before begin_frame() and can be discarded at end_frame().

The hotkeys are duplicated in runtime_clipboard.c and ui.c - my understanding is that the only way to unify would be to have all of them as #defines (because commands in ui.c need to be constants) and that would be fairly ugly.

The new API would probably need to be documented.

The change introduces a bit of new organization, feedback and nitpicks are welcome. The new `clipboardArena` seems very special case but we can't use `frameArena` because it gets reset at `begin_frame()`, and clipboard text will likely come before `begin_frame()` and can be discarded at `end_frame()`. The hotkeys are duplicated in `runtime_clipboard.c` and `ui.c` - my understanding is that the only way to unify would be to have all of them as `#define`s (because commands in `ui.c` need to be constants) and that would be fairly ugly. The new API would probably need to be documented.

ilidemi force-pushed minimal-clipboard from aea6543c7c to f0616ba27e

2023-09-11 09:30:18 +00:00

Compare

ilidemi commented

2023-09-11 09:34:56 +00:00

Would also appreciate someone testing on a Mac.

MartinFouilleul commented

2023-09-11 10:38:15 +00:00

A few nitpicks:

You should avoid directly using oc_scratch() outside of the highest level runloops, since you can accidentally clear stuff pushed upstream (I think it can happen in oc_runtime_clipboard_process_event(), line 78 for example). Prefer using oc_scratch_begin()/oc_scratch_end(). (I should probably remove oc_scratch() altogether)
Is oc_wasm_arena_push_addr() really necessary? We always call oc_wasm_address_to_ptr next. However, it is a good call to check the length available to that pointer! But we could do that check in oc_wasm_arena_push()
You can #if guard the whole oc_ui_clipboard_register_default_hotkeys (and its declaration), so that using it would result in a compile-time error rather than a runtime one
Do we need pasted text in every box ui_sig struct? Couldn't we rather check oc_clipboard_pasted_text when text box has focus?
Maybe we could clear frameArena at the end of the frame and use it for clipboard stuff (avoiding a second clipboardArena)
Maybe we could avoid having to call oc_ui_clipboard_register_default_hotkeys() by having a function to query the clipboard (that takes an arena) that is only enabled when processing a paste event. Registering different hotkeys also wouldn't need passing the ui context.

A few nitpicks: - You should avoid directly using `oc_scratch()` outside of the highest level runloops, since you can accidentally clear stuff pushed upstream (I think it can happen in `oc_runtime_clipboard_process_event()`, line 78 for example). Prefer using `oc_scratch_begin()`/`oc_scratch_end()`. (I should probably remove `oc_scratch()` altogether) - Is `oc_wasm_arena_push_addr()` really necessary? We always call `oc_wasm_address_to_ptr` next. However, it is a good call to check the length available to that pointer! But we could do that check in `oc_wasm_arena_push()` - You can `#if` guard the whole `oc_ui_clipboard_register_default_hotkeys` (and its declaration), so that using it would result in a compile-time error rather than a runtime one - Do we need pasted text in every box `ui_sig` struct? Couldn't we rather check `oc_clipboard_pasted_text` when text box has focus? - Maybe we could clear frameArena at the _end_ of the frame and use it for clipboard stuff (avoiding a second clipboardArena) - Maybe we could avoid having to call `oc_ui_clipboard_register_default_hotkeys()` by having a function to query the clipboard (that takes an arena) that is _only_ enabled when processing a paste event. Registering different hotkeys also wouldn't need passing the ui context.

MartinFouilleul commented

2023-09-11 10:41:05 +00:00

Would also appreciate someone testing on a Mac.

Works on macOS btw!

> Would also appreciate someone testing on a Mac. Works on macOS btw!

ilidemi commented

2023-09-12 21:44:05 +00:00

All good feedback. On frameArena, I got a wrong impression that boxes live there and need to survive till render, now I see there's a persistent pool.

Changed the paste to generate an extra event so that the guest doesn't have to guess where did their keypresses go if they're not using the clipboard.

All good feedback. On `frameArena`, I got a wrong impression that boxes live there and need to survive till render, now I see there's a persistent pool. Changed the paste to generate an extra event so that the guest doesn't have to guess where did their keypresses go if they're not using the clipboard.

MartinFouilleul force-pushed minimal-clipboard from f92523d13c to e057105586

2023-09-13 14:56:52 +00:00

Compare

MartinFouilleul force-pushed minimal-clipboard from e057105586 to a5567da82c

2023-09-13 14:59:59 +00:00

Compare

MartinFouilleul merged commit a5567da82c into main

2023-09-13 15:00:32 +00:00

Sign in to join this conversation.

No reviewers

No Label

macOS

windows

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hmn/orca#95