Page MenuHomeWildfire Games
Differential D2822

Rewrite the civinfo page to use OOP principles
ClosedPublic
Actions

Authored by s0600204 on Jun 18 2020, 12:43 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24289: Rewrite the civinfo page to use OOP principles
Trac Tickets
#5387
Summary

Continuation of the general effort to convert gui pages to use OOP principles.

There are two visual alterations:

  • Scrollbars have been enabled on the textboxes (only appearing when necessary).
  • The "Special Technologies"/"Special Structures" subsection has been split in two (separation of concerns).

Note: This revision is a step towards adapting the civinfo page to load information from entity/technology/aura data files - where relevant - instead of from the {civ}.json files. (This is why the civinfo codebase has been relocated into the gui/reference folder.)

Requires D2734

Test Plan
  • Apply revision;
  • Run 0A.D.;
  • Open civinfo page;
  • Confirm it runs correctly.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

s0600204 created this revision.Jun 18 2020, 12:43 AM
s0600204 added a parent revision: D2734: Rewrite Structure Tree and Template Viewer to use OOP principles.
s0600204 mentioned this in D759: Adapt civinfo page to load relevant info from templates..Jun 18 2020, 12:50 AM
s0600204 added a child revision: D759: Adapt civinfo page to load relevant info from templates..
s0600204 updated this revision to Diff 12703.Jul 16 2020, 5:29 AM

Rebase of work

Vulcan added a comment.Jul 16 2020, 5:38 AM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2657/display/redirect

Harbormaster completed remote builds in B12420: Diff 12703.Jul 16 2020, 5:48 AM
wraitii added a subscriber: wraitii.Jul 16 2020, 10:03 AM

Reasonable changes overall. I think this is a good example of code that would be improved by merging JS and XML together (D2805).

I have one remark on StructTreeButton which seems completely dispensable to me and weirdly located.

binaries/data/mods/public/gui/reference/common/Buttons/StructreeButton.js
27 ↗(On Diff #12703)

I don't really understand what this is doing in common/

Freagarach added a subscriber: Freagarach.Jul 16 2020, 10:26 AM

Scrollbars have been enabled on the textboxes (only appearing when necessary).

Could you also allow them on the "History" text?

binaries/data/mods/public/gui/reference/civinfo/CivInfoPage.js
73 ↗(On Diff #12703)

(+.)

s0600204 updated this revision to Diff 12731.Jul 17 2020, 3:44 AM
s0600204 marked an inline comment as done.

Enable scrolling on the History textbox

binaries/data/mods/public/gui/reference/common/Buttons/StructreeButton.js
27 ↗(On Diff #12703)

For much the same reason that there's a CivInfoButton{.xml|.js} in the same location - so it may be used in other pages in future pages without inadvertently including the entire civinfo or structree codebases.

(In case you're wondering how likely that is - three/four years ago I was working on a gui page that, if I were to resurrect, would benefit from both buttons.)

Vulcan added a comment.Jul 17 2020, 3:50 AM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2673/display/redirect

Harbormaster completed remote builds in B12445: Diff 12731.Jul 17 2020, 4:03 AM
wraitii added a comment.Jul 17 2020, 9:38 AM

I'm getting some errors:

ERROR: CCacheLoader failed to find archived or source file for: "gui/reference/civinfo/sprites.xml"
ERROR: CCacheLoader failed to find archived or source file for: "gui/reference/civinfo/setup.xml"
ERROR: Cannot find tooltip object named 'civInfoTooltip'
ERROR: Cannot find tooltip named 'civInfoTooltip'
ERROR: Cannot find tooltip named 'civInfoTooltip' or it is not a tooltip
binaries/data/mods/public/gui/reference/common/Buttons/StructreeButton.js
27 ↗(On Diff #12703)

My point is more that it doesn't actually seem to provide much functionality, and thus might be worth duplicating.

As a matter of fact, I'm not a fan of making buttons with specific behaviours classes of their own (so the same applies to CloseButton I guess). It might be worth putting the text in JSON files and then just defining a 'button' somewhere, but meh.

Anyways, this isn't really a dealbreaker to me.

s0600204 added a comment.Jul 17 2020, 4:24 PM
In D2822#124288, @wraitii wrote:

I'm getting some errors:

[...]

I can't reproduce, sorry. Applying this revision from phabricator with arc works for me on both my svn and git working copies, and it runs without noticeable error.

Nescio added a subscriber: Nescio.Jul 17 2020, 4:31 PM
  • Apply revision;
  • Run 0A.D.;
  • Open civinfo page;
  • Confirm it runs correctly.

For what's it worth, I just executed the above and didn't encounter any errors.

Freagarach added a comment.Nov 26 2020, 11:44 AM

Works as advertised :) From the main menu, from the match setup menu and from within a match.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 29 2020, 5:21 AM
Closed by commit rP24289: Rewrite the civinfo page to use OOP principles (authored by s0600204). · Explain Why
This revision was automatically updated to reflect the committed changes.
s0600204 added a commit: rP24289: Rewrite the civinfo page to use OOP principles.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
civinfo/
153 lines
141 lines
14 lines
25 lines
common/
8 lines
12 lines
reference/
civinfo/
122 lines
Sections/
30 lines
23 lines
23 lines
29 lines
7 lines
Subsections/
35 lines
9 lines
30 lines
9 lines
24 lines
9 lines
30 lines
9 lines
12 lines
39 lines
14 lines
25 lines
10 lines
common/
Buttons/
27 lines
8 lines
l10n/
4 lines

Diff 14268

View Options

ps/trunk/binaries/data/mods/public/gui/civinfo/civinfo.js

Loading...
View Options

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

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/civinfo/setup.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/civinfo/sprites.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/common/functions_utility.js

Loading...
View Options

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

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/CivInfoPage.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/GameplaySection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/GameplaySection.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/HistorySection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/HistorySection.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/BonusesSubsection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/BonusesSubsection.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/HeroesSubsection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/HeroesSubsection.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/StructuresSubsection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/StructuresSubsection.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/TechnologiesSubsection.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/Sections/Subsections/TechnologiesSubsection.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/civinfo.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/civinfo.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/setup.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/sprites.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/civinfo/styles.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/common/Buttons/StructreeButton.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/reference/common/Buttons/StructreeButton.xml

Loading...
View Options

ps/trunk/binaries/data/mods/public/l10n/messages.json

Loading...
Wildfire Games · Phabricator