Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Refactor SDL input code to fix menu exit #146

Merged
merged 6 commits into from
Feb 18, 2023
Merged

Conversation

ndren
Copy link
Contributor

@ndren ndren commented Nov 21, 2022

Continued from #144, includes KEY_RETURN.

@ndren ndren changed the title Add back isNoUnicodeKey to fix menu exit in SDL Refactor SDL input code to fix menu exit Nov 21, 2022
@ndren
Copy link
Contributor Author

ndren commented Nov 22, 2022

@paradust7 Please let me know if you have some feedback for this approach. Thanks!

@ndren ndren mentioned this pull request Nov 22, 2022
8 tasks
@ndren
Copy link
Contributor Author

ndren commented Nov 26, 2022

This PR is ready for review. I've fixed all the regressions I could find.

include/IGUIEnvironment.h Outdated Show resolved Hide resolved
@sfan5
Copy link
Member

sfan5 commented Nov 28, 2022

Why are we adding a new, different method now? We have a working IME model that (as far as I can tell) SDL just needs to use, please read #144 (comment).

@Desour
Copy link
Member

Desour commented Nov 28, 2022

Why are we adding a new, different method now? We have a working IME model that (as far as I can tell) SDL just needs to use

Sorry, that was an unnecessary idea by me, I was stupid and didn't read carefully enough.
The idea was that the driver could be more independent of any gui systems by offering a function that en-/disables text-input, and that key events are not fired for text keys if in text input mode.

please read #144 (comment).

If I understand correctly, starting and stopping SDL text input doesn't solve the issue. It just en-/disables the extra text events. But the issue is that there are also key events that cause keybindings in minetest to be triggered, i.e. inventory key. According to libsdl-org/SDL#2897 (and libsdl-org/SDL#4739), text input is on by default anyway.

@ndren
Copy link
Contributor Author

ndren commented Nov 28, 2022

@sfan5 I went back to the original plan, thanks. Let me know how it looks now.
@Desour Although disabling/enabling does not do anything by itself, it does mean that the keyboard shortcut events for unicode keys (including the one for the inventory!) can be thrown away when something is in focus (so the other unicode handler deals with it), hence solving the issue.

@ndren
Copy link
Contributor Author

ndren commented Nov 28, 2022

Side note: CI decided not to run (cancelled itself) on macos. Not sure why or how to fix.

@ndren
Copy link
Contributor Author

ndren commented Dec 2, 2022

I squashed the commits together, hopefully now CI decides to run.

@ndren
Copy link
Contributor Author

ndren commented Dec 5, 2022

CI builds well now, let me know if there's anything else to be done.

@sfan5 sfan5 self-requested a review December 23, 2022 16:51
source/Irrlicht/CIrrDeviceSDL.cpp Outdated Show resolved Hide resolved
source/Irrlicht/CIrrDeviceSDL.h Outdated Show resolved Hide resolved
@ndren
Copy link
Contributor Author

ndren commented Jan 7, 2023

@sfan5 I've had to remove const when adding static, let me know if that works.

@sfan5
Copy link
Member

sfan5 commented Feb 6, 2023

@Desour if you have the time, can you confirm for me whether this works?

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

Tested.
Works fine otherwise.

source/Irrlicht/CIrrDeviceSDL.cpp Show resolved Hide resolved
@Desour
Copy link
Member

Desour commented Feb 7, 2023

I've had to remove const when adding static, let me know if that works.

Btw. just FYI, @ndren, the const after a member function is for the implicit this ptr (and hence also all non-static class member variables), which doesn't exist for static member functions.

@ndren
Copy link
Contributor Author

ndren commented Feb 7, 2023

@Desour Thanks for the clarification and testing!

@ndren
Copy link
Contributor Author

ndren commented Feb 8, 2023

@Desour Can you confirm this works with the new SDL check included?

@Desour
Copy link
Member

Desour commented Feb 8, 2023

👍 Works.
(Code is also fine, btw..)

@sfan5 sfan5 merged commit cd3e784 into minetest:master Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants