Simplify a function in the Credits page
ClosedPublic
Actions

Authored by elexis on Aug 12 2017, 10:56 AM.

Details

Reviewers

vladislavbelov

Commits

rP19989: Simplify Tab handling of the credits page.

Summary

The tabbing code in the credits page uses 6 lines of code, including a global variable where 2 lines of code and no global are sufficient.
The disadvantage of the patch is that we now have few worse performance by few microseconds in case someone clicks on a tab.
The advantage is duplicating as few code as possible in case we add more pages with tabs (while not providing some C++ object type for tabs).

Test Plan

Main Menu -> Credits -> some clicking.

Notice there should be the rename ForegroundBox -> TabForeground (same for background) in either this patch (credits.js) or D786.

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.Aug 12 2017, 10:56 AM

elexis mentioned this in D786: Move credits page tab style to Modern.Aug 12 2017, 2:13 PM

elexis added a parent revision: D786: Move credits page tab style to Modern.Aug 12 2017, 2:14 PM

elexis edited the test plan for this revision. (Show Details)

Looks good as those lines don't seem to be a bugfix. I'd ask @Itms if you haven't already done so.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

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

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/credits/credits.js
|  41| »   »   button.onPress·=·(i·=>·function()·{selectPanel(i);})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/credits/credits.js
|  41| »   »   button.onPress·=·(i·=>·function()·{selectPanel(i);})(i);
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.
Executing section XML GUI...
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/credits/credits.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/credits/credits.xml
|   1|   1| <?xml version="1.0" encoding="utf-8"?>
|   2|    |-
|   3|   2| <!--
|   4|   3| ==========================================
|   5|   4| - CREDITS PAGE -
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/credits/credits.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/credits/credits.xml
|   5|   5| - CREDITS PAGE -
|   6|   6| ==========================================
|   7|   7| -->
|   8|    |-
|   9|   8| <objects>
|  10|   9| 	<script file="gui/common/functions_global_object.js"/>
|  11|  10| 	<script directory="gui/credits/"/>
|    | [INFO] XMLBear:
|    | XML can be formatted better.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/credits/credits.xml
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/credits/credits.xml
|  28|  28| 		</object>
|  29|  29| 
|  30|  30| 		<object type="image" sprite="ModernFade" size="220 30 100%-20 100%-54">
|  31|    |-			<object name="creditsText" type="text" style="textPanel" scroll_top="true" />
|    |  31|+			<object name="creditsText" type="text" style="textPanel" scroll_top="true"/>
|  32|  32| 		</object>
|  33|  33| 
|  34|  34| 		<!-- Close dialog -->
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/386/ for more details.

Harbormaster completed remote builds in B2774: Diff 3087.Aug 12 2017, 2:56 PM

It works after applying D786 on Windows 8.1.

binaries/data/mods/public/gui/credits/credits.js
87 ↗	(On Diff #3087)	This could be written like: Engine.GetGUIObjectByName("creditsPanelButtons").children.forEach( (button, j) => button.sprite = i == j ? "TabForeground" : "TabBackground"); But it doesn't make sense, because in this case it returns `bool`.

This revision is now accepted and ready to land.Aug 13 2017, 10:01 PM

elexis mentioned this in D797: New order categories in options.Aug 18 2017, 3:21 PM

Thanks for the review

binaries/data/mods/public/gui/credits/credits.js
87 ↗	(On Diff #3087)	Yep, good practice to add the function braces. Not adding them leaves the impression that we want to return a value. (There was a concern raised for that for that reason some weeks ago, so it's commonly accepted.)