Opened 8 years ago

Closed 14 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 Imarok)

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)

keyfix.patch (2.7 KB ) - added by Imarok 8 years ago.
change SDL_HOTKEYDOWN to SDL_HOTKEYUP and move the text input part
keyfixv2.patch (3.5 KB ) - added by Imarok 8 years ago.
fixes #3870 but the inserted tchat text doesn't get sended anymore
keyfixv3.patch (3.0 KB ) - added by Imarok 8 years ago.
Now the chat message gets sent
keyfixv4.patch (6.5 KB ) - added by njm 8 years ago.
Improvement to Imarok's patch: Typing t in chat does not close the chat anymore. More information below.
keyfixv5.patch (6.5 KB ) - added by Imarok 8 years ago.
Now hotkeys shouldn't work in chat
textinputfix.patch (4.3 KB ) - added by echotangoecho 7 years ago.
Seems to work for linux, can't test for windows (probably doesn't work).

Download all attachments as: .zip

Change History (72)

comment:1 by Imarok, 8 years ago

Cc: jon12@… added
Description: modified (diff)

comment:2 by elexis, 8 years ago

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 of function toggleChatWindow() in menu.js is sufficient?

If it truly is a platform-dependent bug, it might appear in other places too though...

comment:3 by Imarok, 8 years ago

Cc: jon12@… removed

comment:4 by Imarok, 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 Imarok, 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?

Last edited 8 years ago by Imarok (previous) (diff)

comment:6 by sanderd17, 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 Imarok, 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 :(

Last edited 8 years ago by Imarok (previous) (diff)

comment:8 by elexis, 8 years ago

Do you still have alpha 19? It didn't happen there?

comment:9 by Imarok, 8 years ago

I installed the new alpha 20 3 hours ago :( Whether it happend in alpha 19 I don't know

Version 1, edited 8 years ago by Imarok (previous) (next) (diff)

comment:10 by Imarok, 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 sanderd17, 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 Imarok, 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 njm, 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 Imarok, 8 years ago

Another way would be to let the function wait until the key is unpressed.

comment:15 by Dummer, 8 years ago

with AutoIt this with be easy

comment:16 by elexis, 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 njm, 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.

comment:18 by Imarok, 8 years ago

I'm just a "noob" to the 0ad code, but this sounds good.

by Imarok, 8 years ago

Attachment: keyfix.patch added

change SDL_HOTKEYDOWN to SDL_HOTKEYUP and move the text input part

comment:19 by Imarok, 8 years ago

Keywords: review patch added
Milestone: BacklogAlpha 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 elexis, 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 Imarok, 8 years ago

Attachment: keyfixv2.patch added

fixes #3870 but the inserted tchat text doesn't get sended anymore

comment:21 by Imarok, 8 years ago

This patch fixes the bug but the inserted text doesn't get sent. I can't figure out why...

by Imarok, 8 years ago

Attachment: keyfixv3.patch added

Now the chat message gets sent

comment:22 by Imarok, 8 years ago

Keywords: review added

comment:23 by elexis, 8 years ago

Description: modified (diff)
Priority: Should HaveMust Have

comment:24 by elexis, 8 years ago

Keywords: review removed

Closes the chat window when typing t (on a gnu/linux machine) :-L

Last edited 8 years ago by elexis (previous) (diff)

comment:25 by Imarok, 8 years ago

I'll give it up :(

comment:26 by Imarok, 8 years ago

Keywords: patch removed
Summary: [PATCH]Opening team chat with key T automatically writes a t in input dialogOpening team chat with key T automatically writes a t in input dialog

in reply to:  25 ; comment:27 by njm, 8 years ago

Replying to Imarok:

I'll give it up :(

Ok, I'll try tomorrow.

in reply to:  27 comment:28 by Imarok, 8 years ago

Replying to njm:

Replying to Imarok:

I'll give it up :(

Ok, I'll try tomorrow.

Good luck ;)

by njm, 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 njm, 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 njm, 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 elexis, 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 with closeMenu()? The former does the same and closes also other stuff, so should be saner.
  • if( should be if (
  • 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 sanderd17, 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 elexis, 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, ...)

in reply to:  10 comment:34 by elexis, 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.

by Imarok, 8 years ago

Attachment: keyfixv5.patch added

Now hotkeys shouldn't work in chat

comment:35 by Imarok, 8 years ago

The new patch disables escape :(

comment:36 by elexis, 8 years ago

Priority: Must HaveRelease Blocker

Nominated for most annoying bugs of the last release. Doesn't need to stay, but should really be fixed.

comment:37 by Imarok, 8 years ago

r17658 is responsible for this bug. (I tested that)

Last edited 8 years ago by Imarok (previous) (diff)

comment:38 by elexis, 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:39 by elexis, 8 years ago

In 18384:

Chat cleanup, refs #3870.

Remove toggleChatWindow which duped openChat and closeChat.
Toggling the chat window with hotkeys is pointless due to the focus.
Inline setTeamChat.
Add some newlines, 2 missing semicolons and remove unneeded comments.

comment:40 by elexis, 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:

  1. Open chat
  2. change a file, so that the code will be hotloaded and the GUI reinitialized. The chat window will be invisible.
  3. Press T. It will add a "T" to the message box.

comment:41 by Andy Alt, 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.

Last edited 8 years ago by Andy Alt (previous) (diff)

comment:42 by Imarok, 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 Imarok, 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.

Last edited 8 years ago by Imarok (previous) (diff)

comment:44 by elexis, 8 years ago

Priority: Release BlockerMust 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 elexis, 7 years ago

Keywords: patch removed
Milestone: Alpha 21Alpha 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:48 by Imarok, 7 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 Imarok, 7 years ago

Keywords: beta added

by echotangoecho, 7 years ago

Attachment: textinputfix.patch added

Seems to work for linux, can't test for windows (probably doesn't work).

comment:50 by echotangoecho, 7 years ago

Patch didn't work on Windows, reported SDL bug: https://bugzilla.libsdl.org/show_bug.cgi?id=3499.

comment:51 by elexis, 7 years ago

Keywords: rfc added

comment:52 by elexis, 7 years ago

In 18990:

Disable SDL text input events before actually focusing a text input element. Patch by echotangoecho, refs #3870.

Thus don't add a "t" to the chat input after opening it with the hotkey on unix.
The bug still occurs on Windows due to an SDL bug reported upstream by echotangoecho.

comment:53 by elexis, 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 elexis, 7 years ago

Component: UI & SimulationCore engine
Keywords: patch added; beta removed
Resolution: fixed
Status: newclosed

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 leper, 7 years ago

Resolution: fixed
Status: closedreopened

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:56 by elexis, 7 years ago

Milestone: Alpha 22Backlog

Backlogging due to lack of progress

comment:57 by Imarok, 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 elexis, 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 line if(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 bb, 6 years ago

Patch: D1386

comment:60 by Itms, 5 years ago

Milestone: BacklogWork In Progress
Patch: D1386Phab:D1386

comment:61 by Silier, 3 years ago

Owner: set to bb
Status: reopenednew

comment:62 by Silier, 3 years ago

Resolution: fixed
Status: newclosed

comment:63 by Silier, 3 years ago

Resolution: fixed
Status: closedreopened

comment:64 by Imarok, 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:65 by bb, 2 years ago

In 26530:

Allied chat opens with t bug (on linux)

Comments By: vladislav, elexis
Solution Proposed By: elexis

fixes #5194
refs #3870

Differential Revision: D1386

comment:66 by bb, 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 Stan, 14 months ago

Milestone: Work In ProgressAlpha 26
Resolution: fixed
Status: reopenedclosed

Works for me on Windows

Note: See TracTickets for help on using tickets.