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

Refactor Application Strategies #250

Merged
merged 4 commits into from
Jul 28, 2022
Merged

Conversation

CarlosNihelton
Copy link
Collaborator

@CarlosNihelton CarlosNihelton commented Jul 27, 2022

Why This PR

This refactoring won't change the current behavior.

  1. To break the existing app strategies were moved to their own "components" (.h/.cpp pair).

As new strategies appear as new files, conditional compilation will be held by the top level header ApplicationStrategy.h, while the Application.h still presents the highest level API-only class.

  1. Extract common behavior for the simpler strategies.

Those not requiring the interlocking between the installer and the slide show (aka splash screen) benefit from those implementations. When migrating the OOBE to Windows a new strategy very close to the NoSplashStrategy implemented for ARM64, but not exactly equal, would cause a lot of code duplication if those were not extracted.

Context Introduction

From the point of view of the launcher, the OOBE consists of an Application<> concrete class that offers extra command line parameter handling, and setup(), reconfigure() and runSplash() methods.

The splash functionality only exists on x64 platforms. Currently it is an application on its own that must be started and controlled (including acquiring the knowledge of its top level window handle to allow hiding and closing that window at strategic moments). That application is fed from the launcher standard console output for a period of its execution, before any action is required from the user to be taken at the console. Thus it has a SplashController with its own state machine takes care of that side of the application.

The setup and reconfiguration can be done in different ways, depending on whether WSLg is available, thus an InstallerController represents the possibilities as a state machine, enabling the higher level application class to react to whether the TUI or GUI was choosen and close or hide the splash window at the most appropriate moment, which differs on each situation.

That complexity of how to interact with those moving parts motivated splitting those controllers and building different AppStrategies for each situation. Currently the differentiating factor between those strategies is whether running the Flutter splash window is supported or not. As we move the GUI to Windows, another orthogonal dimmension is added and more strategies will arise. The higher level Application<> class takes care of the command line composition and expects from those underlying strategies the following public interface:

      public:
        /// Places the sequence of events to make the OOBE perform an automatic installation.
        HRESULT do_autoinstall(const std::filesystem::path& autoinstall_file);

        /// Places the sequence of events to make the OOBE perform an interactive installation.
        /// By default GUI support is checked before launching the OOBE, unless [forceTextMode]
        /// is set to true, in which case the check is skipped and the installer run in TUI mode.
        HRESULT do_install(Mode uiMode);

        /// Places the sequence of events to make the OOBE perform an automatic installation.
        HRESULT do_reconfigure();

        /// Triggers the console redirection and launch the splash screen application.
        void do_run_splash(bool hideConsole = false);
        

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/AppStrategyCommon.h Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.h Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.cpp Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.h Outdated Show resolved Hide resolved
@CarlosNihelton CarlosNihelton changed the title Common bits of the App strategies Refactor Application Strategies Jul 27, 2022
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/NoSplashStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/SplashEnabledStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/NoSplashStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/NoSplashStrategy.cpp Outdated Show resolved Hide resolved
DistroLauncher/NoSplashStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/NoSplashStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/NoSplashStrategy.cpp Outdated Show resolved Hide resolved
DistroLauncher/NoSplashStrategy.cpp Outdated Show resolved Hide resolved
@CarlosNihelton CarlosNihelton marked this pull request as ready for review July 27, 2022 16:01
DistroLauncher/ApplicationStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/ApplicationStrategy.h Outdated Show resolved Hide resolved
DistroLauncher/AppStrategyCommon.h Outdated Show resolved Hide resolved
Simpler strategies that lack the interlocking between the installer and the slide show benefit from those implementations.
When migrating the OOBE to Windows a new strategy very close to the NoSplashStrategy implemented for ARM64, but not exactly equal, would cause a lot of code duplication if those were not extracted.

This refactoring won't change the current behavior.
@CarlosNihelton
Copy link
Collaborator Author

I squashed down all the changes while rebasing to avoid merge conflicts. Essentially, the last changes were the renamings you suggested and using instead of #define.

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/ApplicationStrategyCommon.h Outdated Show resolved Hide resolved
@CarlosNihelton CarlosNihelton merged commit a06e223 into main Jul 28, 2022
@CarlosNihelton CarlosNihelton deleted the common-strategy-functions branch July 28, 2022 11:29
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.

2 participants