[File Explorer] Added an overall toggle switch (#23723) #33475

acekirkpatrick · 2024-06-21T22:32:38Z

Summary of the Pull Request

Added an enable toggle switch for File Explorer.

PR Checklist

Closes: Add toggle switch to disable all File Explorer addons #23723, [Settings > Dashboard] Explorer add-ons missing #29827
Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
Tests: Added/updated and all pass
Localization: All end user facing strings can be localized
Dev docs: Not necessary
New binaries: None necessary
Documentation updated: Updated group policy docs and picture of File Explorer page MicrosoftDocs/windows-dev-docs#5103

Detailed Description of the Pull Request / Additional comments

Added an overall GPO for File Explorer.
Added it as a module to the dashboard and all apps popup.
Stopped File Explorer Preview from running if it isn't enabled in the settings.

Validation Steps Performed

Setting saved to JSON

Opened Settings
Flipped the switch
Exited the runner
Opened Settings
The setting had been saved correctly

Previews were not run if disabled

Built and ran installer
Had previews and thumbnails ON
Previews and thumbnails ran and were visible
Turned the switch OFF
Previews and thumbnails no longer showed up
Turned the switch ON
Previews and thumbnails appeared again

GPO

Logged in as an admin
Set the group policy for File Explorer add-ons to disabled
The card was disabled and showed a lock icon, the switch was disabled and had a notification saying the setting was controlled, and the previews did not appear
Set the group policy to enabled
Restarted PowerToys
Everything was still locked, but the previews showed up and the switch was on

Screenshots

Module Policy Not Configured

Module Policy Disabled

Module Policy Enabled

…disabled

htcfreek · 2024-06-24T05:51:40Z

@acekirkpatrick
Plz ping me for review if the PR is ready.

acekirkpatrick · 2024-06-25T22:24:49Z

@htcfreek
It's ready as far as I can tell!

htcfreek · 2024-06-26T02:05:54Z

@acekirkpatrick
Can you please provide screenshots? That makes it easier to review.

acekirkpatrick · 2024-06-26T16:26:36Z

@htcfreek
I put some screenshots in the description.

htcfreek

I did not test it with a local build yet. But I left some comments.

htcfreek · 2024-06-26T16:57:14Z

src/settings-ui/Settings.UI.Library/EnabledModules.cs

-        [JsonPropertyName("File Explorer Preview")]
+        [JsonPropertyName("File Explorer")]


Does this change breake existing settings that users made?

The property PowerPreview had no references in the source code before.

htcfreek · 2024-06-26T16:58:00Z

src/gpo/assets/PowerToys.admx

@@ -19,6 +19,7 @@
      <definition name="SUPPORTED_POWERTOYS_0_78_0" displayName="$(string.SUPPORTED_POWERTOYS_0_78_0)"/>
      <definition name="SUPPORTED_POWERTOYS_0_81_0" displayName="$(string.SUPPORTED_POWERTOYS_0_81_0)"/>
      <definition name="SUPPORTED_POWERTOYS_0_81_1" displayName="$(string.SUPPORTED_POWERTOYS_0_81_1)"/>
+      <definition name="SUPPORTED_POWERTOYS_0_82_0" displayName="$(string.SUPPORTED_POWERTOYS_0_82_0)"/>


Will be 0.83.0 . Please update all occurrences.

htcfreek · 2024-06-26T16:59:32Z

src/settings-ui/Settings.UI/SettingsXAML/Views/PowerPreviewPage.xaml

+                    IsTabStop="{x:Bind ViewModel.IsEnabledGpoConfigured, Mode=OneWay}"
+                    Severity="Informational" />
+
+                <controls:SettingsGroup x:Uid="FileExplorerPreview_PreviewPane" IsEnabled="{x:Bind ViewModel.IsEnabled, Mode=OneWay}">
                    <InfoBar


Please hide all this info bars that are not related to the overall Module enabled GPO when the GPO is disabled. (We make it on the other pages the same.)

The incompatibility warnings and the reboot required notice should be hidden when the GPO is set to Disabled?

Or should I hide them whenever the switch is off regardless of GPO setting?

The incompatibility warnings and the reboot required notice should be hidden when the GPO is set to Disabled?

yes.

should I hide them whenever the switch is off regardless of GPO setting?

yes

You can implement a view model property "showInfobars" that is false if gpo is disabled or toggle is disabled (overall: if toggle is not enabled).

I just used IsEnabled. Is that okay?

If it works as expected, yes. I didn't thought about this solution yet.

htcfreek · 2024-06-26T17:01:32Z

src/gpo/assets/PowerToys.admx

    <policy name="ConfigureEnabledUtilityFileExplorerSVGPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerSVGPreview)" explainText="$(string.ConfigureEnabledUtilityDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerSVGPreview">
      <parentCategory ref="PowerToys" />


We could move all this File Explorer preview GPOs into a new subcategory "File Previewers". That makes it easier to find the GPOs for the single previewers. What do you think?

I was thinking about that when I made the GPO. I think we should. It was hard to find the overall GPO even when I knew exactly what I was looking for.

But the overall gpo has to keep in the main folder.

htcfreek · 2024-06-26T17:03:59Z

src/gpo/assets/PowerToys.admx

@@ -136,6 +137,16 @@
        <decimal value="0" />
      </disabledValue>
    </policy>
+    <policy name="ConfigureEnabledUtilityFileExplorerPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerPreview)" explainText="$(string.ConfigureEnabledUtilityDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerPreview">


We need a new description with the addition that disabling also disabled all single previewer. ANd with the info that force enabling it not force enables the toggles of the single previewers.

Done (line 49).

htcfreek · 2024-06-26T18:56:24Z

@acekirkpatrick
Did you reorderd the admx/adml? If yes please undo this. It is hard to review now because of the changes that are unrelated to file explorer policies.

And for your information: We have a policy documentation in our end-user docs that needs an update too.

htcfreek · 2024-06-26T19:14:41Z

Ooh wow. You fixed #29827 too.

acekirkpatrick · 2024-06-26T19:23:29Z

@htcfreek
When do I submit a PR to update the docs?

htcfreek · 2024-06-27T05:06:14Z

When do I submit a PR to update the docs?

@acekirkpatrick
At any time. I personally do that immediately at the end of the feature PR and mostly add the following notes to the PR description:

Please don't merge before the PowerToys release xyz. (In your case: Version 0.83.0 .)
Please don't merge before it is reviewed by the PowerToys project core maintainers.

htcfreek

We should rename the Policy group. (See my comment.)
Tray flyout and Settings dashboard using the wrong name. The should show "File Explorer add-ons" and not "File Explorer". (See your screenshots.)
If you disable the policy "Configure global utility enabled state" and enable "File Explorer add-ons: Configure enabled state" then you ran into a conflict. (The reason is that the "Configure global utility enabled state" policy affect the individual previewer policies.) - We either have to improve this or explain it in the policy description.
Regarding hiding the info bars if overall toggle is disabled by user or policy you can add a new property to the view model that is directly linked to the non-gpo info bars and add && _isEnabled as condition to the properties SomePreviewPaneEnabledGposConfigured and SomeThumbnailEnabledGposConfigured. (Important you have to call OnPropertyChanged() for all three infobar properties in the setter of IsEnabled.)
I didn't try the behavior of the previewers itself for the different policies. But looking at the code it seems that you did not update the code:

PowerToys/src/modules/previewpane/powerpreview/powerpreview.cpp

Line 224 in 8b8c75b

const auto gpo_rule = fileExplorerModule.checkModuleGPOEnabledRuleFunction();

htcfreek · 2024-06-27T14:39:17Z

src/gpo/assets/PowerToys.admx

-    <policy name="ConfigureEnabledUtilityFileExplorerMarkdownPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerMarkdownPreview)" explainText="$(string.ConfigureEnabledUtilityDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerMarkdownPreview">
-      <parentCategory ref="PowerToys" />
+    <policy name="ConfigureEnabledUtilityFileExplorerMarkdownPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerMarkdownPreview)" explainText="$(string.ConfigureEnabledFilePreviewerDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerMarkdownPreview">
+      <parentCategory ref="FilePreviewers" />


@acekirkpatrick
Should be File Explorer add-ons.

@jaimecbernardo
Can move the policies into the sub category, even if it breake existing Intune policy configuration sets? (Normal Group Policy configuration won't break.)

Not sure. Think we might need to add a note about that in release notes. we also broke "All utitilities" GPO backcompatibility in Intune. Not sure why Intune needs to be different 🤷

Not sure. Think we might need to add a note about that in release notes. we also broke "All utitilities" GPO backcompatibility in Intune. Not sure why Intune needs to be different 🤷

Side note: And All Experiments GPO is broken too after mofing into sub category. (PR 33529)

acekirkpatrick · 2024-06-27T22:02:26Z

src/modules/previewpane/powerpreview/powerpreview.cpp

@@ -218,56 +230,59 @@ void PowerPreviewModule::apply_settings(const PowerToysSettings::PowerToyValues&
 {
    bool notifyShell = false;

-    for (auto& fileExplorerModule : m_fileExplorerModules)
+    if (m_enabled)


@htcfreek
For point 5, I added a check that should prevent any preview handlers from running if the module is not enabled.

Sure. But the policy state won't be written into the settings file. You have to explicit query the policy values and evaluate them.

Like here:

PowerToys/src/modules/previewpane/powerpreview/powerpreview.cpp

Line 31 in 8b8c75b

.checkModuleGPOEnabledRuleFunction = powertoys_gpo::getConfiguredSvgPreviewEnabledValue,

Because you shouldn't be able to bypass a force-enabled add-on by turning off all extensions?

But what about force disabled?

What do you think of powerpreview.cpp now?

yes, it does look good to me now.

acekirkpatrick · 2024-07-03T16:06:30Z

@htcfreek
Do you think disable() should call apply_settings() or disable the previewers itself?

htcfreek · 2024-07-03T16:12:56Z

@htcfreek
Do you think disable() should call apply_settings() or disable the previewers itself?

I have no preference on this as long as the result is correct.

acekirkpatrick · 2024-07-03T16:14:52Z

@htcfreek
If so, I think we might be done. I'm going to build the installer and make sure everything works as expected. I'll add some updated screenshots, then we will hopefully be good to merge!

htcfreek · 2024-07-03T16:44:56Z

@htcfreek
If so, I think we might be done. I'm going to build the installer and make sure everything works as expected. I'll add some updated screenshots, then we will hopefully be good to merge!

Don't wonder if I add the "Review" label then. I don't have the permission for the final review and merging.

Please create a PR to update the end user docs.

acekirkpatrick · 2024-07-03T19:59:45Z

@htcfreek
I tested it and behavior was as expected. Putting new screenshots in the PR description now. It should be ready for final review!

acekirkpatrick · 2024-07-03T20:00:04Z

@htcfreek
Thank you for helping me with this!

htcfreek

LGTM

htcfreek · 2024-07-03T20:28:08Z

Note for reviewers (from the core team):

I think this is good to merge. But a test from another person would be good.
Moving the Policies for the individual previewers into a sub category breaks existing Intune Policy set as soon as the changed ADMX files will be imported to intune. (This is because the OMA Uri changes.) Is this something we can be okay with?
Do we need a special solution for Code Files Preview (Monaco) as the previewer code and settings are used in Peek too?

cc: @jaimecbernardo , @stefansjfw

acekirkpatrick · 2024-07-04T19:59:28Z

@htcfreek
What is the Intune OMA-Uri for the File Explorer add-ons? (I'm updating the docs)

htcfreek · 2024-07-04T22:28:13Z

@htcfreek
What is the Intune OMA-Uri for the File Explorer add-ons? (I'm updating the docs)

@acekirkpatrick
See this comment for the uri syntax: https://github.com/MicrosoftDocs/windows-dev-docs/blob/2a5fbf881b664cabba17002a2f4910af2edc5a19/hub/powertoys/grouppolicy.md?plain=1#L42

jaimecbernardo · 2024-07-22T15:46:08Z

I've merged with main to resolve conflicts.

jaimecbernardo · 2024-07-22T16:02:10Z

src/common/utils/gpo.h

@@ -286,47 +287,47 @@ namespace powertoys_gpo {

    inline gpo_rule_configured_t getConfiguredSvgPreviewEnabledValue()
    {
-        return getUtilityEnabledValue(POLICY_CONFIGURE_ENABLED_SVG_PREVIEW);
+        return getConfiguredValue(POLICY_CONFIGURE_ENABLED_SVG_PREVIEW);


Why are we making these utilities no longer respond to the "All utilities" GPO?

It was based on how it is working now. We discussed this in the PR discussion. They are handled as sub settings now.

And the new "main" utility GPO is the one for "File Explorer add-ons module". And this new policy is respecting the "all utilities gpo".

If the previewers itself tespect the all utilities gpo they can override the explorer add-ons module gpo. And this can lead to confusing behavior.

@jaimecbernardo (and @acekirkpatrick)
For clarification. The problem is not that individual previewers will override the "All Utilities GPO". The problem is that with "All Utilities GPO" being disabled and "FileExplorer Addons Module GPO" being enabled, the resulting behavior is strange.

Example:

All Utillities GPO = disabled

File Explorer Add-ons Module enabled state = enabled

Individual File Explorer add-ons enabled state = All are not configured.

Result: The module is enabled and no individual previewer works as they are still disabled by the "All Utilities GPO".

Possible solution 1 for the problem:

If we need a policy to disable new/unknown previewers like the old behavior was, I suggest the following: Add a new policy like we have for unmanaged PT Run plugins. We can name it "Default enabled state for all File Explorer Add-ons" and place it in the "File Explorer" category. (Then we are as flexible as possible.)
The behavior would be:

Not configured: User takes control.

Enabled: Add-on is enabled.

Disabled: Add-on is disabled.

Overwritten by individual add-on policy: Value of individual policy.

(This solution fixes the conflict we have when using the "All Utilities GPO" while still providing the possibility to handle newly added Add-ons by default.)

Possible solution 2 for the problem:

We revert the gpo related changes to the old behavior. (Maybe except moving the policies into a sub category.)

And then we add code to calculate the global FE Add-on enabled toggle state based on the individual policies:

At least one add-on is force enabled: Global toggle is force enabled.

All add-ons are force disabled: Global toggle is force disabled.

(Thi solves the problems we get with a new utility policy while not breaking current policy behavior and keeping the policy configuration flexible and simple.)

7/22/2024 Edit 1: Add not about overwrite behavior to solution 1.
7/22/2024 Edit 2: Add solution 2.

@jaimecbernardo and @acekirkpatrick
What do you think about this? I personally prefer to go with solution 2 as it keeps the administration simple and doesn't break existing policies.

@htcfreek @jaimecbernardo
I would agree that solution 2 is the most similar to how it was before, if we are still considering each previewer to be its own module.
However, they don't appear to be separate once we add the overall toggle. It feels like it would be best to make it like PT Run, where the individual previewers are considered to be part of the same module.
My solution 3:
Make the File Explorer module very similar to PT Run, where the individual previewer policies override the module policy. So you can disable the module policy, which disables any new previewers, but you can force-enable certain previewers so that they still run.
The main problem with this would be that unlike PT Run, the user would have no option to simply stop using the previewers. They would have to stop using PowerToys completely if they didn't want to use a force-enabled previewer.

src/gpo/assets/en-US/PowerToys.adml

Co-authored-by: Heiko <[email protected]>

htcfreek · 2024-08-02T23:03:34Z

ping @jaimecbernardo. And can you please look at our suggestions on this discussion.

htcfreek · 2024-08-22T05:55:20Z

@jaimecbernardo or @stefansjfw
Can one of you look soon at the discussion and my suggestions about the "All utilities gpo" behavior here.

I have a conflicting PR #33703 open and need to know how long this Explorer add-ons PR needs to be ready. I like to get my PR in soon which is impossible if I wait on this here. I suggest to get my PR in and update this here with the new info bar icons.

acekirkpatrick added 15 commits June 19, 2024 10:10

Added stuff for an enabled toggle

7e4e12b

Removed incorrect summary

79919c9

Added most of a GPO

2b513b6

Added to BugReportTool

9dabee3

Updated .adml and .admx

d6bd83b

Added a card to the Dashboard

b65f633

Fixed adml

a40d5c5

Reordered things so File Explorer appears in the right place

cd3172c

Finished copying from other modules

40707c4

Made sure it was using its own GPO

75e8ed9

Allowed it to save the setting

43074da

Disabled settings when module is not enabled

877d12c

Reformatted .adml and .admx

e5798ee

Added a check to stop it from starting the handlers if the module is …

9e85b63

…disabled

Made sure enable() called apply_settings so that the modules start

b35df52

acekirkpatrick changed the title ~~23723~~ [File Explorer] Added an overall toggle switch (#23723) Jun 21, 2024

acekirkpatrick marked this pull request as ready for review June 25, 2024 22:12

htcfreek suggested changes Jun 26, 2024

View reviewed changes

Updated .adml and .admx

f197ee3

Reordered .adml and .admx

4f15ce6

htcfreek suggested changes Jun 27, 2024

View reviewed changes

acekirkpatrick commented Jun 27, 2024

View reviewed changes

acekirkpatrick requested a review from htcfreek July 3, 2024 20:05

Updated .adml

779214e

htcfreek approved these changes Jul 3, 2024

View reviewed changes

htcfreek added the Needs-Review This Pull Request awaits the review of a maintainer. label Jul 3, 2024

acekirkpatrick mentioned this pull request Jul 4, 2024

Updated group policy docs and picture of File Explorer page MicrosoftDocs/windows-dev-docs#5051

Merged

htcfreek mentioned this pull request Jul 7, 2024

[Settings] Update GPO infobar icon, improve some pages and fix bugs #33703

Open

11 tasks

Merge branch 'main' into 23723

115d3e8

jaimecbernardo reviewed Jul 22, 2024

View reviewed changes

htcfreek suggested changes Jul 22, 2024

View reviewed changes

src/gpo/assets/en-US/PowerToys.adml Outdated Show resolved Hide resolved

src/gpo/assets/en-US/PowerToys.adml Outdated Show resolved Hide resolved

acekirkpatrick and others added 2 commits July 22, 2024 14:21

Update .adml for clarification

9c83638

Co-authored-by: Heiko <[email protected]>

Update .adml for clarification

b31bcab

Co-authored-by: Heiko <[email protected]>

Updated version information in the .admx and .adml

7f6ab6f

acekirkpatrick requested review from jaimecbernardo and htcfreek August 8, 2024 20:44

acekirkpatrick mentioned this pull request Aug 9, 2024

Updated group policy docs and picture of File Explorer page MicrosoftDocs/windows-dev-docs#5103

Draft

acekirkpatrick added 3 commits September 4, 2024 15:30

Updated .adml and .admx to version 0.85.0

893e5de

Change to revision comment

34381ad

Merged main into 23723

dc1746a

		[JsonPropertyName("File Explorer Preview")]
		[JsonPropertyName("File Explorer")]

		<policy name="ConfigureEnabledUtilityFileExplorerSVGPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerSVGPreview)" explainText="$(string.ConfigureEnabledUtilityDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerSVGPreview">
		<parentCategory ref="PowerToys" />

[File Explorer] Added an overall toggle switch (#23723) #33475

Are you sure you want to change the base?

[File Explorer] Added an overall toggle switch (#23723) #33475

Conversation

acekirkpatrick commented Jun 21, 2024 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Setting saved to JSON

Previews were not run if disabled

GPO

Screenshots

Module Policy Not Configured

Module Policy Disabled

Module Policy Enabled

htcfreek commented Jun 24, 2024

acekirkpatrick commented Jun 25, 2024

htcfreek commented Jun 26, 2024

acekirkpatrick commented Jun 26, 2024

htcfreek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acekirkpatrick Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

htcfreek Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htcfreek commented Jun 26, 2024

htcfreek commented Jun 26, 2024 • edited Loading

acekirkpatrick commented Jun 26, 2024 • edited Loading

htcfreek commented Jun 27, 2024 • edited Loading

htcfreek left a comment • edited Loading

Choose a reason for hiding this comment

htcfreek Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htcfreek Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htcfreek Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acekirkpatrick commented Jul 3, 2024

htcfreek commented Jul 3, 2024

acekirkpatrick commented Jul 3, 2024

htcfreek commented Jul 3, 2024

acekirkpatrick commented Jul 3, 2024

acekirkpatrick commented Jul 3, 2024

htcfreek left a comment

Choose a reason for hiding this comment

htcfreek commented Jul 3, 2024

acekirkpatrick commented Jul 4, 2024

htcfreek commented Jul 4, 2024 • edited Loading

jaimecbernardo commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htcfreek Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Example:

Possible solution 1 for the problem:

Possible solution 2 for the problem:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acekirkpatrick commented Jun 21, 2024 •

edited

Loading

acekirkpatrick Jun 26, 2024 •

edited

Loading

htcfreek Jun 27, 2024 •

edited

Loading

htcfreek commented Jun 26, 2024 •

edited

Loading

acekirkpatrick commented Jun 26, 2024 •

edited

Loading

htcfreek commented Jun 27, 2024 •

edited

Loading

htcfreek left a comment •

edited

Loading

htcfreek Jun 27, 2024 •

edited

Loading

htcfreek Jul 22, 2024 •

edited

Loading

htcfreek Jun 27, 2024 •

edited

Loading

htcfreek commented Jul 4, 2024 •

edited

Loading

htcfreek Jul 22, 2024 •

edited

Loading