Page MenuHomeWildfire Games

COList - allow hiding of columns
ClosedPublic

Authored by elexis on Mar 17 2017, 1:35 AM.

Details

Summary

We might want to add optional columns (f.e. D125), so the JS side should be able to hide them specifically.
Equal to the list_ property which is added for each column name, the patch adds a hidden_ property and
takes it into account when drawing the headers, the rows and when handling click events on the header.

Test Plan

Apply the patch in D125 and see it in action.
Since COList only has 470 lines, we can quickly observe that all occurances are taken into account.

We can test the validity of the rng file by running the game. If it contains a wrong entry, it will complain when encountering the hidden property in a column.
The validity of the RELAX NG compact syntax file can be tested by executing trang gui.rnc gui.rnc and observing that the resulting gui.rng works as advertized.

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

elexis created this revision.Mar 17 2017, 1:35 AM
Vulcan added a subscriber: Vulcan.Mar 17 2017, 2:19 AM

Build is green

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

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

vladislavbelov accepted this revision.Mar 17 2017, 3:02 AM
This revision is now accepted and ready to land.Mar 17 2017, 3:02 AM
elexis planned changes to this revision.Mar 17 2017, 4:02 AM

Should it support setting the property from XML too? Typicall good practice to keep JS and XML abilities in sync.
If so, the properties in COListColumn text color, text and width should be come settings too. Only the m_Id identifier remains.
So it seems like that COListColumn should be replaced by a vector of string identifiers.
Notice that getting the width would be needed for D125 too if we want to nuke the TODO.
Will become a rewrite I guess.
Could be done in several steps.
Notice how none of this would be needed if we would show that one column always in the first place.
On the other hand, more columns were thought about (victory condition, late observer flag, perhaps buddy count, observer count),
so that players with small resolutions (1024) only have very few columns that they prefer while HD users (1920 and beyond) can see all columns.

elexis requested review of this revision.Mar 25 2017, 4:35 PM

We discussed this a bit in irc. Indeed the struct should be nuked, but we thought about implementing the partial rewrite in several steps to have smaller diffs that are more easy to review.

This revision is now accepted and ready to land.Mar 25 2017, 4:35 PM
elexis planned changes to this revision.Mar 28 2017, 12:51 AM

As discussed with Vladislav on irc, the new setting should be able to be set from XML (not only JS).
We don't have to touch (remove) the struct in this proposal, nor have to change it, as the property is new.

elexis updated this revision to Diff 975.Mar 28 2017, 12:52 AM

Implement the new JS setting also as an XML property.

This revision is now accepted and ready to land.Mar 28 2017, 12:52 AM
elexis requested review of this revision.Mar 28 2017, 12:54 AM
elexis edited edge metadata.
vladislavbelov requested changes to this revision.EditedMar 28 2017, 1:28 AM

Why only .rng? .rnc should contain the new description of the column too.

This revision now requires changes to proceed.Mar 28 2017, 1:28 AM

Build is green

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

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

elexis updated this revision to Diff 976.Mar 28 2017, 2:18 AM
elexis edited edge metadata.

Add missing RELAX NG compact syntax entry that should always be kept in sync with the RELAX NG XML schema counterpart.

elexis edited the test plan for this revision. (Show Details)Mar 28 2017, 2:20 AM
vladislavbelov accepted this revision.Mar 28 2017, 2:20 AM
This revision is now accepted and ready to land.Mar 28 2017, 2:20 AM
This revision was automatically updated to reflect the committed changes.

Build is green

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

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