Page MenuHomeWildfire Games

Add readonly mode to the CInput
ClosedPublic

Authored by vladislavbelov on Aug 1 2017, 12:05 AM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#4225
Summary

Added readonly mode to the CInput, useful for replays and other places with some system information, because it allows to copy the text without changes.

Test Plan
  1. Run game
  2. Open replays
  3. Select any replay
  4. Try to change the text

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 2680
Build 4570: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.Aug 1 2017, 12:05 AM
Vulcan added a subscriber: Vulcan.Aug 1 2017, 12:06 AM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1789/
See console output for more information: http://jw:8080/job/phabricator/1789/console

0ad-project/binaries/... That's not going to apply cleanly when someone tries to test your patch. Please either use arc (described in https://trac.wildfiregames.com/wiki/Phabricator#UsingArcanist), or at the very least ensure that you create the patch using the directory that has binaries/ and source/ as the root.

0ad-project/binaries/... That's not going to apply cleanly when someone tries to test your patch. Please either use arc (described in https://trac.wildfiregames.com/wiki/Phabricator#UsingArcanist), or at the very least ensure that you create the patch using the directory that has binaries/ and source/ as the root.

Oh, thank you! I didn't notice it, patcher selected wrong root, I'll fix it soon. Arc isn't good for me, because it crashes (exceptions) very often.

Fix contexts.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1791/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1792/ for more details.

Imarok added a subscriber: Imarok.EditedAug 3 2017, 1:22 AM

I think you should disable the cursor or allow using arrow keys to navigate and mark text.
Else looks very nice ;)
(Hoping the next thing while be placeholder text ;P)

elexis added a subscriber: elexis.

Rest works as advertized (including changing the "readonly" property from JS) and ok with me. Nice patch!

binaries/data/mods/public/gui/replaymenu/replay_menu.js
274 ↗(On Diff #2996)

Why was escapeText removed?
As far as I know it's required for windows path separators, see rP17134.
This was converted to JS escapeText with r19491, so rP19650 used it as well.

Setting caption = "foo\bar" shows an unprintable character here, with and without escapeText.

Should be tested on windows.

source/gui/CInput.cpp
544 ↗(On Diff #2996)

No else after return, but \n

source/gui/CInput.h
178 ↗(On Diff #2996)

// Something
Perhaps move the other comment here.

vladislavbelov marked an inline comment as done.Aug 10 2017, 9:46 PM
In D763#30083, @Imarok wrote:

I think you should disable the cursor or allow using arrow keys to navigate and mark text.
Else looks very nice ;)

I'll disable the cursor.

(Hoping the next thing while be placeholder text ;P)

I want to make a placeholder too.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
274 ↗(On Diff #2996)

It's tested and it works. Because the input shows the raw string as is.

vladislavbelov marked an inline comment as done.

Do not draw the cursor if the input is readonly

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1835/ for more details.

elexis edited edge metadata.Aug 11 2017, 10:31 PM
In D763#30083, @Imarok wrote:

I think you should disable the cursor or allow using arrow keys to navigate and mark text.
Else looks very nice ;)

I'll disable the cursor.

I'm not sure about the cursor.
Readonly should mean that we can do everything except writing to the element.
That includes navigating with arrow keys, home, end, super+arrow keys, skipping words with control and selecting text with shift+arrowkeys.
That is possible with writable elements, so why should it be impossible with readonly ones?
Mostly finding this odd because it's possible to select only parts of the word with the mouse, so the cursor position is still noticeable, it's just hidden.

On windows and common browsers, the behavior is the same, for example:
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml_input_readonly

You can hide the cursor if you want, but I'd keep the readonly operations accessible.

@Imarok agrees?

binaries/data/mods/public/gui/replaymenu/replay_menu.js
274 ↗(On Diff #2996)

According to Stan it works on windows without the escapeText:

E:\Users\dolci\Documents\My Games\0ad\replays\0.0.23\2017-08-08_0001

As discussed on irc with Vladislav, removing the escapeText from the "text" elements doesn't work. So the change is correct and complete.

Added @elexis 's suggestion to use arrows to select too.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1914/ for more details.

Added @elexis `s suggestion to add 2 functions for the switch.

Build was aborted

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)...............io.cpp(58): Function call failed: return value was -1 (Function failed (no details available))
Function call failed: return value was -1 (Function failed (no details available))
Location: io.cpp:58 (Issue)

Call stack:

(0xb8d70e) ./test() [0xb8d70e]
(0xb1a9c9) ./test() [0xb1a9c9]
(0xb1c8aa) ./test() [0xb1c8aa]
(0xb32b47) ./test() [0xb32b47]
(0xba95ae) ./test() [0xba95ae]
(0xbac3fc) ./test() [0xbac3fc]
(0xb380d8) ./test() [0xb380d8]
(0x825ea9) ./test() [0x825ea9]
(0x827ae4) ./test() [0x827ae4]
(0x9dce47) ./test() [0x9dce47]
(0x8c9519) ./test() [0x8c9519]
(0x8ca4eb) ./test() [0x8ca4eb]
(0xb55621) ./test() [0xb55621]
(0x8cbb53) ./test() [0x8cbb53]
(0x483cdf) ./test() [0x483cdf]
(0x449595) ./test() [0x449595]

errno = 0 (?)
OS error = ?


(C)ontinue, (S)uppress, (B)reak, Launch (D)ebugger, or (E)xit?
Sleeping until debugger attaches.
Please wait.
Debugger launch failed: No such file or directory

Link to build: http://jw:8080/job/phabricator/1916/
See console output for more information: http://jw:8080/job/phabricator/1916/console

  • Ok for me to use both Immutable and Readonly phrases in the patch.
  • argument instead of copy&paste:
		// Since the GUI framework doesn't handle to set settings
		//  in Unicode (CStrW), we'll simply retrieve the actual
		//  pointer and edit that.
		CStrW* pCaption = (CStrW*)m_Settings["caption"].m_pSetting;
  • const int szChar -> SDL_KeyCode
  • Ok to not return IN_HANDLED in the two new functions, because that value should be returned in any case (because even if we press unrecognized characters, other GUI objects shouldn't be influenced).
  • SDLK_DOWN and SDLK_UP are almost completely copies, but I don't care to unify.
  • Not relevant code-wise, but seems a tiny bit more readable with scopes in all cases.
source/gui/CInput.cpp
221 ↗(On Diff #3317)

Don't really need the \t comment, nor the autocomplete one, nor the "let JS figure it out comment"

223 ↗(On Diff #3317)

Since you included some whitespace fixes, then we can fix the different comment formats here as well.

243 ↗(On Diff #3317)

wrong indentation

513 ↗(On Diff #3317)

same amount as tabs as the if ( has, then 4 spaces

552 ↗(On Diff #3317)

Re `/* END: Message History Lookup */`, there had been history lookup at some time in this file but it was deleted. So the comment is no good and should be deleted.

567 ↗(On Diff #3317)

Preferable to not hardcode a list here but let the if-chain handle that.

265 ↗(On Diff #3314)

Wat? (rP1904)

source/gui/CInput.h
1 ↗(On Diff #3317)

bump

elexis accepted this revision.Aug 30 2017, 1:51 AM

Thanks for the patch!

This revision is now accepted and ready to land.Aug 30 2017, 1:51 AM
ffffffff added a subscriber: ffffffff.EditedSep 19 2017, 11:41 PM

if u now get text color tags to cinput we could have select/copyable chat text window in lobby

if u now get text color tags to cinput we could have select/copyable chat text window in lobby

There is some source/gui/ function to remove GUI tags - or one to get the non-formatted text as far as I know.
However I wonder if copy&pasting lobby or ingame chat will be desirable feature.