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

Fix Issue: WASM, Reading console input does not work #360

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

federicovilla55
Copy link
Contributor

In the current WebAssembly (WASM) version of Ripes there's an issue preventing input reading from the console. This because pressing Enter does not return control to the processor, as stated in issue #304.

The problems stems from the behaviour of the Console::keyPressEvent(QKeyEvent *e) method, which requires a QKeyEvent parameter, that appears to behave differently in the binary executable compared to the WASM version.
In the binary executable, when pressing Enter or Return, the QKeyEvent associated with the keys Qt::Key_Return or Qt::Key_Enter has a non empty text field (e->text()). However, in the WASM version that field is empty and therefore the check if (!e->text().isEmpty()) always returns false when pressing Enter, resulting in the already reported error.
The same happens with the Backspace Key.

Proposed fix

I propose altering the handling of these events: rather than mandating a non-empty text field in the QKeyEvent, I suggest to verify only the key associated with each command. Text inspection should only occur for the events where the key is not checked.

As stated in the Qt documentation of the QKeyEvent Class of key():

Returns the code of the key that was pressed or released.
See Qt::Key for the list of keyboard codes. These codes are independent of the underlying window system.

The key() of an event is independent from the underlying system while the text() may not.

In the modified code, all types of QKeyEvent are processed using the same switch-case structure, as currently happens for the directional keys. When a key event is triggered, the program checks if the key corresponds to any of the arrow keys, the Enter key, the Return key or the Backspace key.
If the key does not match any of these, the event is handled as it used to: first, it checks that the content of the key event is not empty, ensuring that modifier keys such as Shift, Control, or Alt are not considered. Then, it adds the content of the event to the buffer and calls the putData() method.

The issue mortbopet#304 is related to WASM console input not working, as by pressing Enter, Return or Backspace key nothing happens. This was due to how those commands were handled: first it was checked that their text was non empty, then their keys were checked with the elements of the `Qt::Key` enumeration. The problem is that in WebAssembly the events related to those keys have an empty text, therefore the reported problem. I modified the logic behind the evaluation of those commands: in the `Console::keyPressEvent(QKeyEvent *e)` method there is an enumeration that used to handle just the arrow keys and the default case; I propose to handle the Return, Enter and Backspace keys differently from the rest of the commands, without checking if their related text is empty.
@federicovilla55 federicovilla55 changed the title Fix Issue: *WASM, Reading console input does not work* Fix Issue: WASM, Reading console input does not work May 2, 2024
@mortbopet
Copy link
Owner

sorry for the delay; been on vacation for the past few weeks.

Thank you for looking into this - awesome that we're going to have a fix for this in WASM! have you tested this by running a local WASM build and (Linux | Windows) build to ensure that it works in both scenarios?

@federicovilla55
Copy link
Contributor Author

I've tested the PR with both local WASM and Linux/Windows builds, and it works in all cases.
Let me know if there are any other scenarious I should check.

@mortbopet
Copy link
Owner

@federicovilla55 sorry for the delay (feel free to ping me if i'm not replying)... awesome - we'll merge it :)

@mortbopet mortbopet merged commit f6627af into mortbopet:master Aug 12, 2024
6 checks passed
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.

2 participants