Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework launchers #18847

Merged
merged 2 commits into from
Nov 27, 2020
Merged

Rework launchers #18847

merged 2 commits into from
Nov 27, 2020

Conversation

pchote
Copy link
Member

@pchote pchote commented Nov 22, 2020

This is the third and final PR backporting the project structure changes from #17989 for the playtest.
The first (new) commit splits the OpenRA.exe entry point into its own project, and changes OpenRA.Game to a library.
The second commit overhauls the windows launchers, taking a new approach that i'm surprised nobody considered earlier, using assembly attributes that can be passed to msbuild at compile time to avoid editing source files.

Depends on #18846.
Supersedes #18582.

Test builds: https://github.com/pchote/OpenRA/releases/tag/devtest-20201122

@pchote pchote added this to the Next Release milestone Nov 22, 2020
@pchote pchote requested a review from teinarss November 22, 2020 17:54
@pchote pchote changed the title Launcher projects Rework launchers Nov 22, 2020
@pchote
Copy link
Member Author

pchote commented Nov 24, 2020

Rebased.

@pchote
Copy link
Member Author

pchote commented Nov 25, 2020

Updated.

abcdefg30
abcdefg30 previously approved these changes Nov 26, 2020
Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm and works, but the packaging changes are untested. One thing I did notice is that the error dialogue doesn't always open in the foreground. Is there some sort of setting to do that?

@pchote
Copy link
Member Author

pchote commented Nov 26, 2020

The packaging changes (except for the latest revision, but I tested that locally and verified that both the 64 and 32 bit installers work) are tested by the devtest tag, with travis logs at https://travis-ci.com/github/pchote/OpenRA/builds/203989439

I could not find any way to force the error dialog on top. I don't think this is supported in SDL 😞

@abcdefg30
Copy link
Member

https://stackoverflow.com/questions/11512373/findwindow-and-setforegroundwindow-alternatives/12758966#12758966 seems to offer a solution. The crash dialogue seems to retain the name from the exe, so I could get one to the front locally by using "TiberianDawn" as process name.

@pchote
Copy link
Member Author

pchote commented Nov 26, 2020

That won't work because SDL.SDL_ShowMessageBox blocks until the dialog is closed, so we don't have the chance to run any code while the dialog is open. Calling SetForegroundWindow before opening the message box doesn't work either because the launcher's MainWindowHandle is 0 (i.e. null) because it doesn't have any windows.

@pchote
Copy link
Member Author

pchote commented Nov 27, 2020

I tried calling SetForegroundWindow on a different thread in several different ways, but couldn't get it to work (it would blink the icon in the taskbar, so something was happening, but the dialog was not brought to the front). I suspect this may be because the message box is not the MainWindowHandle, but I don't know how to get any other window handles.

I'm out of ideas, so can't proceed any further with that suggestion/request without help.

If anyone wants to try modifying the code to test ideas you can compile the TD launcher with

msbuild -t:Build "OpenRA.WindowsLauncher/OpenRA.WindowsLauncher.csproj" -restore -p:Configuration=Release -p:TargetPlatform="win-x64" -p:LauncherName="TiberianDawn" -p:ModID="cnc" -p:DisplayName="Tiberian Dawn" -p:FaqUrl="http://wiki.openra.net/FAQ"

and then copy TiberianDawn.exe from the bin dir into an existing Windows install for testing.

@pchote
Copy link
Member Author

pchote commented Nov 27, 2020

The behaviour I mentioned above is explained by this link:

An application cannot force a window to the foreground while the user is working with another window. Instead, Windows flashes the taskbar button of the window to notify the user.

So it sounds like we should be expecting this to fail.

@pchote
Copy link
Member Author

pchote commented Nov 27, 2020

The trick is that the game process needs to grant permission for the launcher to set the foreground window before it crashes. Added a new commit to fix this, which you can test using the msbuild command above.

@abcdefg30
Copy link
Member

That did the trick, good job. 👍

@pchote
Copy link
Member Author

pchote commented Nov 27, 2020

Open question: now that the platform launchers link against OpenRA.Game.dll, should we stop shipping the generic OpenRA.exe launcher in the windows builds? This will stop people from "crashing" if they try to run OpenRA.exe through the GUI.

@abcdefg30
Copy link
Member

If there is no need to ship the generic exe, then we should drop it.

@pchote
Copy link
Member Author

pchote commented Nov 27, 2020

Ok, done. I also scope creeped in replacing MakeLAA.cs/exe with a python script to remove another change from #17989.

New packages will be available from https://github.com/pchote/OpenRA/releases/tag/devtest-20201127-2

- Use SDL2 message boxes instead of Winforms.
- Use a proper project instead of compiling a single file.
- Use assembly attributes instead of modifying strings in the source code.
- Remove generic OpenRA.exe launcher.
- Replace MakeLAA.exe with a python script.
@pchote
Copy link
Member Author

pchote commented Nov 27, 2020

Updated. I also removed the Release-x86 configurations because I remembered that #17744 removed them everywhere else.

@teinarss teinarss merged commit d35768e into OpenRA:bleed Nov 27, 2020
@pchote pchote deleted the launcher-projects branch December 6, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants