[PATCH] CInput doesn't follow the cursor on few events

[Vladislav Belov]

If you press Ctrl-V in the CInput and the buffer won't empty, then you will just insert it, but if the buffer is bigger than the CInput length or cursor_position + buffer_length > cinput_visible_length, then cursor will be out of CInput. Also it will paste in the invisible part after continuing of pasting.

How to reproduce:

1. Open the Multiplayer > Game Lobby.
2. Type sample text in the login field.
3. Copy it (Ctrl-C/Cmd-C) and paste it few times (Ctrl-V/Cmd-V).

[Vladislav Belov]

Adds the AutoScroll

[elexis]

Since cinput_length is not a variable found in the code, the ticket description should elucidate that cinput_length means the visible width of the GUI object in number of characters (and not the maximum number of characters typable there).

Considering this ticket and r18935, I'm expecting further missing UpdateAutoScroll(); occurances.

Looking at the code I could identify at least two cases: 1) SDLK_PAGEUP and SDLK_PAGEDOWN, even if we don't have any multiline input object yet. 2) Autocomplete with Tab (for example autoCompleteNick in gamesetup/session/lobby/replay menu.

One should take a brief look at UpdateAutoScroll() to see whether it could bug in edge cases as in that commit (likely doesn't, as the function is also called for example for empty text at other times).

max_length looks like something that also might bug in some cases as it is only taken into account in one place. Also reminds me of #3651.

Also copy & paste is broken on unix and even crashes sometimes: #3145.

[Vladislav Belov]

Replying to elexis:

Since cinput_length is not a variable found in the code, the ticket description should elucidate that cinput_length means the visible width of the GUI object in number of characters (and not the maximum number of characters typable there).

max_length is maximum number of character.

1) SDLK_PAGEUP and SDLK_PAGEDOWN, even if we don't have any multiline input object yet.

Fixed.

2) Autocomplete with Tab (for example autoCompleteNick in gamesetup/session/lobby/replay menu.

It should be solved not in the tab part, because JS handles it, but I'm fixing.

One should take a brief look at UpdateAutoScroll() to see whether it could bug in edge cases as in that commit (likely doesn't, as the function is also called for example for empty text at other times).

It works even for an empty string.

max_length looks like something that also might bug in some cases as it is only taken into account in one place. Also reminds me of #3651. Also copy & paste is broken on unix and even crashes sometimes: #3145.

There is no check when the paste size bigger than the max length. And it's not the goal of the ticket.

[Vladislav Belov]

Adds the AutoScroll on tab/paste/pageup/pagedown/dblclick

[elexis]

Don't forget to readd the keyword after fixing things.

Also I noticed that removing text also yields a similar issue. If you want to remove the last N words but only M<N words are visible, then one has to scroll before seeing the last N-M words.

[Vladislav Belov]

Adds the AutoScroll on tab/paste/pageup/pagedown/dblclick/remove

[Vladislav Belov]

Replying to elexis:

Don't forget to read the keyword after fixing things.

Thanks, just waited for another issues.

Also I noticed that removing text also yields a similar issue. If you want to remove the last N words but only M<N words are visible, then one has to scroll before seeing the last N-M words.

Fixed.

[Vladislav Belov]

Adds the AutoScroll on tab/paste/pageup/pagedown/dblclick/remove

[Vladislav Belov]

IRC:

10:48 < elexis> Vladislav2: did you test the autoscroll on tab? since tab sends an event and the gui event might be executed after the autoscroll call
10:54 < Vladislav2> Yes, it works for me
10:54 < elexis> might still be a race condition, but let me finish the review
10:58 < Vladislav2> But shouldn't
11:02 < elexis> UpdateAutoScroll in L594 should happen for cut, but not copy I guess, and that call again should go to DeleteCurSelection
11:02 < elexis> which also makes the L639 occurance unneeded
11:02 < elexis> might want to delete some unneeded braces if you want to do something in that line
11:04 < elexis> nope, looked at the wrong line number
11:08 < elexis> statement still holds despite different line nr apparently, as in all cases DeleteCurSelection will be called at last for the "text.delete.left" case
11:23 < Vladislav2> UpdateAutoScroll still needed
11:24 < Vladislav2> But it could be for all cases
11:29 < elexis> if you move the auto scroll to DeleteCurSelection, then 2 occurances are not needed anymore
11:32 < Vladislav2> elexis: do we need the Update for each DeleteCurSelection?
11:32 < elexis> seems reasonable, doesnt it?
11:36 < Vladislav2> If we delete the current selection, then it means that cursor already inside bounds
11:41 < elexis> apparently not in all cases
11:43 < Vladislav2> When selected yes, because cursor should be inside bounds
11:43 < Vladislav2> in all cases
11:44 < elexis> why do we need 594 and 639  then?
11:51 < elexis> 639 only because m_iBufferPos was changed I guess, but 594 seems useless then
11:57 < Vladislav2> elexis: then need to move to Delete or near
11:59 < elexis> Vladislav2: if you say we dont need it in DeleteCurSelection, then we don't need it at all for cut
12:09 < Vladislav2> elexis: don't need, yes
12:10 < elexis> good
12:10 < elexis> then next poiint: what if the multiline property is changed from JS? UpdateText is called then, but does that doesn't have autoscroll
12:10 < elexis> so should GUIM_LOAD, UpdateCachedSize and UpdateText also autoscroll, as the boundaries changed?
12:20 < elexis> but they shouldnt have to
12:23 < Vladislav2> elexis: nope, UpdateText is called after text input or setting changed, but there is a missed place: L829 should be out of current brackets, before break
12:24 < elexis> changing the text or settings sounds like scrolling would be needed
12:26 < Vladislav2> elexis: there are few lines when m_iBufferPos is changed after UpdateText call, so to have UpdateAutoscroll there isn't needed
12:28 < elexis> Vladislav: m_iBufferPos can also go out of bounds if the size of the gui object changed
12:28 < Vladislav> elexis: probably I didn't test it until it's not the goal
12:29 < Vladislav> but usually there is a min(m_ibufferpos, length);
12:29 < elexis> task of the ticket is to add missing scrolls
12:30 < Vladislav> but I've not noticed that m_ibufferpos go out of bounds
12:30 < Vladislav> UpdateAutoscroll solve the OOB
12:50 < WildfireBot> News from 0adsvn: Ticket #3403 ([PATCH] Show graphs in the summary screen) updated <http://trac.wildfiregames.com/ticket/3403#comment:37>
12:56 < Vladislav> elexis: so there is a choice: or add all properties (keys) at the one time, or add only needed, but by hands
13:15 < elexis> you mean the choice between adding UpdateAutoScroll to only the places where it is needed or to add them to a function? I'd say the latter, as calling the function doesn't hurt while being needed in some cases and it would also cover future edge cases, though I have to read again if adding only the required cases isn't more clean
13:21 < Vladislav> elexis: if doesn't hurt we could Update after all calls, in the event handler
13:21 < Vladislav> *could add

[elexis]

Thanks for the patch!

• Entry for tab not necessary in the engine, as that doesn't change the text, but if the JS page decides to do that it calls engine code which calls the updateautoscroll thing, so it's fine

• Ok with not calling UpdateAutoScroll from DeleteCurSelection as we call it after each occurance and as we'd still likely have to call UpdateAutoScroll afterwards as the text content might be further modified after deletion

• UpdateAutoScroll call after if (hotkey == "cut") not needed in the sense that the cursor is outside of the text field area, but it is still useful to have that call here as the now free space will be filled with the text that was previously invisible before the cursor (test: fill text field completely, cut some text)

• Thanks for linking the chatlog, that has helped with reviewing the patch! Notice the chatlog being from 2016-12-20-QuakeNet-#0ad-dev.log and the patch having addressed some of the comments, in particular the GUIM_LOAD entry

[elexis]

Fixed by r19234

[Itms]

In 19579:

Fix out of range access in CInput, patch by vladislavbelov. Fixes #4570.

Some invalid cursor positions were not checked, resulting in a crash in Debug mode. This was revealed by r19234, refs #4401.

Reviewed By: echotangoecho, Itms

Differential Revision: ​https://code.wildfiregames.com/D484