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

Electron input handling #3381

Merged
merged 6 commits into from
May 20, 2024
Merged

Electron input handling #3381

merged 6 commits into from
May 20, 2024

Conversation

jmercouris
Copy link
Member

@jmercouris jmercouris commented Apr 5, 2024

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
Copy link
Member Author

@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 :-)

@aadcg
Copy link
Member

aadcg commented Apr 5, 2024

@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 aadcg removed their request for review April 5, 2024 07:46
@jmercouris
Copy link
Member Author

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

@jmercouris
Copy link
Member Author

@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?

@aadcg
Copy link
Member

aadcg commented Apr 5, 2024

I'm not sure I understand the consequences of each of the 2 approaches. The one from commit 519d380 seems closer to how it works in GTK. The other one, from commit d058c17, 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 d058c17 superior to 519d380?

@aadcg aadcg force-pushed the electron-input-handling branch 2 times, most recently from 1d23810 to d7af2fc Compare May 20, 2024 19:41
@aadcg aadcg marked this pull request as ready for review May 20, 2024 20:00
@aadcg
Copy link
Member

aadcg commented May 20, 2024

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 and others added 6 commits May 20, 2024 23:11
Otherwise, it wouldn't be possible to execute JavaScript when handling input
from the renderer (Electron port).

In more detail:

- JavaScript waits for a synchronous response from Nyxt 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().

- 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.

- Our Lisp program will not continue until the JavaScript we tried to execute
runs.

- Our Lisp program will be unable to return t/nil.

- We have deadlocked our program.
cl-electron must be located in a path where ASDF sees it.  Note that the all
non-lisp dependencies are now handled by Node.js.

Reverts commit a39942d.
Handles both the WebKitGTK and Electron ports.
@aadcg aadcg merged commit 63fa20d into master May 20, 2024
3 checks passed
@aadcg aadcg deleted the electron-input-handling branch May 20, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants