Page MenuHomeWildfire Games

Stop new-line character making line longer than it should be.
ClosedPublic

Authored by s0600204 on Feb 13 2018, 8:17 PM.

Details

Summary

When a line of text has a new-line character (\n), the line seems to be measured to be wider than it should be.

This was noted back when work was being done on the summary screen to add the statistics of building capturing (#3216). There, it was noted that having the following, centred in a text object:

a / b\nc / d

was resulting in:

a / b
 c / d

(The workaround at the time was to add a new-line character to the second line so that both line have the same initial width/length.)

The reason appears to be as follows:

  • \n is an unprintable character.
  • Thus, it does not have a glyph in any of the font files.
  • Thus, when it comes to measure that character, the measuring code (source/graphics/Font.cpp CFont::GetCharacterWidth()) returns the width of the "Missing Glyph" symbol.

For the purposes of demonstration, changes to the summary screen have been included to show how the second lines no longer need a new-line character tacked on the end. Whether or not these changes get committed with the c++ changes (should this revision be accepted) is up to the reviewer(s). (Further work to permit the postfix attribute to be optional is a possibility for another revision.)

Depends on D1298

Test Plan
  • Apply the patch, but don't (re)compile.
  • Start 0ad, and go to the summary screen (start a new sp game, then leave it so as to get a screen with blank stats)
  • Notice that without the \ns postfixed to the end of the second lines of text, the rows on the "Buildings" and "Units" tabs are misaligned.
  • Quit, and recompile
  • Start 0ad, and return to the summary screen
  • Note how the rows are now aligned

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

s0600204 created this revision.Feb 13 2018, 8:17 PM
Vulcan added a subscriber: Vulcan.Feb 13 2018, 8:21 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 503| 503| 	Engine.GetGUIObjectByName("summaryText").caption =
| 504| 504| 		g_GameData.gui.isInGame ?
| 505| 505| 			translate("Current Scores") :
| 506|    |-		g_GameData.gui.isReplay ?
|    | 506|+			g_GameData.gui.isReplay ?
| 507| 507| 			translate("Scores at the end of the game.") :
| 508| 508| 		g_GameData.gui.disconnected ?
| 509| 509| 			translate("You have been disconnected.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 504| 504| 		g_GameData.gui.isInGame ?
| 505| 505| 			translate("Current Scores") :
| 506| 506| 		g_GameData.gui.isReplay ?
| 507|    |-			translate("Scores at the end of the game.") :
|    | 507|+				translate("Scores at the end of the game.") :
| 508| 508| 		g_GameData.gui.disconnected ?
| 509| 509| 			translate("You have been disconnected.") :
| 510| 510| 		!assignedState ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 505| 505| 			translate("Current Scores") :
| 506| 506| 		g_GameData.gui.isReplay ?
| 507| 507| 			translate("Scores at the end of the game.") :
| 508|    |-		g_GameData.gui.disconnected ?
|    | 508|+				g_GameData.gui.disconnected ?
| 509| 509| 			translate("You have been disconnected.") :
| 510| 510| 		!assignedState ?
| 511| 511| 			translate("You have left the game.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 506| 506| 		g_GameData.gui.isReplay ?
| 507| 507| 			translate("Scores at the end of the game.") :
| 508| 508| 		g_GameData.gui.disconnected ?
| 509|    |-			translate("You have been disconnected.") :
|    | 509|+					translate("You have been disconnected.") :
| 510| 510| 		!assignedState ?
| 511| 511| 			translate("You have left the game.") :
| 512| 512| 		assignedState.state == "won" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 507| 507| 			translate("Scores at the end of the game.") :
| 508| 508| 		g_GameData.gui.disconnected ?
| 509| 509| 			translate("You have been disconnected.") :
| 510|    |-		!assignedState ?
|    | 510|+					!assignedState ?
| 511| 511| 			translate("You have left the game.") :
| 512| 512| 		assignedState.state == "won" ?
| 513| 513| 			translate("You have won the battle!") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 508| 508| 		g_GameData.gui.disconnected ?
| 509| 509| 			translate("You have been disconnected.") :
| 510| 510| 		!assignedState ?
| 511|    |-			translate("You have left the game.") :
|    | 511|+						translate("You have left the game.") :
| 512| 512| 		assignedState.state == "won" ?
| 513| 513| 			translate("You have won the battle!") :
| 514| 514| 		assignedState.state == "defeated" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 509| 509| 			translate("You have been disconnected.") :
| 510| 510| 		!assignedState ?
| 511| 511| 			translate("You have left the game.") :
| 512|    |-		assignedState.state == "won" ?
|    | 512|+						assignedState.state == "won" ?
| 513| 513| 			translate("You have won the battle!") :
| 514| 514| 		assignedState.state == "defeated" ?
| 515| 515| 			translate("You have been defeated…") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 510| 510| 		!assignedState ?
| 511| 511| 			translate("You have left the game.") :
| 512| 512| 		assignedState.state == "won" ?
| 513|    |-			translate("You have won the battle!") :
|    | 513|+							translate("You have won the battle!") :
| 514| 514| 		assignedState.state == "defeated" ?
| 515| 515| 			translate("You have been defeated…") :
| 516| 516| 			translate("You have abandoned the game.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 511| 511| 			translate("You have left the game.") :
| 512| 512| 		assignedState.state == "won" ?
| 513| 513| 			translate("You have won the battle!") :
| 514|    |-		assignedState.state == "defeated" ?
|    | 514|+							assignedState.state == "defeated" ?
| 515| 515| 			translate("You have been defeated…") :
| 516| 516| 			translate("You have abandoned the game.");
| 517| 517| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 512| 512| 		assignedState.state == "won" ?
| 513| 513| 			translate("You have won the battle!") :
| 514| 514| 		assignedState.state == "defeated" ?
| 515|    |-			translate("You have been defeated…") :
|    | 515|+								translate("You have been defeated…") :
| 516| 516| 			translate("You have abandoned the game.");
| 517| 517| 
| 518| 518| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 513| 513| 			translate("You have won the battle!") :
| 514| 514| 		assignedState.state == "defeated" ?
| 515| 515| 			translate("You have been defeated…") :
| 516|    |-			translate("You have abandoned the game.");
|    | 516|+								translate("You have abandoned the game.");
| 517| 517| 
| 518| 518| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
| 519| 519| 		translate("Game time elapsed: %(time)s"), {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 518| 518| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
| 519| 519| 		translate("Game time elapsed: %(time)s"), {
| 520| 520| 			"time": timeToString(g_GameData.sim.timeElapsed)
| 521|    |-	});
|    | 521|+		});
| 522| 522| 
| 523| 523| 	let mapType = g_Settings.MapTypes.find(type => type.Name == g_GameData.sim.mapSettings.mapType);
| 524| 524| 	let mapSize = g_Settings.MapSizes.find(size => size.Tiles == g_GameData.sim.mapSettings.Size || 0);

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

elexis added a subscriber: elexis.Feb 13 2018, 8:51 PM
elexis added inline comments.
binaries/data/mods/public/gui/summary/summary.js
76 ↗(On Diff #5781)

those 2 above not?

(Also " / " seems like a strange postfix)

source/gui/CGUI.cpp
696 ↗(On Diff #5781)

Do we have the same problem with other non-glyph characters?

s0600204 added inline comments.Feb 16 2018, 12:48 PM
binaries/data/mods/public/gui/summary/summary.js
76 ↗(On Diff #5781)

The two above are used at the end of the first line (the \n following the b in the example given in the revision description).

source/gui/CGUI.cpp
696 ↗(On Diff #5781)

Simple and short answer: apparently not.

Longer answer:

Most valid (in the sense that they're part of utf8) non-printable characters should probably not be included in in-game texts at all (for simplicities sake, particularly if they're being handed to translators), so deliberately not catching them (causing the text to not look right) would help identify problems.

Actually testing to see what happens if one tries to use other non-printable characters (using https://en.wikipedia.org/wiki/C0_and_C1_control_codes as a guide to possible \{*} indicators):

  • \0, \b, \t, \v, \f, \r --> replaced with the "missing glyph" symbol
  • \a --> a
  • \e--> e

Whilst not exactly exhaustive, it appears that attempting to use other control-characters cause something to be rendered, and as that something has a width, not including that in the total width of the line would be incorrect.

As to printable characters that we don't have glyphs for in the default fonts (ie. if the East Asian locales mod is not installed and enabled) the "missing glyph" symbol will be rendered, and that definitely has width, so returning 0 width in CFont if a glyph is missing would be incorrect behaviour.

Imarok accepted this revision.Mar 3 2018, 10:54 AM
Imarok added a subscriber: Imarok.

Tested it. Works. Code looks good.
The bug came from rP13051 adding the line line_width += Feedback2.m_Size.cx; at the wrong place.

Thanks for the patch. It always annoyed me that the summary entries are not centered.

This revision is now accepted and ready to land.Mar 3 2018, 10:54 AM
elexis added inline comments.Mar 3 2018, 1:18 PM
source/gui/CGUI.cpp
696 ↗(On Diff #5781)

Thanks for the well researched answer :-)

non-printable characters should probably not be included in in-game texts at all

Agree with the 'should' policy

replaced with the "missing glyph" symbol

Probably shouldn't relay on that in an ideal world

Wondering why the newline character has width at all - If it didn't we wouldn't have to change the code.
A "printable" check would come in handy if we can't rely on the width being reasonable.
On the other hand meh.

This revision was automatically updated to reflect the committed changes.