Page MenuHomeWildfire Games
Differential D106

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

Authored by Sandarac on Jan 29 2017, 7:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 13, 11:08 AM
Unknown Object (File)
Sun, Sep 8, 3:27 PM
Unknown Object (File)
Thu, Sep 5, 8:36 AM
Unknown Object (File)
Thu, Sep 5, 8:30 AM
Unknown Object (File)
Wed, Sep 4, 6:48 AM
Unknown Object (File)
Wed, Sep 4, 6:48 AM
Unknown Object (File)
Wed, Sep 4, 6:48 AM
Unknown Object (File)
Mon, Sep 2, 10:56 AM
View All 25 Files
Subscribers
elexis
Vulcan

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…
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 Not Applicable
Unit
Tests Not Applicable

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.

Harbormaster completed remote builds in B389: Diff 380.Jan 29 2017, 7:56 AM
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.

Vulcan added a comment.Jan 30 2017, 6:50 AM

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.

Harbormaster completed remote builds in B400: Diff 391.Jan 30 2017, 6:50 AM
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
332 ↗(On Diff #391)

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

336 ↗(On Diff #391)

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

337 ↗(On Diff #391)

"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 ↗(On Diff #391)

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 ↗(On Diff #391)

Same as default.cfg

binaries/data/mods/public/gui/session/messages.js
879 ↗(On Diff #391)

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

891 ↗(On Diff #391)

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

903 ↗(On Diff #391)

!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.

Vulcan added a comment.Jan 31 2017, 1:39 AM

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.

Harbormaster completed remote builds in B414: Diff 404.Jan 31 2017, 1:39 AM
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 ↗(On Diff #404)

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 ↗(On Diff #404)

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

273 ↗(On Diff #404)

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 ↗(On Diff #404)

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 ↗(On Diff #404)

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
elexis added a comment.Feb 14 2017, 12:24 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
Vulcan added a comment.Feb 20 2017, 12:51 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.

Harbormaster completed remote builds in B547: Diff 558.Feb 20 2017, 12:51 AM
elexis accepted this revision.Feb 23 2017, 8:37 PM
elexis added inline comments.
binaries/data/config/default.cfg
342 ↗(On Diff #558)

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

binaries/data/mods/public/gui/options/options.xml
72 ↗(On Diff #404)

applied the uniform vertical distribution

binaries/data/mods/public/gui/session/messages.js
888 ↗(On Diff #558)

message =

894 ↗(On Diff #558)

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
Closed by commit rP19237: Show the tribute chat notification if a team member sends to another and if… (authored by elexis). · Explain WhyFeb 23 2017, 8:37 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
config/
6 lines
mods/
public/
gui/
options/
23 lines
19 lines
session/
20 lines

Diff 587

View Options

ps/trunk/binaries/data/config/default.cfg

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/options/options.json

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/options/options.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/messages.js

Loading...
Wildfire Games · Phabricator