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

Move QuietBackgroundProcesses UI to Windows customization #2511

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

nieubank
Copy link
Contributor

@nieubank nieubank commented Apr 1, 2024

Summary of the pull request

Move QuietBackgroundProcesses UI to Windows customization instead of a top-level page.

References and relevant issues

image

Detailed description of the pull request / Additional comments

Merge the UI project for QuietBackgroundProcesses into Customization and control the view's visibility based on the experimental features enablement status.

Validation steps performed

With the feature disabled in Settings -> Experimental features navigate to Windows customization and see that the Quiet mode card is not visible.

Enable the feature and notice that the settings card is visible upon navigating back to Windows customization.

Start a session, navigate away from the page and back and ensure the session remains active.

Close Dev Home and verify that DevHome.QuietBackgroundProcesses.ElevatedServer.exe is still running, launch, navigate back to Windows customization and stop session. Confirm that everything cleans up.

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

@nieubank nieubank requested a review from jsidewhite April 1, 2024 21:01
#nullable enable
private DevHome.QuietBackgroundProcesses.QuietBackgroundProcessesSession? _session;
private QuietBackgroundProcessesSession? _session;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nullable is enabled in this project, you might want to review these #nullable statements. Not sure if disabling it is what you want.

@jsidewhite
Copy link
Member

I'm good with these changes as-is, but @EricJohnson327 mentioned here that,

"The QuiteBackgroundProcesses stuff should go in its own tools/QuietBackgroundProcesses project. All code should be written as if it wasn't an experimental feature, then the experimental feature checks just wrap it to enable/disable it. That way when it's not experimental anymore, we just remove the checks and the rest of the code stays exactly the same."

I'm signing off, but giving Eric a chance to respond (I'm not sure if what he's saying is possible for WindCust anyway).

Thanks @nieubank!

Copy link
Member

@jsidewhite jsidewhite left a comment

Choose a reason for hiding this comment

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

:shipit:

private readonly TimeSpan _oneSecond = new TimeSpan(0, 0, 1);
private readonly IExperimentationService _experimentationService;

private readonly TimeSpan _zero = new(0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: TimeSpan.FromSeconds(1)

Comment on lines +37 to +38
<ctControls:SettingsCard x:Uid="QuietBackgroundProcessesExplanation" HorizontalContentAlignment="Left" >
</ctControls:SettingsCard>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ctControls:SettingsCard x:Uid="QuietBackgroundProcessesExplanation" HorizontalContentAlignment="Left" >
</ctControls:SettingsCard>
<ctControls:SettingsCard x:Uid="QuietBackgroundProcessesExplanation" HorizontalContentAlignment="Left" />

@nieubank nieubank merged commit edd2f28 into main Apr 3, 2024
4 checks passed
@nieubank nieubank deleted the user/nieubank/quiettocust branch April 3, 2024 00:06
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.

4 participants