Details
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
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/gui/tests/test_GuiManager.h | 29| class·TestGuiManager·:·public·CxxTest::TestSuite | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classTestGuiManager:' 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/706/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/749/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/237/display/redirect
Sorry that I was too lazy to review the original patch, one needs to know the circumstances of the hotkey code which takes some time to get back into again.
For the audit in rP22909 I looked all of that up again and the committed code indeed looks better than the previous code which called one event handler from the other handler.
I don't remember seeing a function with requests for two contexts in one scope, but it shouldn't make a difference (since it should be compiled to the same code if one of them is in a separate function, aside from closing the context sooner).
Moving hotkey.js hotkey.xml into a new hotkey folder would allow future GUI test pages to be distinguished, like we have in public/gui/ and mod/gui/. (Current directory contents gui_page.rng gui.rng hotkey.js hotkey.xml page_hotkey.xml)
(So I would recommend to do that but I dont insist)
Tested the patch with the barter dialog, and the new global hotkey method from the mainmenu (history / civ icon hotkeys), and ./binaries/system/test. So in case its broken too at least I tried.
| source/gui/CGUI.cpp | ||
|---|---|---|
| 76 ↗ | (On Diff #9792) | Correct to keep the ret assignment, if the program enters this scop there is definitely a handler performed. |
| source/gui/tests/test_GuiManager.h | ||
| 58 ↗ | (On Diff #9844) | -\n |
| 65 ↗ | (On Diff #9844) | JSContext* cx = scriptInterface.GetContext(); |
| 85 ↗ | (On Diff #9844) | JSContext* cxPage = pageScriptInterface.GetContext(); |
| 69 ↗ | (On Diff #9792) | (should be(come) possible to pass an empty shared_ptr?) |
TBH that's fine. Your time, like mine, is limited. You'll never have the time to really look at everything in advance, but if you look at what gets committed that optimises your time and the code still gets to where it needs to be.
Indeed this patch was fairly tricky, I too had to dive deep again before committing because I'd forgotten why it worked.
Moving hotkey.js hotkey.xml into a new hotkey folder would allow future GUI test pages to be distinguished, like we have in public/gui/ and mod/gui/. (Current directory contents gui_page.rng gui.rng hotkey.js hotkey.xml page_hotkey.xml)
Ah, I think I understand why - you mean that we can have different schemas then, correct?
I'll do that anyways, I just forgot til now.
Thanks for the comments :)
I dont know whether you want a different schema per test, but certainly we want different JS and XML files per test, so each test contains multiple files and we dont want the folder to store multiple files per test and multiple tests per folder.
A ps/trunk/binaries/data/mods/_test.gui/gui/gui.rng
A ps/trunk/binaries/data/mods/_test.gui/gui/gui_page.rng
A ps/trunk/binaries/data/mods/_test.gui/gui/hotkey.js
A ps/trunk/binaries/data/mods/_test.gui/gui/hotkey.xml
A ps/trunk/binaries/data/mods/_test.gui/gui/page_hotkey.xml
Consider adding one test for every current GUI page we have, how the folder would look like if there wasn't a subfolder per test compared to how it would look like if there was one.
For example if public/gui/ didn't have these subfolders:
./aiconfig/aiconfig.js ./aiconfig/aiconfig.xml ./civinfo/civinfo.js ./civinfo/civinfo.xml ./civinfo/setup.xml ./civinfo/sprites.xml ./civinfo/svn-commit.tmp ./civinfo/svn-commit.tmp.save ./civinfo/svn-commit.tmp.save.1 ./common/colorFades.js ./common/color.js ./common/functions_global_object.js ./common/functions_utility.js ./common/gamedescription.js ./common/global.xml ./common/music.js ./common/network.js ./common/settings.js ./common/setup_resources.xml ./common/setup.xml ./common/sprites.xml ./common/styles.xml ./common/tab_buttons.js ./common/tab_buttons.xml ./common/timer.js ./common/tooltips.js ./credits/credits.js ./credits/credits.xml ./credits/texts/art.json ./credits/texts/donators.json ./credits/texts/misc.json ./credits/texts/programming.json ./credits/texts/special.json ./credits/texts/translators.json ./gamesetup/gamesetup.js ./gamesetup/gamesetup.xml ./gamesetup_mp/gamesetup_mp.js ./gamesetup_mp/gamesetup_mp.xml ./gamesetup/setup.xml ./gamesetup/styles.xml ./loadgame/load.js ./loadgame/load.xml ./loadgame/SavegameDeleter.js ./loadgame/SavegameDetails.js ./loadgame/SavegameList.js ./loadgame/SavegameLoader.js ./loadgame/SavegameWriter.js ./loading/loading.js ./loading/loading.xml ./loading/sprites.xml ./loading/styles.xml ./lobby/lobby.js ./lobby/lobby_panels.xml ./lobby/lobby.xml ./locale_advanced/locale_advanced.js ./locale_advanced/locale_advanced.xml ./locale/locale.js ./locale/locale.xml ./manual/manual.xml ./options/options.js ./options/options.json ./options/options.xml ./page_aiconfig.xml ./page_atlas.xml ./page_civinfo.xml ./page_credits.xml ./page_gamesetup_mp.xml ./page_gamesetup.xml ./page_loadgame.xml ./page_loading.xml ./page_lobby.xml ./page_locale_advanced.xml ./page_locale.xml ./page_manual.xml ./page_options.xml ./page_pregame.xml ./page_prelobby_entrance.xml ./page_prelobby_login.xml ./page_prelobby_register.xml ./page_replaymenu.xml ./page_session.xml ./page_splashscreen.xml ./page_structree.xml ./page_summary.xml ./page_viewer.xml ./pregame/BackgroundHandler.js ./pregame/backgrounds/carthage.js ./pregame/backgrounds/carthage.xml ./pregame/backgrounds/hellenes.js ./pregame/backgrounds/hellenes.xml ./pregame/backgrounds/kush.js ./pregame/backgrounds/kush.xml ./pregame/backgrounds/seleucid.js ./pregame/backgrounds/seleucid.xml ./pregame/backgrounds.xml ./pregame/MainMenuItemHandler.js ./pregame/MainMenuItems.js ./pregame/mainmenu.js ./pregame/mainmenu.xml ./pregame/menupanel.xml ./pregame/ProjectInformation.js ./pregame/ProjectInformation.xml ./pregame/SplashscreenHandler.js ./pregame/sprites.xml ./pregame/styles.xml ./pregame/userreport/userreport.js ./pregame/userreport/userreport.xml ./prelobby/common/credentials/confirmpassword.xml ./prelobby/common/credentials/credentials.js ./prelobby/common/credentials/credentials.xml ./prelobby/common/credentials/rememberpassword.xml ./prelobby/common/feedback/feedback.js ./prelobby/common/feedback/feedback.xml ./prelobby/common/terms/termslobby.js ./prelobby/common/terms/termslobby.xml ./prelobby/entrance/entrance.js ./prelobby/entrance/entrance.xml ./prelobby/login/login.js ./prelobby/login/login.xml ./prelobby/register/register.js ./prelobby/register/register.xml ./reference/common/core.js ./reference/common/draw.js ./reference/common/helper.js ./reference/common/load.js ./reference/common/setup.xml ./reference/common/sprites.xml ./reference/common/styles.xml ./reference/structree/draw.js ./reference/structree/rows.xml ./reference/structree/sprites.xml ./reference/structree/structree.js ./reference/structree/structree.xml ./reference/structree/styles.xml ./reference/viewer/sprites.xml ./reference/viewer/styles.xml ./reference/viewer/viewer.js ./reference/viewer/viewer.xml ./replaymenu/replay_actions.js ./replaymenu/replay_filters.js ./replaymenu/replay_menu.js ./replaymenu/replay_menu.xml ./replaymenu/sprites.xml ./session/chat_window.xml ./session/developer_overlay.js ./session/developer_overlay.xml ./session/dialogs/yes_no.xml ./session/diplomacy_window.xml ./session/hotkeys/camera.xml ./session/hotkeys/misc.xml ./session/hotkeys/training.xml ./session/input.js ./session/menu.js ./session/menu.xml ./session/messages.js ./session/minimap_panel.xml ./session/objectives_window.xml ./session/placement.js ./session/selection_details.js ./session/selection.js ./session/selection_panels_helpers.js ./session/selection_panels.js ./session/selection_panels_left/alert_panel.xml ./session/selection_panels_left/barter_panel.xml ./session/selection_panels_left/formation_panel.xml ./session/selection_panels_left/garrison_panel.xml ./session/selection_panels_left/stance_panel.xml ./session/selection_panels_middle/multiple_details_area.xml ./session/selection_panels_middle/single_details_area.xml ./session/selection_panels_middle/unit_ally_commands.xml ./session/selection_panels_middle/unit_commands.xml ./session/selection_panels_right/construction_panel.xml ./session/selection_panels_right/gate_panel.xml ./session/selection_panels_right/pack_panel.xml ./session/selection_panels_right/queue_panel.xml ./session/selection_panels_right/research_panel.xml ./session/selection_panels_right/training_panel.xml ./session/selection_panels_right/upgrade_panel.xml ./session/session.js ./session/session_objects/panel_entities.xml ./session/session_objects/research_progress.xml ./session/session_objects/selection_group_icons.xml ./session/session.xml ./session/sprites.xml ./session/styles.xml ./session/top_panel/button_diplomacy.xml ./session/top_panel/button_game_speed.xml ./session/top_panel/button_menu.xml ./session/top_panel/button_objectives.xml ./session/top_panel/button_trade.xml ./session/top_panel/civ_icon.xml ./session/top_panel/label.xml ./session/top_panel/resource_population.xml ./session/top_panel/resources.xml ./session/top_panel.xml ./session/trade_window.xml ./session/tutorial_panel.xml ./session/unit_actions.js ./session/unit_commands.js ./splashscreen/setup.xml ./splashscreen/splashscreen.js ./splashscreen/splashscreen.xml ./summary/counters.js ./summary/layout.js ./summary/summary.js ./summary/summary.xml
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/275/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
source/gui/tests/test_GuiManager.h
| 29| class·TestGuiManager·:·public·CxxTest::TestSuite
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classTestGuiManager:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
binaries/data/mods/_test.gui/gui/hotkey/hotkey.js
| 4| function·handleInputBeforeGui(ev)·{
| | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
| | Opening curly brace appears on the same line as controlling statement.
binaries/data/mods/_test.gui/gui/hotkey/hotkey.js
| 10| function·handleInputAfterGui(ev)·{
| | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
| | Opening curly brace appears on the same line as controlling statement.
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/787/display/redirect
It seems the diff uploaded to here was correct, as it includes above diff, but not the one committed?
https://code.wildfiregames.com/D2295#96710
Indeed, that appears to be what I've done. I generally download the latest patch from Phabricator when committing, but I must have messed sphere, perhaps because my last downloaded file was already this diff but in an earlier version.
Sorry about the mess.