Page MenuHomeWildfire Games

More detailed what changed messages.
Needs RevisionPublic

Authored by gentz on Jan 5 2018, 6:00 AM.

Details

Summary

Makes the what changed messages more detailed.

Test Plan

Possibly find less wordy what changed sentences.
Fix weird bugs.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4330
Build 7593: Vulcan BuildJenkins
Build 7592: arc lint + arc unit

Event Timeline

gentz created this revision.Jan 5 2018, 6:00 AM
Owners added a subscriber: Restricted Owners Package.Jan 5 2018, 6:00 AM
gentz edited the summary of this revision. (Show Details)Jan 5 2018, 6:06 AM
Vulcan added a subscriber: Vulcan.Jan 5 2018, 7:07 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Angen added a subscriber: Angen.EditedJan 5 2018, 11:56 AM

I tried it as host and I got these messages on start hosting:
1 You have changed some settings
2 Angen has joined
3 Some player settings have been changed
4 You have changed som settings

If you meant these 1, 3, 4, so they are not random.
1 is related to resetReadyData("-host"); in updateGameAttributes()
3 is from resetReadyData("-settings-PlayerData"); in onClientJoin(newGUID, newAssignments)
4 I am not sure

3 you could eliminate if you can prevent to send it if you are the host
1 I think you could ignore first sending of this message because on host start it is changed as new settings are created

elexis added a subscriber: elexis.Jan 5 2018, 1:18 PM

What does it show if I change every gamesetting while nobody set ready? Only the last setting changed or every setting changed? If it's the first, it seems misleading, if it's the last it seems too spammy, no?

binaries/data/mods/public/gui/gamesetup/gamesetup.js
174

Not like that.
The gamesetup is supposed to be unaware of the existence of any seting besides in those two arrays g_Dropdowns, g_Checkboxes and the order.
Every hardcoding of gameseutp names elsewhere must be considered a bug.
So move those strings to a property of these objects.

Angen added a comment.EditedJan 5 2018, 2:18 PM

If you keep changing the same settings, there is no message.
Consider this:
first
you disable spies, they get message, but you enable them back and they get no message
second
you can play with more options and they will be spammed

  • maybe send the changes from more options only on OK pressed

In D1195#48465, @elexis wrote:

What does it show if I change every gamesetting while nobody set ready? Only the last setting changed or every setting changed? If it's the first, it seems misleading, if it's the last it seems too spammy, no?

Even when I was not ready I got every change message from more options.

Angen added a comment.Jan 5 2018, 2:25 PM
This comment was removed by Angen.
ffffffff added a subscriber: ffffffff.
ffffffff added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
174

Good strings! Just the code style needs change. : )

gentz updated this revision to Diff 5091.Jan 5 2018, 7:44 PM
gentz edited the summary of this revision. (Show Details)

Fixed these 3 bugs:

  1. 3 random messages the host gets when he starts hosting, how do I get rid of those?
  2. Scrolling too fast makes it cycle between the real message and another message
  3. The first proper message appears twice for some reason
elexis requested changes to this revision.Jan 5 2018, 7:52 PM
elexis added a subscriber: Imarok.
elexis added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
164

The wheather strings are half-sentences. Also it should be possible to clarify depending on the given value. For example "Wonder Victory timer something is now 20 minutes", or "Garrisoning heroes has been disabled".

174

All strings must be moved to g_Dropdowns and g_Checkboxes.

1405

I guess @Imarok would say that we want a gamesetup rewrite so that we can identify changed settings uniquely, not the resulting g_GameAttributes object.
This function is a terrible hack representative of the gamesetup. I'm afraid this should not be committed this way prior to refactoring the g_GameAttributes object.

This revision now requires changes to proceed.Jan 5 2018, 7:52 PM
Vulcan added a comment.Jan 5 2018, 8:47 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
gentz updated this revision to Diff 5092.Jan 5 2018, 9:09 PM
Vulcan added a comment.Jan 5 2018, 9:13 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
gentz added a comment.Jan 5 2018, 9:16 PM
In D1195#48465, @elexis wrote:

What does it show if I change every gamesetting while nobody set ready? Only the last setting changed or every setting changed? If it's the first, it seems misleading, if it's the last it seems too spammy, no?

Even when I was not ready I got every change message from more options.

It shows a message if someone is ready or if this change is different from the last. The idea was to stop the host from changing some random setting then secertly changing the setting they want, however if you think it's too spammy I can simply make it if someone is ready.

elexis added a comment.Jan 5 2018, 9:18 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
147–171

It's sufficient if we get a JS error message

177

The lines above shouldn't be needed, at most one entry

456
1455

All of this shouldn't be here either but in g_Dropdowns and g_Checkboxes

1455

All of this shouldn't be here either. It should be(come) possible to derive the changed setting directly. This is related to the rewrite by Imarok mentioned above. If someone wants to change a setting, only that one setting name should be sent by the server. Then we don't need to use black magic to find which setting changed. This will also allow other players to setup the game and giving privileges which settings they are allowed to change.

I would say we must find a way to get Imarok to finish the rewrite, perhaps we have to threaten him to help him ;-)

gentz marked 3 inline comments as done.EditedJan 5 2018, 9:21 PM

@elexis @Imarok What exactly should I change? Do you want me to make it only send the differences across the network?

Edit: didn't see the comment above, nvm

gentz added inline comments.Jan 5 2018, 9:24 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
456

They are marked with translate() in g_FormatChatMessage, do you want it moved?

elexis added inline comments.Jan 5 2018, 9:30 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
456

It must, since the python script extracting the strings from all kinds of textual files, doesnt infer any logic, but reads those lines one by one and forgets about the next line right after

gentz updated this revision to Diff 5093.Jan 5 2018, 9:38 PM

Made it throw an error instead.

gentz marked an inline comment as done.Jan 5 2018, 9:39 PM
gentz added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
456

I was under the assumption it was a function, will fix.

Vulcan added a comment.Jan 5 2018, 9:40 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
gentz updated this revision to Diff 5094.Jan 5 2018, 9:45 PM

Translation thingy

gentz updated this revision to Diff 5095.Jan 5 2018, 9:46 PM

Missed one

Vulcan added a comment.Jan 5 2018, 9:47 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
gentz marked 4 inline comments as done.Jan 5 2018, 9:48 PM
Vulcan added a comment.Jan 5 2018, 9:48 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
vladislavbelov requested changes to this revision.Feb 21 2018, 5:46 PM
vladislavbelov added a subscriber: vladislavbelov.
vladislavbelov added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
147

Why system was removed?

147

I don't think, that it's good to have many similar names: settings-*. It'd be better to have the one settings node, like:

"settings": [
  "host": ...,
  "map": ...,
  ...
]
1388

These functions are pretty common, so they should be in common folder to be able use your code in other places (i.e. binaries/data/mods/public/globalscripts/utility.js).

Also we use lower camel case in CC.

1405

I agree with @elexis, we need to refactoring attributes.

1455

To many similar code, we need to avoid it.

This revision now requires changes to proceed.Feb 21 2018, 5:46 PM