[PATCH] Don't say late observers are rejoining

[Imarok]

When a late observer joins It says: Player is starting to rejoin and Player has rejoined. excpected: s/rejoin/join

[elexis]

About the ticket: The verb rejoined was reported few times as it is factually wrong to say rejoined when a late observer that didn't join before joined (so indeed why not address the confusion).

About the patch: People on transifex will likely ask what's the difference between these strings. So adding a translation comment, stating that one string applies to players and the other only to observers (that might not have joined before) would prevent further confusion.

(Bug reported by Hannibal Barca I guess?)

[Imarok]

Replying to elexis:

About the patch: People on transifex will likely ask what's the difference between these strings. So adding a translation comment, stating that one string applies to players and the other only to observers (that might not have joined before) would prevent further confusion.

Isn't the difference between join and rejoin clear?

(Bug reported by Hannibal Barca I guess?)

Nope. That annoys me since a long time...

[Imarok]

Add translation comments to clarify the difference between join and rejoin

[elexis]

Actually I misread the patch. The if statement checks whether the client has been known at turn 0, right? So it would still be wrong for late observers who rejoined prior.

Would you agree to using a playerIndex != -1 check, so that all clients that are or have been players receive the "rejoining" string while usual observers receive the "rejoining" string?

Replying to Imarok:

Isn't the difference between join and rejoin clear?

The difference between the meaning of the words in the strings is clear, but it is not obvious when the strings are actually used. You might be right that it is an implementation decision which could change later and thus might be avoided, I will leave it up to you (you might want to check transifex every now and then anyway when adding strings).

(Bug reported by Hannibal Barca I guess?)

Nope. That annoys me since a long time...

So the use case is real.

((Also funny coincidence. He reported that string issue about 2 days ago, told him to write a ticket. He posted a diff distinguishing between those 2 strings. When asking who else in the lobby can be requested for proposals, I could only refer to you. ​100th monkey?))

[Imarok]

Replying to elexis:

Would you agree to using a playerIndex != -1 check, so that all clients that are or have been players receive the "rejoining" string while usual observers receive the "rejoining" string?

I think that's what I'm doing already: g_GameAttributes.settings.PlayerData[g_PlayerAssignments[msg.guid].player - 1] checks if the player is an observer.

[elexis]

Right (it were the player assignments in the NetServer that saved the observers that are known at turn 0, not this array). But g_PlayerAssignments[msg.guid].player != -1 would still be shorter.

[Imarok]

true

[Imarok]

shorter observer check

[elexis]

Thanks

[Imarok]

Replying to elexis:

Thanks

was that a review? ;)

[elexis]

I gave some comments on the patch (most importantly whether all requirements have been identified, whether the code design is going in the right direction and how the code should be shaped to stay maintainable), which for the most part implies doing a review.

At this point, one of the core developers should review the final patch on the following points: Code style (check if the patch follows all Coding Conventions) An in-game test (check if the patch implements the advertised changes) Unit tests (check if the required tests are implemented and the tests still work) Completeness (verifying that there are no files forgotten which require similar changes)

[mimo]

Isn't it too much requirements for a patch which only changes two sprintf, which is proposed by a team member (whom i'm confident have already checked it) and which was already commented by another team member?

[Imarok]

In 19082:

Don't say late observers are rejoining. fixes #4410

Reviewed by: elexis
Differential Revision: ​https://code.wildfiregames.com/D5

[elexis]

Thanks for the enhancement!