Page MenuHomeWildfire Games

Allied chat opens with t bug on linux
Needs ReviewPublic

Authored by bb on Mar 14 2018, 1:42 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Trac Tickets
#3870
Summary

Sometimes when opening the allies chat for the first time there already is a "t" present (how many times we not say "t9"). Here is a way to reliable reproduce the issue at my machine:

Host a game
Join with another player => press ready => type something in gamesetup chat so the chat is focused (with joiner)
Start game
press "t" with joiner

This is due to SDL_TextEvent being set when we focus the chat, but never stopped when switching pages, so SDL still passes the textevent to the input field and thus creates a "t". This is fixed by explicitly stopping the textEvent when switching gui pages using the Destroy function.

Test Plan

Use the above procedure to reproduce the bug and see it resolved.
Also use L hotkey (private chat) and notice no problems

Agree that when switching gui pages a text input needs to be refocused before typing (ofc a page could auto focus)

Test on windows too (there still is the SDL bug though)

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5587
Build 9412: Vulcan BuildJenkins
Build 9411: arc lint + arc unit

Event Timeline

bb created this revision.Mar 14 2018, 1:42 AM
Vulcan added a subscriber: Vulcan.Mar 14 2018, 5:07 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/242/display/redirect

bb updated this revision to Diff 6166.EditedMar 14 2018, 11:15 AM

years and comment

To be more precise in what happens in the code:

When the joiner focusses the gamesetup chat SDL_StartTextInput is send so he can start typing, but as he doesn't touch anything the chat is still focussed when the host pressed the "Game Start" button (this also implies the bug won't occur for the host as the action of pressing the button will unfocus the chat). So for the joiner SDL_TextEvents are still send! Thus when the joiner then presses "t" ingame, SDL sends two events:
SDL_KEYDOWN => opens the chat window and so focussus the input field (so the SDL gets the rectangle passed were to put the text, and is open for TEXTEVENTS).
SDL_TEXTEVENT => puts a "t" in the input field

A next opening of the chat won't trigger the TEXTEVENT since onClose the input field is unfocussed. Thus the TEXTEVENTS not being send until it focusses again which happens after the act of pressing "t" (as the happens on the first KEYDOWN), thus SDL doesn't send the TEXTEVENT.
This same thing is used here to fix the issue: not sending the SDL_TEXTEVENT by stopping the query when switching gui pages.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/244/display/redirect

bb added a comment.Mar 14 2018, 12:21 PM

The code from rP18990 actually is correct and needed too, but didn't fix the entire issue, since it only fixes the issue when no input field was focussed. But the code there is still required since if we end up on a page where we can open an input field with a hotkey, without prior switching a gui page (f.e. some togglable input field on main-menu) the TXTEVENT shouldn't be send either, thus we need to break that SDL default to our own.

It seems the bug still present in SDL: https://bugzilla.libsdl.org/show_bug.cgi?id=3499. The diff looks more like a hack. @echotangoecho had a patch for the SDL to fix it. But I agree to commit it for the release, if my comment about parallel fields isn't reproducible.

source/gui/CInput.cpp
2157

I think, it may lead to weird behaviour with GUI (not sure that it exists in the public, but it's real in mods), when a player entering a chat text and at the same time some text field is closing. I.e. a temporal message box with the CInput. Did you test it?

vladislavbelov added inline comments.Mar 14 2018, 6:27 PM
source/gui/CInput.h
136

Who is calling this?

elexis added a subscriber: elexis.Mar 14 2018, 6:29 PM
elexis added inline comments.
source/gui/CInput.cpp
2157

Isnt this only called when the GUI page is irrecoverably closed and deleted in c++?

So the only way this to be wrong is having a text field focused in page 1, then opening page 2 that has a text field, closing page 2, then the textfield in page 1 might still be focused without the SDL_StartTextInput thign being called?

bb updated this revision to Diff 6169.Mar 14 2018, 10:55 PM

It seems the bug still present in SDL: https://bugzilla.libsdl.org/show_bug.cgi?id=3499. The diff looks more like a hack. @echotangoecho had a patch for the SDL to fix it.

I wonder I how the SDL is considered bugged here (I read the report ofc): if the textInput query is on it should send the textevents, no matter if there is another thing happening on KEYDOWN for the same key. Actually the same happens in MP gamesetup when pressing alt+s/w (move tab and write text) and also that is more of a 0ad bug than an SDL.
Also the SDL patch is just a windows fix and this bugs on linux too. Adding that check as proposed also won't have an effect on this issue since the check would return true is this case.

source/gui/CInput.cpp
2157

Well probably even easier: have two pages open simultaneously, focus a textfield on one, close the other (without dropping focus)

That is a very hypothetical case however, since closing a page without dropping focus would be meh. But for the sake of completeness we could add a check for being focussed on this inputfield (we would still bug then if both fields have a inputfield which both are focussed, but considering that a bug already)

source/gui/CInput.h
136

Called from the CGUI destroy function.

elexis added inline comments.Mar 14 2018, 11:02 PM
source/gui/CInput.cpp
2157

There is not really a situation where two GUI pages are open and processed simultaneously.
You can only switch or push a GUI page and every GUI page is running with it's own ScriptInterface, so they are entirely agnostic of each other.
For instance remove the fade from the AI config dialog and open that in the gameseutp. The gamesetup page doesn't receive events anymore.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/246/display/redirect

It doesn't fix the problem for Windows 10.

source/gui/CInput.cpp
44–45

It's the 112 characters long for no reason.

source/gui/CInput.h
135

We use upper camel case with the m_* prefix. It should be m_Focus.

bb updated this revision to Diff 6180.Mar 15 2018, 10:57 PM
bb retitled this revision from Allied chat opens with t bug to Allied chat opens with t bug on linux.
bb edited the test plan for this revision. (Show Details)

Don't entirely like that new function name but nothing better came up (onActivatePage, Activate, PageActivated, GuiPageActivated, ActivateGuiPage)

It doesn't fix the problem for Windows 10.

SDL bug

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/254/display/redirect

bb updated this revision to Diff 6192.Mar 17 2018, 12:48 AM

fix the case two input fields are present and the first is focussed

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/261/display/redirect

elexis added inline comments.Dec 27 2018, 1:27 PM
source/gui/CInput.cpp
78

This reference to the top-page sounds investigateable.

It may not always the topmost page that that creates the CInput object.
For example a JS script that calls PushGUIPage will continue to run in the parent GUI page for the lifetime of the function call.
Equally the onFoo GUI events.

Would a reference to the GUI page that owns this object be more precise / universal / reliable?

1100

Perhaps we don't need the "activate" function at all, but just call SDL_StartTextInput(); here (perhaps only if the GUI page is the topmost one too)?

source/gui/GUIManager.cpp
101

;;

305

Removing this proxy sounds safer, requiring the caller to actively figure out which GUI page it wants to operate on.
For example m_PageStack.back()->gui->ActivatePage();.
(In general it would be appropriate to minimize the GUI manager as far as possible and operate directly with the available GUIPages where possible)

source/gui/GUIManager.h
98

It's not clear what "activate" means.

bb marked 3 inline comments as done.Dec 27 2018, 3:00 PM
bb added inline comments.
source/gui/CInput.cpp
78

certainly investigation

It doen't matter that the topmost page has the CInput object or not, but I would expect any other page than the top page would not recieve textInputs, even if it is the only page with a textinput.

What we want to achieve with this call is that whatever happens, the the SDL_TEXTINPTUT thingy is set correctly for the topmost page, so every time a gui page switches we need to check it (whether it is pushing, switching or popping a page, doesn't matter). So I guess these two calls could get a better location.

1100

well no, that call already is there 12 lines below...

source/gui/GUIManager.h
98

(there is some comment in the history saying I dislike the names)

elexis added inline comments.Dec 27 2018, 3:09 PM
source/gui/CInput.cpp
1100

Then one could consider whether sending the GUIM_GOT_FOCUS message upon pagestack changes would be more versatile or shorter. (I'm uninformed)

elexis added inline comments.Dec 27 2018, 3:20 PM
source/gui/CGUI.cpp
335

This sounds like it could cost some performance / doing more work than necessary.

One can get the focused object of a GUI page with a getter, the function only operates on that one.

source/gui/CInput.cpp
77

the old field is still focussed

Which could be fixed in the previously focused object, when it loses the focus?

Shouldn't this just be if m_Focused then SDL_StartTextInput?

Does calling the stop then the start function differ from noop?
The comment suggests that the SDL function only operates on one GUI Object, but the function is global.

78

It doen't matter that the topmost page has the CInput object or not

If the topmost page differs from the the page that owns this object, then the call to the topmost page would be unrelated to this GUI object instance, no?

If m_pGUI is not used here, there should be a very good reason for a GUIObject of page X to do something for GUI page Y.