Page MenuHomeWildfire Games

Button for quick change between civilization history and structure tree page
ClosedPublic

Authored by ffffffff on Aug 29 2017, 6:40 PM.

Details

Summary

Button for switching from structuretree to history page.

Helpful for in-game history page switch when in structure tree.
Most important for getting civ information and bonuses read in-game.
Best when random civ.

In gamesetup also switch to history page from structure tree (via info button) possible then.
For fast relookup, when history page/structure tree closed in gamesetup, it reopens same page again.

Switch button below next to close button on history page and structure tree page.

Test Plan

test ingame switch history, structure tree working correctly and in main menu

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
ffffffff edited the test plan for this revision. (Show Details)Aug 29 2017, 6:40 PM
ffffffff edited the summary of this revision. (Show Details)Aug 30 2017, 5:58 PM

futher we should eventualy considering renaming structuretree/* into civilizations/* and page_structuretree.xml into page_civilizations.xml when in time.

s0600204 added a subscriber: Restricted Owners Package.EditedAug 30 2017, 6:29 PM

This idea, or something very similar, has been suggested before:

Discussion should continue on from there.

  • @sanderd17 made the argument (in the above thread, comment 8) that as the civinfo page deals with real-world information, and the structree deals with game-specific information, the two should be kept separate.
  • From a technical perspective, the structree is already a fairly complicated and interwoven bit of scripting. Adding in the code and currently flawed approach of the civinfo page would make it more difficult to maintain.

Therefore, I'm currently personally against any attempt to merge the two pages. Feel free to make counter-arguments. πŸ˜‰


Edit: A formal decision wasn't actually made previously, so I shouldn't claim it was rejected. My mistake.

I agree with what sanderd17 said many months ago, i.e. that the civ page and the structure tree have two different use cases.
But IMO displaying both page contents in the same page seems nice, as one can switch quickly without having to close and reopen the dialogs each time.

So if the proposed GUI design is agreed upon, then it should be implemented in a way that the logic of the different pages should still be split in several files.
If there is code duplication, that can be nuked separately beforehand.
Also should avoid merge conflicts with the unit viewer thing.

ffffffff updated this revision to Diff 4316.Nov 22 2017, 12:58 AM
ffffffff retitled this revision from Merge civilization history and structure tree page to Button for quick change between civilization history and structure tree page.
ffffffff edited the summary of this revision. (Show Details)

updated to buttons.

ffffffff edited the summary of this revision. (Show Details)Nov 22 2017, 1:04 AM
ffffffff edited the summary of this revision. (Show Details)Dec 5 2017, 12:00 AM
ffffffff updated this revision to Diff 4836.Dec 21 2017, 3:32 PM

synccleans

ffffffff edited the summary of this revision. (Show Details)Dec 21 2017, 3:34 PM
ffffffff added subscribers: temple, bb, Imarok.
mapkoc added a subscriber: mapkoc.Dec 23 2017, 12:56 AM

OMG, please commit. History thing rarely read but this gives access to civ tree from game setup, a must.

Yes. Indeed. Very handsome. Also civ bonuses ingame, when get random civ or lookup from other civ team bonus for you. Teambonuses must somehow maybe been overlayed once maybe at start game what's you get from team mates.

Maybe messaging the player of the team bonus (s)he gets is better so they can be seen later in chat log. I guess some people dont know to press ` or F9 to get dev overlay.

elexis updated the Trac tickets for this revision.Jan 18 2018, 5:37 AM
ffffffff added a comment.EditedJan 25 2018, 11:25 PM

@s0600204 ur state needed
very nice idea @s0600204 !
if it doesn't come fast we go with this?

s0600204 requested changes to this revision.Jan 27 2018, 11:21 PM
  • If you're adding a g_Callback (string) variable, the g_CallbackSet (boolean) variable is no longer necessary.
    • Might be better named g_CallbackName as we're not storing the actual callback, just the name of it.
  • If a user closes the civinfo page in game, the tooltip of the Emblem continues to read {civ} - Structure Tree, but pressing opens the civinfo page.
    • Similarly, if a user closes the structree page in the game setup, the tooltip of the icon continues to read View civilisation info instead of reflecting the fact that pressing it will (at that point) open the structree.

@s0600204 ur state needed

$ getUserState("s0600204")
INDIVIDUAL.REVIEWING
binaries/data/mods/public/gui/civinfo/civinfo.js
14 β†—(On Diff #4836)
  1. Set a default value (data = {})
16 β†—(On Diff #4836)
  1. if (data.callback)
27 β†—(On Diff #4836)
  1. Remove line
28 β†—(On Diff #4836)
  1. Use data.civ instead of g_SelectedCiv (g_SelectedCiv gets set in selectCiv(), which gets called straight away when this line is run)
binaries/data/mods/public/gui/civinfo/civinfo.xml
120 β†—(On Diff #4836)

Can lose the <![CDATA[ (and ]]> below)

Ditto for other files.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
30 β†—(On Diff #4836)

Unused variable.

2466 β†—(On Diff #4836)

Is there any time that this function is called where data (or its attributes) is not defined?

binaries/data/mods/public/gui/reference/structree/structree.js
53 β†—(On Diff #4836)

Why switchToHistoryPage and not switchToCivInfoPage?

binaries/data/mods/public/gui/reference/structree/structree.xml
112 β†—(On Diff #4836)

Get the comments the correct way round, please.

121 β†—(On Diff #4836)

/n

binaries/data/mods/public/gui/session/menu.js
28 β†—(On Diff #4836)
/**
1098 β†—(On Diff #4836)

Layout code as before please.

1103 β†—(On Diff #4836)

And again, is there any time that this function is called and data is not defined?

This revision now requires changes to proceed.Jan 27 2018, 11:21 PM
ffffffff updated this revision to Diff 5545.Jan 28 2018, 1:45 PM
elexis added inline comments.Jan 28 2018, 2:23 PM
binaries/data/mods/public/gui/civinfo/civinfo.xml
7 β†—(On Diff #5545)

I'd prefer not including single files but keeping the directory structures in a way that allows us to only include entire directories or nothing.
What is the include used for?

binaries/data/mods/public/gui/gamesetup/gamesetup.js
2466 β†—(On Diff #4836)

Would be more readable if it were two statements rather than the destructuring assignment

21 β†—(On Diff #5545)

Could mention the purpose too (so the same civ and page is opened again when clicking on the button again?)

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
78 β†—(On Diff #5545)

Could move the fallback of g_CivInfo.page to the top of gamesetup.js, no?

binaries/data/mods/public/gui/manual/intro.txt
97 β†—(On Diff #5545)

(remember to update HotKeys after commit anyone)

binaries/data/mods/public/gui/reference/common/core.js
4 β†—(On Diff #5545)

default values should be avoided, because it allows the caller to not think about which values should be passed, which allows the caller to pass wrong values. better force them to think and pass correct values.

binaries/data/mods/public/gui/reference/structree/structree.js
12 β†—(On Diff #5545)

Not a function but a function name as s0600204 pointed out

binaries/data/mods/public/gui/session/menu.js
1103 β†—(On Diff #4836)

same about the civ

ffffffff added inline comments.Jan 28 2018, 2:29 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
21 β†—(On Diff #5545)

what

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
78 β†—(On Diff #5545)

what top

binaries/data/mods/public/gui/reference/common/core.js
4 β†—(On Diff #5545)

he dont need to think

binaries/data/mods/public/gui/reference/structree/structree.js
12 β†—(On Diff #5545)

if needed

ffffffff added inline comments.Jan 29 2018, 5:06 AM
binaries/data/mods/public/gui/civinfo/civinfo.xml
7 β†—(On Diff #5545)

Yes, I delete and use popGuiPageCB() directly in close function.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
2466 β†—(On Diff #4836)

Yes, sorry. Really wasn't not there when writing that code. :(

21 β†—(On Diff #5545)

Yes, omg sorry. :(

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
78 β†—(On Diff #5545)

really sorry about my answer wasn't really focused

ofc yes!
:(

binaries/data/mods/public/gui/reference/common/core.js
4 β†—(On Diff #5545)

Sry was lazy. :(
Wanted not to change all other code that uses this function. But as i see that it should not include a single file, it might be better, if its just implemented in the file itself, instead of using this function. So i leave this function now as it is.

binaries/data/mods/public/gui/reference/structree/structree.js
12 β†—(On Diff #5545)

omg

My bad. :(
Yes!

binaries/data/mods/public/gui/session/menu.js
1103 β†—(On Diff #4836)

Yes, sorry. :(

ffffffff updated this revision to Diff 5557.Jan 29 2018, 5:09 AM

Sorry @elexis for my bad comments!!!

Here the update yes really sorry :(!!

ok i make it ready

binaries/data/mods/public/gui/reference/structree/structree.xml
112 β†—(On Diff #4836)

What's wrong here?

s0600204 requested changes to this revision.Feb 1 2018, 5:45 PM

Much improved, thank you.

binaries/data/mods/public/gui/civinfo/civinfo.js
6 β†—(On Diff #5557)
/**
9 β†—(On Diff #5557)

As in structree.js below, the comment should probably mention that this contains the name, not the actual function.

Or rename the variable to g_CallbackName wherever it is used.

binaries/data/mods/public/gui/reference/structree/structree.js
11 β†—(On Diff #5557)
/**
binaries/data/mods/public/gui/reference/structree/structree.xml
112 β†—(On Diff #4836)

Comment says "History Page", but the following xml defines the close button.

Below that is a comment "Close dialog" which is followed by the xml definition of the button that opens the civinfo page.

This revision now requires changes to proceed.Feb 1 2018, 5:45 PM
ffffffff updated this revision to Diff 5624.Feb 1 2018, 6:50 PM
ffffffff updated this revision to Diff 5631.Feb 1 2018, 9:53 PM
s0600204 added inline comments.Feb 8 2018, 9:10 PM
binaries/data/mods/public/gui/civinfo/civinfo.js
29 β†—(On Diff #5631)

Afaik, the term "page" is not exposed to the user - it's what they're called in code, but the end user doesn't need to know that.

ack --type=js -i "\.tooltip.*page" reveals these (and the two later in this patch) are the only JS-set tooltips where "page" is mentioned.

ack --type=xml -i "tooltip.*page" reveals only one location where the word "page" is used in a tooltip - and that is inside the phrase "Click to open the 0 A.D. translate page in your browser." and refers to a webpage.

My point is, better tooltips would be "Switch to the Structure Tree" and "Close History" respectively.

binaries/data/mods/public/gui/civinfo/civinfo.xml
127 β†—(On Diff #5631)

\n

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1119 β†—(On Diff #5631)

There is a similar tooltip later on in session.js. Whilst this has hotkey1 / hotkey2: name1 / name2, the one later is press hotkey1 to open name1 \n press hotkey2 to open name2. This is inconsistent, so please decide on one way or the other.

Could also add a "Click to reopen whichever was last viewed" (or something to that effect) so the user knows what pressing the button will do.

Also, most other tooltips refer to the civinfo page as "History".

binaries/data/mods/public/gui/pregame/mainmenu.js
49 β†—(On Diff #5631)

I'm unsure about putting the hotkeys at the end of the tooltips. This is the only location (according to ack --type=js "\(hotkey\)") that it is done in this manner.

(There are one or two other cases of it being at the end, but they are incorporated as part of the sentence, ie. "you can also press %hotkey". The lines above do not do that.)

But then these are also the only options in the mainmenu that have hotkeys assigned.

binaries/data/mods/public/gui/reference/structree/structree.js
50 β†—(On Diff #5631)

And as earlier, "Switch to History" and "Close Structure Tree".

binaries/data/mods/public/gui/session/session.js
576 β†—(On Diff #5631)

Capitalisation - most everywhere else its "Structure Tree" and "Civilisation Info", not "structure tree" and "civilisation info".

In addition, most other tooltips refer to the civinfo page as "History".

See earlier comment about formatting.

ffffffff updated this revision to Diff 5726.Feb 8 2018, 10:51 PM

Just quickly tested in gamesetup and it feels nice. (especially the hotkeys)

s0600204 accepted this revision.Feb 21 2018, 10:42 PM
This revision is now accepted and ready to land.Feb 21 2018, 10:42 PM
This revision was automatically updated to reflect the committed changes.