nyxt icon indicating copy to clipboard operation
nyxt copied to clipboard

Electron input handling

Open jmercouris opened this issue 1 year ago • 5 comments

Description

  • Support synchronous input handling in cl-electron.

Checklist:

  • [ ] Git branch state is mergable.
  • [ ] Changelog is up to date (via a separate commit).
  • [ ] New dependencies are accounted for.
  • [ ] Documentation is up to date.
  • [ ] Compilation and tests ((asdf:test-system :nyxt/<renderer>))
    • No new compilation warnings.
    • Tests are sufficient.

jmercouris avatar Apr 05 '24 02:04 jmercouris

@aadcg There are two commits here, both show two different ways of handling the new challenges faced with synchronous input handling in electron. The bottom line:

WE CANNOT execute ANY JavaScript when handling input from the renderer. We need to return t/nil first AND THEN execute whatever JavaScript that occurs as a result of handling input.

If we do not do this, the system will deadlock.

Why?

  1. JavaScript is waiting for a synchronous response from us as to whether it should prevent an event from bubbling, or consume it. In other words, JavaScript needs to know whether it needs to run event.preventDefault().
  2. In our input handling on the Lisp side, if we try to execute some JavaScript before we return t/nil we'll send a request to a blocked JavaScript program (the one waiting for synchronous input), asking it to do something.
  3. Our Lisp program will not continue until the JavaScript we tried to execute runs.
  4. Our Lisp program will be unable to return t/nil.
  5. We have deadlocked our program.

I hope the above makes sense. It is kind of hard to write out :-)

jmercouris avatar Apr 05 '24 02:04 jmercouris

@jmercouris I think I understood the ideas. If I'm not mistaken, you're still trying out different approaches so I'll review when ready.


Please limit you summary commit messages to 50 chars.

(source) A note on commit messages: Though not required, it’s a good idea to begin the commit message with a single short (no more than 50 characters) line summarizing the change, followed by a blank line and then a more thorough description.

aadcg avatar Apr 05 '24 07:04 aadcg

I will keep this in mind, and whenever reasonable, will limit my commit message summaries!

jmercouris avatar Apr 05 '24 16:04 jmercouris

@jmercouris I think I understood the ideas. If I'm not mistaken, you're still trying out different approaches so I'll review when ready.

I am ready to continue with one approach, I would like your opinion about which one it should be. Do you have any suggestions about what you think?

jmercouris avatar Apr 05 '24 16:04 jmercouris

I'm not sure I understand the consequences of each of the 2 approaches. The one from commit 519d38065 seems closer to how it works in GTK. The other one, from commit d058c177e, seems odd in the sense that consume-input-event-p and dispatch-input-event need to coexist, even though they're cognitively connected.

So, my question would be: in what way is d058c177e superior to 519d38065?

aadcg avatar Apr 05 '24 19:04 aadcg

For future reference, {sly,slime}-lisp-implementations has been set to

(nyxt-nix
 ("nix-shell" "shell.nix" "--run" "sbcl --dynamic-space-size 3072")
 :directory "~/common-lisp/nyxt/build-scripts/")

@jmercouris I took the approach that doesn't block on input dispatch. Seems to be working well.

Unfortunately, I wasn't able to ensure that it doesn't break anything on the WebKitGTK port since the dev env is broken on both Guix and Nix (seems related to WebKitGTK).

aadcg avatar May 20 '24 20:05 aadcg