Page MenuHomeWildfire Games

JS Error instead of debug_warn when attempting PopGuiPage too often
ClosedPublic

Authored by elexis on Sep 3 2019, 3:42 PM.

Details

Summary

If a modder or developer working on a GUI patch invalidly calls PopGUIPage when there is no page to pop (cant pop if there is only 1 page), then a debug_warn is triggered.
But mods shouldn't be able to trigger such "crashes" easily, especially when it can be caught easily and communicated to the JS side with JS syntax.

Test Plan

One could check whether the users to this new getter function might not want to be improved further somehow, and whether there might be a use case for exposing the entire stack instead of only the count (sounds like there might be a use case for that in the future when JS pages can communicate with each other, but not currently).
One could check whether more debug_warns or ENSUREs could be triggered from the JS GUI.
Notice that AutoRequest is necessary for ReportError, and it seems like its missing in many more places!

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.Sep 3 2019, 3:42 PM
Vulcan added a comment.Sep 3 2019, 3:44 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/62/display/redirect

Vulcan added a comment.Sep 3 2019, 3:51 PM

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

Linter detected issues:
Executing section Source...

source/gui/GUIManager.h
|  47| class·CGUIManager
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCGUIManager{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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

Why not keep the haspages function as well, which makes the intent more clear ?

source/gui/scripting/JSInterface_GUIManager.cpp
46 ↗(On Diff #9614)

Can't pop a GUI page when there are less than two pages open!
Can't pop GUI pages when there are less than two pages open!
Can't pop GUI pages when less than two pages are opened! <- That last one sounds the most english to me. @Gallaecio

In D2255#93731, @Stan wrote:

Why not keep the haspages function as well, which makes the intent more clear ?

Mostly don't see the need for a second getter if its already covered by the first one.

elexis added inline comments.Sep 4 2019, 5:37 PM
source/gui/scripting/JSInterface_GUIManager.cpp
46 ↗(On Diff #9614)

I'll take that (last) one, thanks.

elexis added inline comments.Sep 4 2019, 5:44 PM
source/ps/Game.cpp
323 ↗(On Diff #9614)

See https://en.cppreference.com/w/cpp/language/implicit_conversion

Boolean conversions
A prvalue of integral, floating-point, unscoped enumeration, pointer, and pointer-to-member types can be converted to a prvalue of type bool.
The value zero (for integral, floating-point, and unscoped enumeration) and the null pointer and the null pointer-to-member values become false. All other values become true.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 4 2019, 5:45 PM
This revision was automatically updated to reflect the committed changes.