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

Hide console only if it's dedicated to the launcher process #184

Merged
merged 12 commits into from
May 3, 2022

Conversation

CarlosNihelton
Copy link
Collaborator

@CarlosNihelton CarlosNihelton commented Apr 27, 2022

This PR is somewhat opinionated, so I'd expect some pushback.

The current implementation hides the console window unconditionally, behavior which drove issue #176 .

Ideally the console window should be hidden if it was only opened for the solely purpose of invoking the launcher to install the distribution, which is the case when the user launches it from start menu or from the store, right after installing the app. It could also be launched from the Run dialog (I don't know a better name for that, the one we get when we hit Super+R). In that case the user could pass an arbitrary combination of command line arguments and still a console window would be opened dedicated to the launcher process.

In contrast, not even for the empty command line case we would want to hide the console window if the user is doing something else in another tab of the Windows terminal, for instance.

There is not a clear way to detect that. Nor could I find a Win32 API call that could satisfy my inquiry about how is the launcher being launched.

The method I chose for this is: detect the current position of the console screen buffer cursor. If it matches the origin, thus we have our own dedicated window and we can and should hide it. Otherwise, user was doing something else and we better not disturb her.

This only works if no console output is done before invoking the constructor of the SplashEnabledStrategy member of the Application class. This is easily achievable because the app object is created early in main() and its first member to be initialized is the strategy, which in turn calls the function responsible for memoizing whether at that moment the console cursor was at {0,0} as its first duty during construction (launchedInDedicatedConsole()). This sequencing is ensured by the order in which the members are declared inside the classes Application<> and SplashEnabledStrategy.

int wmain(int argc, wchar_t const *argv[])
{
    // Update the title bar of the console window.
    SetConsoleTitleW(DistributionInfo::WindowTitle.c_str());

    // Initialize a vector of arguments.
    std::vector<std::wstring_view> arguments;
    for (int index = 1; index < argc; index += 1) {
        arguments.push_back(argv[index]);
    }

    Oobe::Application<> app(arguments);

I'm writting that much because, although this works I don't know when in the future somebody well intentioned would stick a printf between those calls and ruin the algorithm. Since the function launchedInDedicatedConsole()memoizes its return value once called for the first time, we could drop at call to it as the first line of main(). It would look wierd and increase slightly the diff to upstream, so I avoided that path. I considered creating a static global constant to force run this function before main(), but then I think we cannot ensure not falling into the static initialization order fiasco, since I don't know for sure whether our static globals would be initialized before or after the console handles being assigned to our process by the operating system, required to check whether the console is at the origin or at some offset.

For now the alternative I'm considering is iterating through all opened process to find the launcher parent's name and decide whether the console should be hidden or not based on that. Seems less performant and more fragile, thus not my first option.

I'm open to discussion and better ideas. Otherwise, this closes #176 .

@didrocks
Copy link
Member

I think this will be better to discuss at a team meeting. I don’t really like the fact that it’s not straightfoward, and so, fragile which can lead to different user experience and issues.
Should we just not hide it then, and let it behind the splash screen + wslg window?

@jibel
Copy link
Collaborator

jibel commented Apr 28, 2022

I agree with @didrocks there. just not hide it and move other windows on top if possible.

@CarlosNihelton CarlosNihelton marked this pull request as draft April 29, 2022 20:32
@CarlosNihelton
Copy link
Collaborator Author

Digging into the AppxManifest documentation I learned about the Parameters option inside the UAP10 namespace, which allows specifying command line arguments with which the application should be launched. That serves almost perfectly to our use case and it is much more reliable, so I'm sticking with it. It affects launching the app from the Start Menu and from the store, but doesn't affect launching from command line, as expected. This PR now creates a new command line option to be used only in that context and all alone, not to be mixed with other existing command line options: --from-start-menu-or-store. If for curiosity user attempts to mix and match that parameter with pre-existing ones it may result in two scenarios:

  1. During installation: Invalid command line arguments, fallback installation is applied and ends with a help message printed on the console.
  2. During regular distro launching it is completely ignored.

To preserve the original experience with old releases I added specific versions of MyDistro.appxmanifest in the meta/src trees accordingly.

I'm not sure about why #188 is changing all line endings on the generated counter parts of the above mentioned appxmanifest files, so I'll leave for @didrocks appreciation before merging that one onto this branch. I double checked whether the committed files in src preserved CRLF line endings.

@CarlosNihelton CarlosNihelton marked this pull request as ready for review May 1, 2022 19:59
Meant for Start Menu / Microsoft Store only.
Application reacts to that option with:
interactive install + autodetect GUI support + hide console

Combined with other options results in either nothing or invalid options
It is meant for Store / Start menu launches, as the name suggests.
Applied by its own on an already registered distro means nothing.
Undocumented for that reason.
Now if splash gets closed by the user before its time, the console
could get stuck hidden because
this function tries to place it behind a window that doesn't exist.
@CarlosNihelton
Copy link
Collaborator Author

Line endings solved. This is finally ready for review.

@CarlosNihelton CarlosNihelton requested a review from didrocks May 2, 2022 11:28
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

So, went the flag path. I have some concerns on the name of the option itself.

Also, I emphase once again that we shouldn’t have a src/ in versioned directory and that their content should be what’s on root directories. This way, opening a new release does not force to copy any files or dirs.

@@ -54,6 +54,11 @@ namespace Oobe::internal

Opts parse(const std::vector<std::wstring_view>& arguments)
{
// launcher.exe --hide-console - Windows Shell GUI invocation as declared in the appxmanifest. Hides the
Copy link
Member

Choose a reason for hiding this comment

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

mismatch: the option is named --from-start-menu-or-store.

TBH, I prefer hide-console which is what the option will do instead of basing on a policy based on where it starts from (and which can evolve)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was the original idea, I changed my mind due the fact that later we may want to do other things besides hidding the console, but I guess you're right. I'm reverting to --hide-console.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

DistroLauncher/extended_cli_parser.h Outdated Show resolved Hide resolved
@CarlosNihelton
Copy link
Collaborator Author

The current state:

  • UAP10 namespace brought into the MyDistro.appxmanifest;
  • uap10:Parameters option passing the command line option --hide-console;
  • The change above affects launching from start menu or app installer, independent of which distro release;
  • No appxmanifest overrides in the meta source trees;
  • Command line parsing educated to warn about other options, but not this one;
  • Console will only be hidden if the CLI matches exactly launcher.exe --hide-console and nothing else.

Instead, we take advantage of the existing empty extended_messages.h files.
Those files override the in-source tree to avoid leaking the new behavior into the old releases.
Now they explicitly defines preprocessor constants that are evaluated to decide parsing the new command lines or just excluding them.
Avoids clang-format bounce between different formatting styles

For this specific declaration.  It doesn't have many knobs for adjusting template instantiation.
help for instance, would otherwise cause a warning.
@CarlosNihelton CarlosNihelton requested a review from didrocks May 2, 2022 20:55
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Concerns addressed, thanks for fixing! Let’s merge :)

@didrocks didrocks merged commit 2f39399 into main May 3, 2022
@didrocks didrocks deleted the hide-console-if branch May 3, 2022 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows terminal hidden on first run of ubuntu2204.exe
3 participants