Page MenuHomeWildfire Games

Add options to show tributes between allies and barter messages in observer mode
ClosedPublic

Authored by Sandarac on Jan 29 2017, 7:05 AM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19237: Show the tribute chat notification if a team member sends to another and if…
Trac Tickets
#4308
Summary

These options (enabled by default) should be added to the option menu. Tributes between allied players will not be shown if teams are not locked, even if the option is enabled. This patch contains code from @elexis .

As discussed in the trac ticket, eventually a new panel could be created where game notifications (for example, barter and tribute messages) would be shown.

Test Plan

If the option that shows tributes between allies is enabled, a message will be shown (but not if an ally player sends a tribute to an enemy, or vise versa). In observer mode, if the barter option is disabled, there will be no message shown if a player barters.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 547
Build 872: Vulcan BuildJenkins

Event Timeline

Sandarac created this revision.Jan 29 2017, 7:05 AM
Vulcan added a subscriber: Vulcan.Jan 29 2017, 7:56 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/264/ for more details.

Sandarac updated this revision to Diff 391.Jan 30 2017, 6:06 AM

Update so that observers will only see tribute messages if the config option is enabled.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/273/ for more details.

elexis requested changes to this revision.Jan 30 2017, 1:14 PM
elexis added a reviewer: elexis.

Thanks for bringing the patch back from the dead. I'll probably commit it if these remarks are addressed and if I don't find anything when testing.

binaries/data/config/default.cfg
331–332

Grouping those three notification types under [gui.session.notifications] would make it easier to read

335

Show a chat notification if an ally tributes resources to another team and all tributes in observermode?
Also notification and message are redundant

336

"active" part not needed as inactive ones can't barter anyway
"Show a chat notification to observers when a player bartered resources" seems grammatically better

binaries/data/mods/public/gui/options/options.json
108

Removing the allied part might be better as it also affects observermode (even if it leaves the wrong impression that it doesnt show tributes to the current player if disabled)

115

Same as default.cfg

binaries/data/mods/public/gui/session/messages.js
879

Optionally show...
no comma before "and", also "and" rather on the end of the previous line (similar to commas)

890

This also shows a notification if we sent resources to an ally? (Probably expected indeed).
But not if the option is disabled? Probably expected too.
In accordance with the other string, should use "You have sent" then

904

!g_IsObserver is more often the case than having the option disabled (which is enabled by default), so might want to switch those terms

This revision now requires changes to proceed.Jan 30 2017, 1:14 PM
elexis edited edge metadata.Jan 30 2017, 1:19 PM

Also at least ffm is annoyed by the attack notification message. It's not too hard to make that an option too and move those three into a separate panel below the general settings (10 line XML copy & paste) and adding a pair of braces to the JSON file. (Can be done somewhere else if you don't want to.)
And don't worry about the options dialog being crowded, we can in the future add tabs similar to the credits page to avoid crowding.

Sandarac updated this revision to Diff 404.Jan 31 2017, 12:21 AM
Sandarac edited edge metadata.

Update after the comments by elexis, and add a "Chat Notifications Settings" panel under "Lobby Settings" in the options menu.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/283/ for more details.

Sandarac marked 8 inline comments as done.Jan 31 2017, 7:08 AM
Sandarac added inline comments.
binaries/data/mods/public/gui/options/options.json
91

There was a small typo here.

elexis requested changes to this revision.Feb 8 2017, 7:31 PM

Sorry that I have to bother you with another revision, but the quality of the outcome improves each time!

binaries/data/mods/public/gui/options/options.json
267

Also shown for gaia, but no need to change the string

273

Falling into the trap of not setting locked teams everytime I'm trying to test this. The string must mention that peculiarity!
How about "Show a chat notification if an ally tributes resources to another team member if teams are locked, and all tributes in observermode"

binaries/data/mods/public/gui/options/options.xml
72

Lobby frame ends at 79%-52, notification panel starts at 72%, so that's messy.
It should always be the last value + some absolute border.
I propose to change the vertical size of the frames to 40%, 40%+8, 65%, 65%+8 so that they look more equally distributed and receive a border at the bottom of the new frame too.

binaries/data/mods/public/gui/session/messages.js
890

It is unexpected that the message is shown if we send tributes to an ally but not if we send tributes to an enemy.
Also it seems more coherent if we always show the notification if the target or source is our own player, independent of the configuration.

This revision now requires changes to proceed.Feb 8 2017, 7:31 PM

In case you had enough of this one, I can fix the remaining things if you like

Sandarac planned changes to this revision.Feb 15 2017, 5:28 AM
Sandarac marked an inline comment as not done.
In D106#5035, @elexis wrote:

In case you had enough of this one, I can fix the remaining things if you like

I just need to work on the xml.

Sandarac updated this revision to Diff 558.Feb 20 2017, 12:06 AM
Sandarac edited edge metadata.

Update.

Sandarac marked 2 inline comments as done.Feb 20 2017, 12:09 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/374/ for more details.

elexis accepted this revision.Feb 23 2017, 8:37 PM
elexis added inline comments.
binaries/data/config/default.cfg
342

strings should be identical in both occurances, locked teams part was missing here

binaries/data/mods/public/gui/options/options.xml
72

applied the uniform vertical distribution

binaries/data/mods/public/gui/session/messages.js
888

message =

894

Great, this is not only more consistent from the logical pov and corresponds to the players expectations but also much better to read now!

This revision is now accepted and ready to land.Feb 23 2017, 8:37 PM
This revision was automatically updated to reflect the committed changes.