Page MenuHomeWildfire Games

Implement placeholder text for input fields and get rid of hack with mod filter
Needs ReviewPublic

Authored by Angen on Dec 9 2019, 10:47 PM.

Details

Reviewers
vladislavbelov
Summary
  • different font colour
  • no need to delete the text, when one wants to write there something
  • text displayed is not a value so field is technically empty
  • will disappear when user writes some character
  • will appear when user deletes all characters
  • we can get rid of text value "Filter" in mod selection screen and ugly hack around it
Test Plan

check functionality
check that anything what can affect generated placeholder will invalidate it on change

Event Timeline

Angen created this revision.Dec 9 2019, 10:47 PM
Angen added a reviewer: Restricted Owners Package.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/728/display/redirect

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  34| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
| 349| 349| 
| 350| 350| 	g_ModsEnabled.sort((folder1, folder2) =>
| 351| 351| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 352|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 352|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 353| 353| 
| 354| 354| 	displayModList("modsEnabledList", g_ModsEnabled);
| 355| 355| }
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1244/display/redirect

Angen updated this revision to Diff 10584.Dec 14 2019, 2:59 PM

make translatable attribute work

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/755/display/redirect

vladislavbelov added inline comments.
source/gui/ObjectTypes/CInput.cpp
1506

I prefer if (m_Caption.empty() && !m_PlaceholderText.empty()).

vladislavbelov added inline comments.Dec 14 2019, 3:13 PM
source/gui/ObjectTypes/CInput.cpp
1509

That's not a correct usage, usually we pass m_PlaceholderText.c_str(), but after C++11 we might also pass m_PlaceholderText.data().

vladislavbelov added inline comments.Dec 14 2019, 3:16 PM
source/gui/ObjectTypes/CInput.cpp
1509

Also you it on each frame, which means that the textRenderer recalculate glyphs/sizes for each frame. Which means a not needed work that costs time.

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  34| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
| 348| 348| 
| 349| 349| 	g_ModsEnabled.sort((folder1, folder2) =>
| 350| 350| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 351|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 351|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 352| 352| 
| 353| 353| 	displayModList("modsEnabledList", g_ModsEnabled);
| 354| 354| }
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1271/display/redirect

Angen updated this revision to Diff 10589.Dec 14 2019, 3:20 PM

user empty() and c_str()

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/758/display/redirect

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  34| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
| 348| 348| 
| 349| 349| 	g_ModsEnabled.sort((folder1, folder2) =>
| 350| 350| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 351|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 351|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 352| 352| 
| 353| 353| 	displayModList("modsEnabledList", g_ModsEnabled);
| 354| 354| }
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1274/display/redirect

Angen planned changes to this revision.Dec 20 2019, 10:34 PM

needs rebase anyway

Vulcan added a comment.Feb 8 2020, 5:38 PM

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  36| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
| 378| 378| 
| 379| 379| 	g_ModsEnabled.sort((folder1, folder2) =>
| 380| 380| 		dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 :
| 381|    |-		dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
|    | 381|+			dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0);
| 382| 382| 
| 383| 383| 	g_ModsEnabledFiltered = displayModList("modsEnabledList", g_ModsEnabled);
| 384| 384| }
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1729/display/redirect

Angen planned changes to this revision.Mar 16 2020, 7:06 PM

Needs rework,
caption change event is triggering rebuild, so solution would be possibly override.

Angen removed a reviewer: Restricted Owners Package.Mar 16 2020, 7:06 PM
Angen updated this revision to Diff 11624.Apr 3 2020, 8:46 PM
Angen edited the test plan for this revision. (Show Details)
Vulcan added a comment.Apr 3 2020, 8:56 PM

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  36| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner,·public·IGUITextOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1964/display/redirect

elexis added a subscriber: elexis.Apr 3 2020, 9:18 PM
elexis added inline comments.
source/gui/ObjectTypes/CInput.h
35

Before someone suggests removing the comment, one could also check whether it would be more logically consistent to not introduce the inheritance, or to inherit but use it for both text renderings, since there would be 3 lines using the new IGUITextOwner inheritance vs. 2000 lines managing and rendering text without IGUITextOwner.

The class should use the IGUITextOwner if CInput shall be able to print selectable colorized, perhaps link-clickable text, for example for ToS, chat, mapdescription, but would require rewriting the entire class (and D1781).

221

Perhaps CGUIText could be used to support tags (such as font) and avoid m_PlaceholderColor (or perhaps CGUIString is more consistent with COList columns or something, dunno, but there are those two possibilities. Actually COList seems to use CStrW, so there are three. CStrW would be consistent with m_Caption.)

Angen added inline comments.Apr 4 2020, 9:45 AM
source/gui/ObjectTypes/CInput.cpp
927

:(

source/gui/ObjectTypes/CInput.h
35

IGUITextOwner is used to prebuild string, but caption of input will change too frequently to benefit from that.
Another way I may copy paste required code and do not inherit but duplication.

221

CGUIString is required for CGUIText constructor so thats why not CStrW. Not sure if CGUIText can be created from js like CGUIString is but I will check.

Angen planned changes to this revision.Jun 14 2020, 9:23 AM

found issue, that it will split longer text to multiple lines based on spaces

source/gui/ObjectTypes/CInput.h
35

I will remove inheritance since I am overriding most of the functions anyway

221

For the record:
Engine.GetGUIObjectByName("modGenericFilter").placeholder_text = setStringTags("Filter", {"color": "255 251 131" });
works with CGUIText perfectly
Even tooltip is using CGUIText
I would prefer to leave placeholder_color to make easier to define color in xml

Angen updated this revision to Diff 12297.Jun 14 2020, 9:35 AM

rebase
remove inheritance

Angen planned changes to this revision.Jun 14 2020, 9:36 AM

@vladislavbelov would you have idea why it splits placeholder text to multiline by empty spaces ?

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  35| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
|    |++++| /zpool0/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
| 105| 105| {
| 106| 106| 	Engine.GetGUIObjectByName("negateFilter").checked = false;
| 107| 107| 
| 108|    |-	Engine.GetGUIObjectByName("modGenericFilter").placeholder_text = setStringTags("Write filter here", {"color": "255 251 131" });
|    | 108|+	Engine.GetGUIObjectByName("modGenericFilter").placeholder_text = setStringTags("Write filter here", { "color": "255 251 131" });
| 109| 109| 	displayModLists();
| 110| 110| }
| 111| 111| 
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2426/display/redirect

The wrapping behaviour is done in the text itself, so probably it's not seeing the size you'd expect.

I'm wondering if you couldn't achieve this by having a CText object operated upon by the Input, but maybe it's not really easier.

Angen updated this revision to Diff 12342.Jun 16 2020, 7:54 PM

working version
fixed wrapping and clipping

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

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CInput.h
|  35| class·CInput·:·public·IGUIObject,·public·IGUIScrollBarOwner
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCInput:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2450/display/redirect

vladislavbelov added inline comments.Sat, Sep 12, 2:05 PM
source/gui/ObjectTypes/CInput.cpp
114

That's the strange line, why this comment and strange cast?

source/gui/ObjectTypes/CInput.h
66

Why do you need these AddText methods? Especially in case there is the only one usage in the constructor.

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

builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1544/display/redirect