Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4237 closed defect (fixed)

[PATCH] Diplomacy window should show player's defeated/active states.

Reported by: javiergodas Owned by: javiergodas
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by Vladislav Belov)

Sometimes while playing in multiplayer, some opponents or allies might disconnect in middle of the battle, and if you get distracted, you may miss the notification saying that they will be back or they surrendered. So you might keep playing thinking that your ally could be alive when he surrendered, or either quit thinking that your ally gave up. Also, when you want to end the game, and you see no enemy units, you always ask your mates who's left. It would be nice to see which ones are dead so you can refinate your search. This is a continuation of ticket #4187.

Attachments (9)

DiplomacyWinLosePatch.diff (4.4 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatch.2.diff (4.4 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatch.png (586.7 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatch.3.diff (4.4 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatch.4.diff (4.4 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatch.5.diff (4.4 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatchFinal.diff (4.5 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatchFinal.2.diff (4.5 KB ) - added by javiergodas 7 years ago.
DiplomacyWinLosePatch_EvenMoreFinalThanBefore.diff (5.8 KB ) - added by elexis 7 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 by javiergodas, 8 years ago

Description: modified (diff)

comment:2 by elexis, 8 years ago

Description: modified (diff)

comment:3 by Imarok, 8 years ago

subset of #3787?

comment:4 by javiergodas, 8 years ago

No, it's more an extension of #4187.

comment:5 by javiergodas, 8 years ago

Owner: set to javiergodas
Status: newassigned

comment:6 by javiergodas, 8 years ago

Milestone: BacklogAlpha 22
Status: assignednew

comment:7 by javiergodas, 8 years ago

On it

Last edited 8 years ago by javiergodas (previous) (diff)

by javiergodas, 7 years ago

Attachment: DiplomacyWinLosePatch.diff added

by javiergodas, 7 years ago

by javiergodas, 7 years ago

Attachment: DiplomacyWinLosePatch.png added

by javiergodas, 7 years ago

by javiergodas, 7 years ago

by javiergodas, 7 years ago

comment:8 by javiergodas, 7 years ago

Keywords: rfc patch added

comment:9 by elexis, 7 years ago

Keywords: review added; rfc removed
Priority: Must HaveShould Have

Didn't test yet, but code looks correct now

comment:10 by wraitii, 7 years ago

Seems to work correctly. I'm not sure the icons are the most obvious way to do this though.

comment:11 by Imarok, 7 years ago

The patch looks good, but there are two "style issues":

  • You should use jsdoc comments for the new function in gui/commons/gamedescription.js (like how its done for the other functions in this file).
  • binaries/data/mods/public/gui/session/menu.js: L340 has a trailing whitespace

Furthermore the patch needs to be rebased.

When those are fixed, I'll commit it ;)

Last edited 7 years ago by Imarok (previous) (diff)

comment:12 by javiergodas, 7 years ago

Hope this fixes it.

by javiergodas, 7 years ago

by javiergodas, 7 years ago

comment:13 by Imarok, 7 years ago

please rebase your patch (update your svn or git and create a diff again)

comment:14 by elexis, 7 years ago

Keywords: simple removed
  • Additionally to the proposed feature, we could display the defeated/won icon in the lobby / replay menu list. In order to accomplish that, we would need a new sprite and style definition, so that we can add the image also for texts. For example, after having added an icon with that name to gui/common/setup.xml, the translateWithContext("playerstate", "defeated") string could be removed and instead [icon="icon_defeated"] would be inserted at the beginning of the string. Unfortunately we wouldn't be able to set a tooltip, so not sure if it's the better choice after all. (Furthermore to avoid duplication of the defeated icon mapping, we wouldn't use an image for the player outcome, but a string as well that would be set to [icon="icon_defeated"]. (I repeat in advance: Don't implement this until we decided on this proposal).
  • source/gui/ bug: The tooltip in the diplomacy screen is visible for all players except the first. The first item will always display the tooltip property defined in the XML (here empty string). Weirdly this bug isn't reproducible with the tooltip of the image in the player assignment menu of the gamesetup which also uses repeat.
  • The icon is given 25px width (diplomacyPlayerOutcome), but diplomacyPlayerName starts at 20px.
  • Perhaps the icon would look better if the size is increased a bit.
  • The tooltip should use the sessionToolTipBold style
  • Trailing whitespace in gamedescription.js
  • The <script file="gui/common/gamedescription.js"/> call should be near the other gui/common calls, in alphabetical order (unless explicitly needed). Removing proposed comment as we only add the comments if we want to show that a weird loading order is needed, which is the case for layout.js, but not for gamedescription.js.
Last edited 7 years ago by elexis (previous) (diff)

comment:15 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:16 by Vladislav Belov, 7 years ago

Description: modified (diff)
Summary: Diplomacy window should show player's defeated/active states.[PATCH] Diplomacy window should show player's defeated/active states.

Isn't the ghost="true" missed in the diplomacy_window.xml:46?

Last edited 7 years ago by Vladislav Belov (previous) (diff)

comment:17 by Imarok, 7 years ago

Resolution: fixed
Status: newclosed

In 19199:

Show the defeat/active state of players in the diplomacy window
Patch by javiergodas, rebased and negative review by elexis. Fixes #4237.

comment:18 by Imarok, 7 years ago

Keywords: review removed
Milestone: Work In ProgressAlpha 22

comment:19 by elexis, 7 years ago

Imarok rebased and fixed javiergodas' patch and it was discussed and reviewed in irc today. Imarok tested how icons would look: https://code.wildfiregames.com/file/data/cwngoyim4l2fx4b6p2n5/PHID-FILE-7um4mnygelhhvcjoxhhu/screenshot0009.png at https://code.wildfiregames.com/F20279 Notice without tooltips, the icons won't be preferable over text.

in reply to:  16 comment:20 by elexis, 7 years ago

Replying to vladislavbelov:

Isn't the ghost="true" missed in the diplomacy_window.xml:46?

Yep, that was the bug that caused the tooltip of the first icon to disappear, since the rows of inactive players were not resized, thus appearing in front of the first icon. This was solved by just hiding every row unless it's used and removing all ghost properties (instead of removing ghost properties of the active rows)

Note: See TracTickets for help on using tickets.