Page MenuHomeWildfire Games
Differential D1058

Ingame summary window as dialog
ClosedPublic
Actions

Authored by ffffffff on Nov 23 2017, 3:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 8, 4:39 AM
Unknown Object (File)
Tue, Sep 24, 9:48 AM
Unknown Object (File)
Sun, Sep 22, 12:30 AM
Unknown Object (File)
Sun, Sep 15, 8:03 PM
Unknown Object (File)
Tue, Sep 10, 12:29 PM
Unknown Object (File)
Sep 4 2024, 4:13 AM
Unknown Object (File)
Aug 31 2024, 11:01 AM
Unknown Object (File)
Aug 29 2024, 5:48 PM
View All 34 Files
Subscribers
bb
Hannibal_Barca
Imarok
temple
Restricted Owners Package

Details

Reviewers
elexis
Commits
rP20782: Display the summary screen as a dialog if it was opened during a running game…
Summary

When in game its realy annoying when open summary screen and its taking the whole screen so the game gets overlapped completely with summary window and the touch to the game gets lost. So as solution having the summary as dialog is much better than any transparency experiment.

Ingame

screenshot0591.png (768×1 px, 653 KB)

screenshot0592.png (768×1 px, 711 KB)

screenshot0593.png (768×1 px, 659 KB)

End summaryscreen still filled completely

screenshot0482.png (768×1 px, 687 KB)

Also in replays menu, perhaps having dialog (maybe to big)

screenshot0483.png (768×1 px, 591 KB)

Test Plan

discuss

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ffffffff created this revision.Nov 23 2017, 3:57 PM
ffffffff edited the summary of this revision. (Show Details)
ffffffff updated this revision to Diff 4338.Nov 23 2017, 4:00 PM
ffffffff edited the summary of this revision. (Show Details)

fade added

ffffffff edited the summary of this revision. (Show Details)Dec 4 2017, 6:40 PM
ffffffff edited the summary of this revision. (Show Details)
ffffffff retitled this revision from Summary window as dialog to Ingame summary window as dialog.Dec 11 2017, 8:19 PM
ffffffff edited the summary of this revision. (Show Details)
ffffffff edited the summary of this revision. (Show Details)
ffffffff mentioned this in D810: Hotkey summary, hotkey tabs and panel/charts remember.Dec 11 2017, 8:44 PM
ffffffff updated this revision to Diff 4721.Dec 11 2017, 8:52 PM

sync

ffffffff added a comment.Dec 21 2017, 12:50 PM

@temple comment here for ingame summary window as dialog window:)

temple added a comment.Dec 21 2017, 5:42 PM
In D1058#46564, @ffffffff wrote:

@temple comment here for ingame summary window as dialog window:)

Seems nice. (What else to say?)

ffffffff added a comment.Dec 21 2017, 6:13 PM

tx.

important.

elexis added a comment.Dec 21 2017, 6:37 PM

Having only the bottommost page in fullscreen and every other on top of it as a dialog means that the user will never assume that the topmost page is the bottommost page.
That is a reason that could lead to the patch being considered correct.

binaries/data/mods/public/gui/summary/summary.js
137 ↗(On Diff #4721)

So we can revert rP20111 or should the same approach be used here?

elexis added inline comments.Dec 21 2017, 6:39 PM
binaries/data/mods/public/gui/summary/summary.js
137 ↗(On Diff #4721)

If it's done via JS and if it were in a separate function initGUIWindow, initGUIWindowDecoration or initGUIWindowLocation, it would mean that the reader doesn't have to interpret this small hunk before reading a summary of what this function handles otherwise.

ffffffff added inline comments.Dec 21 2017, 7:05 PM
binaries/data/mods/public/gui/summary/summary.js
137 ↗(On Diff #4721)

lol

all

true

XD

ffffffff updated this revision to Diff 4897.Dec 22 2017, 11:51 PM
ffffffff edited the summary of this revision. (Show Details)
ffffffff added a parent revision: D1170: Function for converting window into dialog window convertToDialogWindow(name, size).
ffffffff updated this revision to Diff 5032.Jan 2 2018, 4:21 PM

Convert summary window to dialog window when wanted.

ffffffff edited the summary of this revision. (Show Details)Jan 2 2018, 4:21 PM
ffffffff removed a parent revision: D1170: Function for converting window into dialog window convertToDialogWindow(name, size).
ffffffff added a child revision: D854: Toggle summary of last multiplayer game in lobby.Jan 2 2018, 4:49 PM
ffffffff mentioned this in D854: Toggle summary of last multiplayer game in lobby.
elexis requested changes to this revision.Jan 2 2018, 8:01 PM

continueButton() shouldn't switch the GUI page if in replay menu but pop it.

binaries/data/mods/public/gui/summary/summary.js
142 ↗(On Diff #5032)

showAsDialogWindow -> dialog
should always be defined for any context, so nuke that isIngame check

call a function setupSummaryWindowStyle(data.dialog) and move all logic there

176 ↗(On Diff #5032)

Move the size from XML to here, or have two empty mock XML objects with the two sizes

sprite = dialog ? "...Dialog" : "...window";

just group everything

This revision now requires changes to proceed.Jan 2 2018, 8:01 PM
ffffffff updated this revision to Diff 5050.Jan 2 2018, 8:44 PM

pop

ffffffff updated this revision to Diff 5128.Jan 6 2018, 5:37 PM
elexis added a comment.Jan 6 2018, 8:03 PM

Looks good

binaries/data/mods/public/gui/summary/summary.js
140 ↗(On Diff #5128)

After reading the comments in this function, you might find a better place within this function to call the function.

174 ↗(On Diff #5128)

initGUIWindow

465 ↗(On Diff #5128)

Only check for g_GameData.dialog and pass the boolean in all calls

binaries/data/mods/public/gui/summary/summary.xml
27 ↗(On Diff #5128)

Was wondering if we should hide it for good measure. Perhaps it would be better for performance (wanna do a test)?

ffffffff updated this revision to Diff 5139.Jan 6 2018, 8:29 PM

y

binaries/data/mods/public/gui/summary/summary.js
140 ↗(On Diff #5128)

y

174 ↗(On Diff #5128)

y

465 ↗(On Diff #5128)

y

binaries/data/mods/public/gui/summary/summary.xml
27 ↗(On Diff #5128)

n

elexis mentioned this in rP20644: Implement hotkeys and mousescroll to change tabs in tabbed pages (credits….Jan 7 2018, 12:49 AM
elexis accepted this revision.Jan 7 2018, 2:07 AM

Actually on first sight it seems wrong to add a dialog property, because it gives the caller the freedom to switch the page with dialog = true or to push the page with dialog = false.
Instead we can get the information wheather it's a dialog or not directly from the GUI Manager like this:

guimanager.patch3 KBDownload

But on second sight it seems ok to have that one of these two freedoms, that is displaying the fullscreen variant when pushing instead of switching the GUI page - as is the case in the current code.

Panels exceed 1024 before and after, I've resigned about that.
It's quite bad that it's rendered beyond the dialog size.

It's not completely obvious whether we want the feature to begin with, since it has the disadvantage that we see less of the summary screen and are distracted by the map visible in the background.
But I accept it because it is only consistent if we use a dialog everywhere a page is stacked.

binaries/data/mods/public/gui/replaymenu/replay_actions.js
124 ↗(On Diff #5139)

In theory it should be opened as a dialog, but we cannot see the background, so the we cannot perceive it as a dialog but only as a messed up window. So keeping the switchpage.

132 ↗(On Diff #5139)

It's a "gui" property

binaries/data/mods/public/gui/summary/summary.js
177 ↗(On Diff #5139)

Unneeded negation

179 ↗(On Diff #5139)

If we add code, we should have a reason to add code.

If it's a dialog I get 80 FPS with the fade shown and 93 FPS without it.
If it's a window I get 100 FPS in both cases.

Since we always want to show it if it's a dialog, the line of code is useless according to this data.

460 ↗(On Diff #5139)

As mentioned, dialog should always be defined.

140 ↗(On Diff #5128)

No, it should have been moved below the // Output globals part.

It's the "input-processing-output" model and your new function is output, not input https://en.wikipedia.org/wiki/IPO_model https://de.wikipedia.org/wiki/EVA-Prinzip

This revision is now accepted and ready to land.Jan 7 2018, 2:07 AM
Closed by commit rP20782: Display the summary screen as a dialog if it was opened during a running game… (authored by elexis). · Explain WhyJan 7 2018, 2:12 AM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 7 2018, 2:12 AM
ffffffff added a comment.EditedJan 10 2018, 1:08 AM

maybe same darkness as fade as for dialog overlay with background color 0 0 0 200 alpha darkness https://imgur.com/a/VtRPu and little more gap from 16,24,16,24 to 24,28,24,28 px

na guess from style points its fine we have atm

ffffffff added inline comments.Jan 10 2018, 1:11 AM
ps/trunk/binaries/data/mods/public/gui/summary/summary.js
177

missing line here

Engine.GetGUIObjectByName("fadeImage").hidden = !g_GameData.gui.dialog;
ps/trunk/binaries/data/mods/public/gui/summary/summary.xml
27

for some reason i forgot to set this hidden or !hidden in js

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
gui/
replaymenu/
1 line
session/
1 line
1 line
summary/
16 lines
5 lines

Diff 5155

View Options

ps/trunk/binaries/data/mods/public/gui/replaymenu/replay_actions.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/menu.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/session/session.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/summary/summary.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/gui/summary/summary.xml

Loading...
Wildfire Games · Phabricator