Page MenuHomeWildfire Games

Add a `GetTextSize()` function to the GUI.
ClosedPublic

Authored by s0600204 on Aug 28 2017, 10:34 PM.

Details

Summary

Somewhat crude (and a bit lengthy, perhaps) function that returns the size of the text inside a given object (if said object has text).

There does already exist a GetTextWidth function (source/scripting/ScriptFunctions.cpp:L943). This uses FontMetrics. However, FontMetrics does not take into account text wrapping, line breaks, new lines, or font changes midway through the text it's fed. (Hence why this does not use a similar approach or, indeed, is in the same file.)

I don't know how useful it is to return left, right, top, or bottom values, but they're included for consistency with the returned parameters of the getComputedSize function.

As I've said before, I'm something of a newbie when it comes to c++, so if I've done something weird, inefficient, or just plain wrong, tell me gently 😁

Depends on D836

Written for eventual use with D297, but not (currently) required by it.

Test Plan
  1. Check code sanity.
  2. Compile.
  3. Find a text object in game and confirm it works.
    • (Entering Engine.GetGUIObjectByName("pgToolTip").getTextSize() onto the in-game command line, whilst hovering the cursor over the various options of the Main Menu works quite well.)
  4. Make sure it doesn't complain when used with any other gui elements:
    • Textboxes, [Textual] Inputs, and Buttons should all return valid and correct values.
    • Radiobuttons and Checkboxes should return a valid value, albeit one that has both width and height at 0. (Because although these gui element types possess caption attributes, they're not used...)
    • All other gui elements should return undefined.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
cpp_textsize_part2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6115
Build 10181: Vulcan BuildJenkins
Build 10180: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/448/ for more details.

vladislavbelov requested changes to this revision.Aug 29 2017, 2:17 PM
vladislavbelov added a subscriber: vladislavbelov.
vladislavbelov added inline comments.
source/gui/CGUI.h
72

We have to avoid these hacks. I think it's better to add a method which returns const map<...>&. Since this hack used for the IGUIScrollBarOwner too.

source/gui/scripting/JSInterface_IGUIObject.cpp
745

The friend is only for this line.

Also why you don't check it != map.end()? Currently the code has UB.

This revision now requires changes to proceed.Aug 29 2017, 2:17 PM
vladislavbelov added a comment.EditedAug 29 2017, 2:18 PM

Bump years. Also there is an apply conflict for me.

The rest looks good.

s0600204 updated this revision to Diff 3378.Aug 30 2017, 12:33 AM
s0600204 marked 2 inline comments as done.
s0600204 edited the summary of this revision. (Show Details)

Move style fetching code to a dedicated method, as suggested. This means that CGUI no longer considers getTextSize a friend. 😒

On reflection, I decided not to return left, right, top, or bottom values as they are not particularly useful, nor are they updated when the text is scrolled up & down.

Also, as CText (the only text-owner with scrollbars) reduces the width of the available space for text if there's a scrollbar enabled, regardless of whether or not the scrollbar is actually needed, we now do the same for consistency.

vladislavbelov requested changes to this revision.Aug 30 2017, 12:48 AM
vladislavbelov added inline comments.
source/gui/CGUI.cpp
482

nullptr.

source/gui/scripting/JSInterface_IGUIObject.cpp
738

The issue still presents: GetScrollBarStyle could return nullptr, then something bad may happen. Because you get a property of the nullptr object.

This revision now requires changes to proceed.Aug 30 2017, 12:48 AM

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://jenkins-master:8080/job/phabricator/1932/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/450/ for more details.

s0600204 updated this revision to Diff 3379.Aug 30 2017, 6:18 AM

Deal with the possibility of a scrollbar style not existing.

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://jenkins-master:8080/job/phabricator/1933/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/451/ for more details.

s0600204 updated this revision to Diff 5376.Jan 19 2018, 2:48 AM
s0600204 marked 2 inline comments as done.

Update in response to Vladislav's line-comments. Also, a rebase to keep stuff current.

s0600204 updated this revision to Diff 6198.Mar 17 2018, 11:08 PM

Use GetTextSize() in Template Viewer

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

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

s0600204 updated this revision to Diff 6602.May 21 2018, 4:32 AM

Rebase, and remove a couple of now unnecessary comments.

Also, this is the solution to something reported on the Forums (https://wildfiregames.com/forum/index.php?/topic/24362-overlapping-text/; second screenshot), so making sure this is ready for review.

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

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

wraitii added a subscriber: wraitii.

Code looks good now, I just have a bunch of remarks. I'll try and test this soon enough and actually accept it.

binaries/data/mods/public/gui/reference/viewer/viewer.js
114–115

The "8" now looks particularly conspicuous. Warrants at least a comment on why it's there.

source/gui/CGUI.h
28

Why is this suddenly necessary?

source/gui/IGUIScrollBarOwner.cpp
1

2018 now

source/gui/scripting/JSInterface_IGUIObject.cpp
85

FYI, I'm somewhat certain this will be useless in SM45.

695

This looks like it could be const-ed.

728

I'm assuming this is why you can't. Maybe it'd be worth consting those and making the cached variable mutable (for another patch)

s0600204 updated this revision to Diff 6608.May 21 2018, 10:21 PM
s0600204 marked 3 inline comments as done.

Bump years, remove unneeded include, and add comment

source/gui/CGUI.h
28

It was added so we could add a friend to the CGUI class in an earlier version of this revision. This friend was subsequently removed, and this wasn't removed at the same time.

source/gui/scripting/JSInterface_IGUIObject.cpp
85

In what way?

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

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

wraitii accepted this revision.Jan 5 2019, 10:26 AM

Code looks good and does what it says on the tin. Will let up for a few days as I kinda dislike committing code without an intended use, particularly GUI codes I'd like to know if this will be used sometimes soon.

In D844#69173, @wraitii wrote:

Code looks good and does what it says on the tin. Will let up for a few days as I kinda dislike committing code without an intended use, particularly GUI codes I'd like to know if this will be used sometimes soon.

Ahmm, it is used in viewer.js. Also if you look at the Stack of this revision you'd see that D1461 depends on this patch.
But I guess the author should commit the patch and not the reviewer. ;)
(Except the author of the patch is not available)

Well that's kinda what I needed to know. Indeed this should get committed by whoever is available ASAP then. Will do later unless someone else's done it first.

elexis added a subscriber: elexis.Jan 5 2019, 2:42 PM

Indeed this should get committed by whoever is available ASAP then. Will do later unless someone else's done it first.

s0600204 isn't retired.

source/gui/scripting/JSInterface_IGUIObject.cpp
700

https://bugzilla.mozilla.org/show_bug.cgi?id=1255800 So it's not yet deprecated and not part of the SM patches. (Got a patch somewhere, it's a 2-3 line replacement to nuke the macro.)

725

Was wondering if it wouldn't be cleaner to call into a function of the specific GUIObject type class that returns the CGUIString caption or getTextSize. But since it doesn't actually hardcode objecttypes here and since there are only 2 or 3 string types, seems okay.
But in general there might be new GUIObject types added that want to return something other than a property of the caption when getTextSize is called. What about dropdowns for instance?
(It sounds like we might want to introduce one JSClass for every GUIObject class that may inherit some general functions)

763

The copy could be avoided and the function length reduced by extending GUIScriptConversions.cpp for CSize. (Dunno if one can introduce only the ToJSVal without introducing FromJSVal for one type)

789

Why are width and height passed here?
They should be the same as right-left and bottom-top, or the different semantics should be reflected by the code by using a different object / reference, no?

s0600204 isn't retired.

Ah, right, mistakenly assumed given the date of the last revision and didn't check.

s0600204 updated this revision to Diff 7304.Jan 7 2019, 10:24 PM
s0600204 marked an inline comment as done.

Rebase, bump years, and remove couple of lines not directly applicable to current revision.

The changes between this and the previous revision-state are minor so, if no-one disagrees, I'll commit in a couple of days.

source/gui/scripting/JSInterface_IGUIObject.cpp
725

There's a part of me that wonders if its not possible to use m_GeneratedTexts of IGUITextOwner (which is a parent-class to all gui objects that display text except CInput) somehow... but working out how to do that seems beyond me.

Perhaps a method of IGUITextOwner could iterate through the generated texts of the object in question, add the text sizes together, then return the result. But that's either not possible, or I'm just not sufficiently proficient in c++ and/or spidermonkey peculiarities to work out how to get the IGUITextOwner of a selected object. (Most likely describing it incorrectly, too.)

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

Link to build: https://jenkins.wildfiregames.com/job/differential/941/

elexis added inline comments.Jan 8 2019, 12:28 AM
source/gui/scripting/JSInterface_IGUIObject.cpp
700

JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
JS::RootedObject thisObj(cx, &args.thisv().toObject());

s0600204 updated this revision to Diff 7310.Jan 8 2019, 3:13 AM

Nuke the macro (courtesy of elexis).

Vulcan added a comment.Jan 8 2019, 3:20 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/946/

This revision was not accepted when it landed; it landed in state Needs Review.Mar 18 2019, 11:15 PM
This revision was automatically updated to reflect the committed changes.
s0600204 marked 2 inline comments as done.
elexis added inline comments.Jul 23 2019, 4:54 AM
source/gui/scripting/JSInterface_IGUIObject.cpp
700

I suppose I took that from JSI_IGUIObject::construct

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::CallArgs

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_InitClass

Unlike other JSNatives, this function must not call JS_THIS or JS_THIS_OBJECT. Instead it must create a new object and return it.

The rest / macro uses the CallReceiver (CallArgs inherits CallReceiver). (So it doesn't seem wrong, just inconsistent.)

elexis added inline comments.Tue, Jul 30, 5:38 PM
ps/trunk/source/gui/scripting/JSInterface_IGUIObject.cpp
690 β†—(On Diff #7592)

See previous remarks that this function should not be located in this file if it only works for a limited set of GUI types #5442.

The first thought was to put it into each of the GUI types that use it, or into a helper function.

But if one investigates the code a bit more it becomes evident that the code is the same as the one in TextOwner, and the GUI types that support this aciton logically are the same ones as the TextOwner users.

So if the code is moved to the correct location (TextOwner::ComputeTextSize and JSI_TextOwner::getTextSize), then it also becomes apparent that the code in this function is duplicating what the TextOwner users do. The redundant code is also doing redundant work, because the GenerateText call results are recomputed each call, not stored, whereas the TextOwner already has a cache.

711 β†—(On Diff #7592)

also: CTooltip,...

715 β†—(On Diff #7592)

I suppose CInput support is only here because it has the caption property and the case has to be caught to not error for the intended use cases.

721 β†—(On Diff #7592)

(CProgressBar has caption float)

742 β†—(On Diff #7592)

all of the above is a astraight copy of CText code with modifications unnecessary if it is located in TextOwner

749 β†—(On Diff #7592)

ToJSVal specialization

751 β†—(On Diff #7592)

This is never thrown, copypasta