Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Display Spectators in Scoreboard #466

Merged
merged 5 commits into from
Dec 3, 2016
Merged

Display Spectators in Scoreboard #466

merged 5 commits into from
Dec 3, 2016

Conversation

VelocityRa
Copy link
Collaborator

@VelocityRa VelocityRa commented Nov 30, 2016 •
edited
Loading

For discussion see the relevant Issue: #412.

Current look:

mschlumpp
mschlumpp reviewed Nov 30, 2016
View reviewed changes
Sources/Client/ScoreboardView.cpp
}
switch (numSpectators) {
case 0: return;
case 1: strcpy(buf, "Spectator:");
Copy link
Contributor

@mschlumpp mschlumpp Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use GetTextPlural to make it more localization friendly.
EDIT: Actually there is a Macro for this: _TrN(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I'll fix that, thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was about to say that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Spectator" doesn't belong in the localization lists though, it's not translated in any language.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet?

Copy link
Collaborator Author

@VelocityRa VelocityRa Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I misinterpreted the Macro's purpose, nvm.
I'll still have to hardcode "Spectator" though, not sure how to do other localization stuff.
Edit: Nvm, figured it out :P

Copy link
Collaborator

@feikname feikname Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VelocityRa
Could you please tell how you did it? I always wondered :)

My guess is update-pot.sh

EDIT: Okay, I understand now, create the string with _Tr or _TrN and xgettext will update the pot file when run as in update-pot.sh

@VelocityRa
Copy link
Collaborator Author

There we go fixed.
Having second thoughts about that colon now... It looks fine but it's not consistent with the the teams' texts.

@feikname
Copy link
Collaborator

I think it looks nice with the colon.

@feikname
Copy link
Collaborator

feikname commented Nov 30, 2016 •
edited
Loading

@VelocityRa I think there is something wrong with your implementation of plural forms, it always stays on the singular for translated texts for me.

EDIT: Remove the image, it was useless.

@VelocityRa
Copy link
Collaborator Author

VelocityRa commented Nov 30, 2016 •
edited
Loading

@feikname Have you specified the plural forms correctly? It should work.
The unlocalized strings should be "Spectator{1}" and "Spectators{1}", AFAIK.

@feikname
Copy link
Collaborator

feikname commented Nov 30, 2016 •
edited
Loading

I tried to simplify the line to strcpy(buf, _TrN("Client", "Spectator", "Spectators", numSpectators).c_str());, updated the .pot file and double-checked my translation and is still not working.

The .pot file was generated using update-pot.sh and the openspades.po was edited from Poedit, so I almost 100% sure it's no syntax error.

The most weird part is that if core_locale is empty, it works fine.

@VelocityRa
Copy link
Collaborator Author

VelocityRa commented Dec 1, 2016 •
edited
Loading

Based on other usages, I think I'm doing this correctly.
Gonna have to ask @yvt's help on this.

@feikname
Copy link
Collaborator

feikname commented Dec 1, 2016

I will double check myself too. I am almost 100% sure you did the right implementation, but I can't get it to work. Perhaps it's some dark auto-casting happening with NumSpectators

@VelocityRa
Copy link
Collaborator Author

Test it with 1 and 2 instead of numSpectators. Does it work correctly then?

@feikname
Copy link
Collaborator

feikname commented Dec 1, 2016 •
edited
Loading

I can only try a few hours from now, I will test as soon as I arrive back home. It will probably be in 3 hours.

@feikname
Copy link
Collaborator

feikname commented Dec 1, 2016 •
edited
Loading

@VelocityRa status update:

Ok, I tested and uploaded my changes to my fork of your fork.

I made sure to double-check the translations and also named them them as Singular{1} and Plural{1}, so I am almost sure there's no way I wronged them. In case you wanna see for yourself, this is the commit where I created them: https://github.com/feikname/openspades/commit/ff81607c3d7f66ff13b566b4f3b1fc171fbcd655

The .pot was generated from update-pot.sh (xgettext) and the pt_br/openspades.po was edited from Poedit, so I am exluding any syntax errors too. update-pot.sh was run from Bash on Windows (lxss).

I also used "2" as numSpectators, so it's not a issue with numSpectators value.

When my core_locale is set to "pt_br", the string translates as Singular:

However, with core_locale set to empty (nothing), it works correctly 😒

This is very weird, maybe a new issue about this should be opened? If you can confirm on your system too, I'd be very grateful.

I am now sure this is a bug within the localization system.

  • Sources/core/string.h
  • Sources/core/string.cpp

@feikname feikname added this to the 0.1.0 milestone Dec 1, 2016
feikname
feikname requested changes Dec 1, 2016
View reviewed changes
Copy link
Collaborator

@feikname feikname left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The translation problem is internal, your implementation is correct :)

The issue was reported in #469.

I requested changes just in case you forget to change spectator team ID to 255 before pushing.

@feikname feikname removed this from the 0.1.0 milestone Dec 1, 2016
@VelocityRa
Copy link
Collaborator Author

This is very weird, maybe a new issue about this should be opened? If you can confirm on your system too, I'd be very grateful.

Yeah it's present on my system too.

yvt and others added 2 commits December 2, 2016 20:48
@yvt @VelocityRa
Fix segfault caused by access to an uninitialized std::string
dd75492 
Segfault seems to occur only with the latest version of GCC or glibc.
Hopefully fixes the second issue reported in yvt#434.
@yvt @VelocityRa
Fix plural form selection code (yvt#470)
65202bb 
Fixes yvt#469.
@VelocityRa
Copy link
Collaborator Author

VelocityRa commented Dec 2, 2016 •
edited
Loading

@yvt Rebased your commits to test the locale thing and it works now! With pt_br locale and feikname's .po additions it displays "Plural:" with multiple spectators as expected.
Should I merge this now?

@yvt yvt merged commit 3c426b4 into yvt:master Dec 3, 2016
@feikname feikname mentioned this pull request Dec 3, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mschlumpp mschlumpp mschlumpp left review comments

@feikname feikname feikname approved these changes

Assignees
No one assigned
Labels
PR: improvement PR: new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VelocityRa @feikname @mschlumpp @yvt