Opened 8 years ago
Closed 15 months ago
#3870 closed defect (fixed)
SDL 2.0.4 update broke hotkeys on windows (Opening team chat with key T automatically writes a t in input dialog)
Reported by: | Imarok | Owned by: | bb |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 26 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: | Phab:D1386 |
Description (last modified by )
On windows, opening the team chat window with key T automatically writes a t in input dialog A Patch could be to open the team chat window not until the t key has been released.
Testing on Windows revealed that the SDL 2.0.4 update in r17658 introduced this behavior.
Attachments (6)
Change History (72)
comment:1 by , 8 years ago
Cc: | added |
---|---|
Description: | modified (diff) |
comment:2 by , 8 years ago
comment:3 by , 8 years ago
Cc: | removed |
---|
comment:4 by , 8 years ago
" but only when the dialog is opened the first time": When you mean the bug only appears the first time you open team chat via "t" and after sending a message and reopen team chat with "t" the bug doesnt occur: thats wrong. The bug always appears. I'm using windows.
comment:5 by , 8 years ago
I will try it after downloading visual c++ and the source. ;) Do you mean the menu.js in: ps/trunk/binaries/data/mods/public/gui/session/menu.js?
comment:6 by , 8 years ago
There's no need to use visual C++. The changes should be done in the scripts (*.js files), so you don't have to compile those. And we offer a precompiled version of the game for windows.
So you should download the source, then modify that file in a good text editor (like notepad++), and then you can just run it from the precompiled binary (under binaries/system).
comment:7 by , 8 years ago
Seems logic :D
I changed the menu.js at binaries\data\mods\public\gui\session to
function toggleChatWindow(teamChat) { if (g_Disconnected) return; let chatWindow = Engine.GetGUIObjectByName("chatDialogPanel"); let chatInput = Engine.GetGUIObjectByName("chatInput"); let hidden = chatWindow.hidden; closeOpenDialogs(); if (hidden) { setTeamChat(teamChat); chatInput.focus(); } else { if (chatInput.caption.length) { submitChatInput(); return; } chatInput.caption = ""; } chatWindow.hidden = !hidden; chatInput.caption = ""; }
started 0ad with binaries/system/pyrogenesis.exe and the bug still remains :(
comment:9 by , 8 years ago
I installed the new alpha 20 3 hours ago :( I don't know if it happend in alpha 19
follow-up: 34 comment:10 by , 8 years ago
I reinstalled alpha 19 now. There the bug only occurs the first time in the running time of the program. In alpha 20 it occurs always.
comment:11 by , 8 years ago
Imarok, can you add a line like
warn("it works");
to that file (next to the line you added), just to assure that the modifications were read (and the game isn't reading the scripts from some other installation. You should see a yellow "It works" line appearing then every time you open the chat window.
comment:12 by , 8 years ago
I added this line, the warning came when pressing t but the bug stayed. I noticed pressing Ctrl+T opens the team chat window without writing t into the input box
comment:13 by , 8 years ago
I think the function toggleChatWindow in menu.js should consume the key event to stop its propagation/bubbling. But I don't know how (yet).
comment:14 by , 8 years ago
Another way would be to let the function wait until the key is unpressed.
comment:16 by , 8 years ago
To me this looks like CGUI.cpp
sending the press
event on SDL_HOTKEYDOWN
, while it should be SDL_HOTKEYUP
, as the latter one also waits for the button to be released.
Changing this however closes the chatbox again when typing t.
comment:17 by , 8 years ago
I suggest to only open the chat window with the hotkey on SDL_HOTKEYUP (as suggested by elexis) and close the chat window when a message is submitted or the dialog is cancelled. The second part is already implemented. The first part would mean to rename toggleChatWindow to openChatWindow and only open the chat window, if it isn't open. Additionally submitChatInput in messages.js should call closeChat in menu.js instead of toggleChatWindow.
The "else"-part in toggleChatWindow seems to be obsolete anyway, because the chat dialog in session.xml uses the function submitChatInput directly.
Do you agree? If so, I can do it.
by , 8 years ago
Attachment: | keyfix.patch added |
---|
change SDL_HOTKEYDOWN to SDL_HOTKEYUP and move the text input part
comment:19 by , 8 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 21 |
Summary: | Opening team chat with key T automatically writes a t in input dialog → [PATCH]Opening team chat with key T automatically writes a t in input dialog |
comment:20 by , 8 years ago
Keywords: | simple review removed |
---|
Doesn't work fully here (linux):
- Escape doesn't close the chat dialog
- Enter sends the chat and opens it again
Also when moving code, the code should be optimized:
- operators like
||
should appear in the end of a line - those 2 ifs could be merged
Not sure if the comment would remain true after moving // else will return IN_PASS because we never used the button
But it seems that code path you moved is the one being triggered incorrectly.
by , 8 years ago
Attachment: | keyfixv2.patch added |
---|
fixes #3870 but the inserted tchat text doesn't get sended anymore
comment:21 by , 8 years ago
This patch fixes the bug but the inserted text doesn't get sent. I can't figure out why...
comment:22 by , 8 years ago
Keywords: | review added |
---|
comment:23 by , 8 years ago
Description: | modified (diff) |
---|---|
Priority: | Should Have → Must Have |
comment:24 by , 8 years ago
Keywords: | review removed |
---|
Closes the chat window when typing t (on a gnu/linux machine) :-L
comment:26 by , 8 years ago
Keywords: | patch removed |
---|---|
Summary: | [PATCH]Opening team chat with key T automatically writes a t in input dialog → Opening team chat with key T automatically writes a t in input dialog |
follow-up: 28 comment:27 by , 8 years ago
comment:28 by , 8 years ago
by , 8 years ago
Attachment: | keyfixv4.patch added |
---|
Improvement to Imarok's patch: Typing t in chat does not close the chat anymore. More information below.
comment:29 by , 8 years ago
Information regarding above keyfixv4.patch: I used the last patch of Imarok (thanks!), that altered C++ files, and I altered JavaScript files.
- Typing a t in chat now does not close the chat anymore.
- Renamed toggleChatWindow to openChatWindow, because it is/was only used for that. Altered containing code.
- Deleted openChat, that was called when opening the chat window via menu, and use openChatWindow instead. Like this, the chat input is not deleted and now focused, when clicking Chat in the menu.
- Further improvement would be to set the cursor to the end of the chat input, when the Chat button in the menu is clicked.
comment:30 by , 8 years ago
Keywords: | review patch added |
---|---|
Summary: | Opening team chat with key T automatically writes a t in input dialog → [PATCH] Opening team chat with key T automatically writes a t in input dialog |
comment:31 by , 8 years ago
Awesome, the patch works, great teamwork!
Can someone recapitulate what the c++ change does and which of these JS changes are crucial to fix the bug?
Now to the codestyle-nitpicking:
- Why replace the call to
closeOpenDialogs
withcloseMenu()
? The former does the same and closes also other stuff, so should be saner. if(
should beif (
- agreing that toggleChat should be replaced with openChat and closeChat
- closeChat()` -> an early return would be better with regards to codestyle. But why add the check in the first place? If you don't want to clear chat in both cases (opening&closing), then completely remove the duplicate clears (and test that it doesnt introduce bugs).
- Make a variable for the GUI object
chatInput
as well, the clear-chat comment is not needed - As mentioned by Imarok, funciton names should be consistent (either both or none of them should contain "Window")
comment:32 by , 8 years ago
Just tested the patch on Linux, and it indeed got rid of the annoying "t" (which I only got on the first opening of the dialog). And I didn't notice any negative side effects.
So functionality-wise, this is fine IMO, and I'll leave it to elexis to do the code review.
comment:33 by , 8 years ago
Keywords: | review removed |
---|
(Most) hotkeys shouldn't work if chat is focused. For example whenever the user types the idle-worker-hotkey (dot), then an idle worker will be selected. The action was not intended though, as the user only wanted to add text. Another example is selecting a CC and then typing a queue hotkey, (Y/Z, X, C, V, ...)
comment:34 by , 8 years ago
Replying to Imarok:
I reinstalled alpha 19 now. There the bug only occurs the first time in the running time of the program. In alpha 20 it occurs always.
So find the revision that introduced the platform-specific difference. It was somewhere between r17299 (alpha19) and r17965 (alpha 20).
As SDL is responsible for input, maybe it was r17658?
Boot windows, revert your stuff, do svn update -r <revision>
and compile the code.
You only need a logarithmical amount of tries to find the commit. If the bug occurs on rev X but not rev Y, then check for the revision right between them. Thus you divide the number of commits to check by 2 everytime.
comment:36 by , 8 years ago
Priority: | Must Have → Release Blocker |
---|
Nominated for most annoying bugs of the last release. Doesn't need to stay, but should really be fixed.
comment:37 by , 8 years ago
r17658 is responsible for this bug.
comment:38 by , 8 years ago
Description: | modified (diff) |
---|---|
Summary: | [PATCH] Opening team chat with key T automatically writes a t in input dialog → [PATCH] SDL 2.0.4 update broke hotkeys on windows (Opening team chat with key T automatically writes a t in input dialog) |
comment:40 by , 8 years ago
It is certain that CGUI::HandleEvent
receives an SDL_TEXTINPUT
the first time pressing T to open the teamchat on linux (everytime on windows) but never receives SDL_TEXTINPUT
the second time to open the dialog with that hotkey on linux. Also suspecting a focus issue.
Found something by accident:
- Open chat
- change a file, so that the code will be hotloaded and the GUI reinitialized. The chat window will be invisible.
- Press T. It will add a "T" to the message box.
comment:41 by , 8 years ago
I've seen this recently, on another project. The devs also decided it had something to do with libSDL 2.0.4.
No fix found yet that I know of, sorry to say. Besides being reported by a small number of users, it happened to my nephew on a laptop running Windows 10. I booted my desktop computer to Win 10, and it did not reproduce. It was referred to as double-toggling (you get 2 key presses for the price of 1).
My suggestion for a work-around: Check if 't' was pressed .001 second ago. If so, ignore this t. Would something like that work or has it already been tried?
I realized something very strange. On the other game, team chat was the only thing that was affected, iirc. The team chat functioned differently however; pressing a key would toggle to team chat, but enter
had to be pressed to open up a line to type in a chat message. So basically what happened was team chat would be toggled to on, then immediately toggled to off.
So the method 0 A.D. is using to use team chat is completely different, but this bug is randomly happening only when using team chat, if I'm understanding it correctly.
Almost as if changing the destination recipients somehow was tied in.
And searching Google and Bing yields no results for a similar bug. Maybe a few that are vaguely related or have to do with SDL < 2.0.3.
comment:42 by , 8 years ago
The bug is not only limited to teamchat: When you set the chat hotkey to some letter(eg. "L") the bug appears too.
comment:43 by , 8 years ago
The problem is when pressing "T" to open the team chat, CGUI::HandleEvent
sometimes recieves a SDL_TEXTINPUT event and sometimes not. 't' is only printed into the inputbox when it recieves this event.
So I guess the problem lies in the SDL library and not inside the 0AD code.
comment:44 by , 8 years ago
Priority: | Release Blocker → Must Have |
---|
Can be pushed to alpha 22 if we can't find the time to fix it, since it's not a game breaking bug. Should still be considered a high priority task.
comment:46 by , 8 years ago
Keywords: | patch removed |
---|---|
Milestone: | Alpha 21 → Alpha 22 |
Summary: | [PATCH] SDL 2.0.4 update broke hotkeys on windows (Opening team chat with key T automatically writes a t in input dialog) → SDL 2.0.4 update broke hotkeys on windows (Opening team chat with key T automatically writes a t in input dialog) |
comment:47 by , 8 years ago
comment:48 by , 8 years ago
SDL 2.0.5 doesn't seem to fix this issue. (I replaced the .dll and the headers and recompiled)
comment:49 by , 8 years ago
Keywords: | beta added |
---|
by , 7 years ago
Attachment: | textinputfix.patch added |
---|
Seems to work for linux, can't test for windows (probably doesn't work).
comment:50 by , 7 years ago
Patch didn't work on Windows, reported SDL bug: https://bugzilla.libsdl.org/show_bug.cgi?id=3499.
comment:51 by , 7 years ago
Keywords: | rfc added |
---|
comment:53 by , 7 years ago
Keywords: | rfc removed |
---|
Thanks for finally nailing it down echotangoecho! I guess the only thing remaining to do here is waiting for the SDL guys to fix the Windows part.
The diff above was tested on Windows by Vladislav and Imarok.
comment:54 by , 7 years ago
Component: | UI & Simulation → Core engine |
---|---|
Keywords: | patch added; beta removed |
Resolution: | → fixed |
Status: | new → closed |
The one who has the energy to spend on writing a 0AD specific workaround to the SDL bug can reopen the ticket.
Other than that I have to thank echotangoecho again for fixing it on linux , writing a minimal testcase with SDL and reporting that upstream!
comment:55 by , 7 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Given that this is still broken on Windows closing this seems wrong. Close this once upstream fixes this, ships a new version and we ship with that on Windows.
Also just dropping bug reports upstream without following up on those might not result in a quick fix, also one can try fixing things in upstream projects.
comment:57 by , 7 years ago
Description: | modified (diff) |
---|
What about providing a patched sdl binary for win, as it seems like the sdl devs won't fix this bug in near future?
comment:58 by , 7 years ago
As Imarok pointed out, there was a potential fix mentioned in http://irclogs.wildfiregames.com/2016-11-22-QuakeNet-%230ad-dev.log
:
19:23 < echotangoecho> next there is a potential fix for the SDL code: in
src/video/windows/SDL_windowsevents.c
try adding the single lineif(GetEventState(SDL_TEXTINPUT) == SDL_ENABLE)
above line 1016, so
TranslateMessage(&msg)
is in the if statement and try compiling it and 0ad again and see whether this fixes the bug
comment:59 by , 6 years ago
Patch: | → D1386 |
---|
comment:60 by , 5 years ago
Milestone: | Backlog → Work In Progress |
---|---|
Patch: | D1386 → Phab:D1386 |
comment:61 by , 3 years ago
Owner: | set to |
---|---|
Status: | reopened → new |
comment:62 by , 3 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:63 by , 3 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:64 by , 2 years ago
For the record:
echotangoecho's ticket at SDL's bugzilla has been moved to github: https://github.com/libsdl-org/SDL/issues/2312
Some time later it has been marked as a duplicate of https://github.com/libsdl-org/SDL/issues/4491 which has been closed as invalid.
Afaics it's not really a duplicate but I'm not sure and don't have the time to really look into it.
comment:66 by , 2 years ago
Can someone on windows test if the bug is still there and report the results here? The linux issue (which also exists on windows) of the persisting SDL flag is fixed by the above commit.
Make sure to test SP, MP (host + client) etc.
comment:67 by , 15 months ago
Milestone: | Work In Progress → Alpha 26 |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
Works for me on Windows
I heard this from others too, but I can't reproduce it. According to sanderd17, it has been like that since some time, but only when the dialog is opened the first time. Maybe it is a platform-dependent bug?
Can someone check if adding
chatInput.caption = "";
to the end offunction toggleChatWindow()
inmenu.js
is sufficient?If it truly is a platform-dependent bug, it might appear in other places too though...