Page MenuHomeWildfire Games

Dynamically sizes the dataCounter overlay
ClosedPublic

Authored by ramtzok1 on Jan 25 2019, 6:05 PM.

Details

Summary

This is a simple fix is for #5385.

Test Plan

Switch to the Bulgarian language and confirm the fps counter is fully shown

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6912
Build 11326: arc lint + arc unit

Event Timeline

ramtzok1 created this revision.Jan 25 2019, 6:05 PM
Owners added a subscriber: Restricted Owners Package.Jan 25 2019, 6:05 PM
ramtzok1 edited the summary of this revision. (Show Details)Jan 25 2019, 6:23 PM
elexis added a subscriber: elexis.Jan 25 2019, 6:49 PM

This is a code problem, not a string problem. It can happen with other languages too. I think the same problem exists with some playernames in the lag-warnings area too.
The *.po files are downloaded from transifex, so they would be edited there ifanything. But I would recommend to not change it that way.

lyv added a subscriber: lyv.Jan 25 2019, 7:14 PM

Removing one arbitrary number with another arbitrary number can fix this.

Index: binaries/data/mods/public/gui/common/functions_global_object.js
===================================================================
--- binaries/data/mods/public/gui/common/functions_global_object.js	(revision 21929)
+++ binaries/data/mods/public/gui/common/functions_global_object.js	(working copy)
@@ -17,9 +17,9 @@
 	dataCounter.caption = counters.join("\n") + "\n";
 	dataCounter.hidden = !counters.length;
 	dataCounter.size = sprintf("%(left)s %(top)s %(right)s %(bottom)s", {
-		"left": "100%%-100",
+		"left": "100%-110",
 		"top": "40",
-		"right": "100%%-5",
+		"right": "100%-5",
 		"bottom": 40 + 14 * counters.length
 	});
 }

However, this would also break given a lengthy enough string. The correct solution would be to adapt the sizes based on the width of the longest string just as how it's done for the network warning thingy. Engine.GetTextWidth(font, string) IIRC.

In D1764#71050, @elexis wrote:

This is a code problem, not a string problem. It can happen with other languages too. I think the same problem exists with some playernames in the lag-warnings area too.
The *.po files are downloaded from transifex, so they would be edited there ifanything. But I would recommend to not change it that way.

Do you have any reference to the playernames problem so I can try to understand the linking between them?

lyv added a comment.Jan 25 2019, 7:22 PM

I think the same problem exists with some playernames in the lag-warnings area too.

It won't exist there as the width check is done in that specific case.

In D1764#71050, @elexis wrote:

This is a code problem, not a string problem. It can happen with other languages too. I think the same problem exists with some playernames in the lag-warnings area too.
The *.po files are downloaded from transifex, so they would be edited there ifanything. But I would recommend to not change it that way.

I found the script "functions_global_object.js" interesting.
The function updateCounters updates the overlay, I think we need to make an if-statement ask if the text overflows the overlay width then subtract X to "left" field but I can't find a way to do so.
For example, in Bulgarian if I subtract to the "left" field 5, it fixes the problem.

dataCounter.size = sprintf("%(left)s %(top)s %(right)s %(bottom)s", {
		"left": "100%%-105", // Was 100
		"top": "40",
		"right": "100%%-5",
		"bottom": 40 + 14 * counters.length
	});

Maybe try rather adapts the code with the fonction enunciate by smiley.

So the updateCounters function that smiley mentioned uses a fixed width, but the width should change dynamically. I thought this was already the case. Also as he correctly pointed out, Engine.GetTextWidth can be used for that, like getNetworkWarnings() in binaries/data/mods/public/gui/common/network.js does. If you want you can try to download the code and try to implement that (JavaScript changes only, no need to compile). BuildInstructions.

ramtzok1 updated this revision to Diff 7402.Jan 27 2019, 1:33 PM

How about this fix? I think it works perfectly with any string.

Owners added a subscriber: Restricted Owners Package.Jan 27 2019, 1:33 PM
lyv added a comment.Jan 27 2019, 1:50 PM

Add yourself to gui/credits/text/programming.json (path might be wrong).
And also remove the unnecessary three files from the diff. Only needs the changed JS file.

binaries/data/mods/public/gui/common/functions_global_object.js
18

The width of the longest counter. The second one might be longer.
longestString = counters.reduce((a, b) => a.length > b.length ? a : b)

Also, that negative sign seems useless.

lyv added inline comments.Jan 27 2019, 1:51 PM
binaries/data/mods/public/gui/common/functions_global_object.js
18

Also, are those %% typos?. I could not find any evidence to suggest otherwise. (Not that I looked).

lyv retitled this revision from Fixes FPS overlay bug in the Bulgarian language to Dynamically sizes the dataCounter overlay.Jan 27 2019, 2:03 PM
lyv edited the summary of this revision. (Show Details)
lyv edited the test plan for this revision. (Show Details)
elexis added inline comments.Jan 27 2019, 3:56 PM
binaries/data/mods/public/gui/common/functions_global_object.js
18

a.length > b.length ? a : b

number of characters != text width.
1.2 looks like a cheat (hiding a bug?) Maybe it just needs to be one more pixel wider than the text width to not trigger a linewrap, or what is the issue?

%%

(I was wondering too and not looking either. I only know them from a translated sprintf string where %% encoded %, but not from a sprintf argument.)

lyv added inline comments.Jan 27 2019, 4:20 PM
binaries/data/mods/public/gui/common/functions_global_object.js
18

Replace with GetTextWidth thingy then.

With very wide strings, that 1.2 would cause issues. I assume its a buffer area, So just add a constant number of pixels.

ramtzok1 marked an inline comment as done.Jan 28 2019, 11:02 AM
In D1764#71134, @smiley wrote:

Add yourself to gui/credits/text/programming.json (path might be wrong).
And also remove the unnecessary three files from the diff. Only needs the changed JS file.

Will do, I'm pretty new to that kind of source control, sorry for my nooby understanding.

ramtzok1 updated this revision to Diff 7413.Jan 28 2019, 11:51 AM
ramtzok1 marked an inline comment as done.

Getting the width of the longest text and then adding a constant to make the text fit to the overlay

ramtzok1 added a comment.EditedJan 28 2019, 12:16 PM
In D1764#71134, @smiley wrote:

Add yourself to gui/credits/text/programming.json (path might be wrong).
And also remove the unnecessary three files from the diff. Only needs the changed JS file.

Sorry for the dumb question but how do I remove these files? I don't understand how to remove these 3 files.

binaries/data/mods/public/gui/common/functions_global_object.js
18

I made that it gets the width of the longest string and then adding a constant number which is 15 (For some reason it works no matter how long the string is, it fits perfectly with the overlay).

(I guess *.exe and *.dll files changed because you compiled. Notice that you don't need to do this as long as you only change .xml / .js)

binaries/data/mods/public/gui/common/functions_global_object.js
18

Perhaps rename a, b to max and current, so that the reader doesn't have to infer.

I think it should be possible to only store the maxWidth instead of maxText, removing two GetTextWidth calls?

reduce consumes two arguments, might be nicer to not rely on the default value (which is the first element of the array) and pass that explicitly (0 in case of computing the width instead of the text).

(I guess that function should also account for different fonts in theory (that would be my mistake when introducing the function). I recall s0600204 has worked on a patch to get the text size of mixed contents: D844. Perhaps the GetTextWidth function can be superseded then.)

Otherwise patch loooks good, thanks!

lyv added inline comments.Feb 1 2019, 6:46 AM
binaries/data/mods/public/gui/common/functions_global_object.js
15

These lines were intentional for making it easier on the eye.

binaries/data/mods/public/gui/credits/texts/programming.json
252

Alphabetical order

ramtzok1 updated this revision to Diff 7474.Feb 8 2019, 9:02 PM
ramtzok1 marked an inline comment as done.

Spaced lines and fixed my name in the programmers credits file.

Itms added a reviewer: Restricted Owners Package.Mar 10 2019, 7:31 PM
Stan added a subscriber: Stan.Apr 19 2019, 6:07 PM

Binary files should be removed from the diff :)

bb added a subscriber: bb.May 31 2019, 1:46 AM
bb added inline comments.
binaries/data/mods/public/gui/common/functions_global_object.js
20–23

Sounds exactly like a use case of getTextSize indeed, see D844

bb accepted this revision.Sep 28 2020, 5:49 PM

Needs rebase, but why not just do that and commit it.

This revision is now accepted and ready to land.Sep 28 2020, 5:49 PM
This revision was automatically updated to reflect the committed changes.