Skip to content

Conversation

acekirkpatrick
Copy link
Contributor

@acekirkpatrick acekirkpatrick commented Jun 21, 2024

Summary of the Pull Request

Added an enable toggle switch for File Explorer.

PR Checklist

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

Disabled Page
Enabled Page
Enabled Dashboard
Enabled All Apps

Module Policy Disabled

Force Disabled Page
Force Disabled Dashboard
Force Disabled All Apps

Module Policy Enabled

Force Enabled Page
Force Enabled Dashboard
Force Enabled All Apps

@acekirkpatrick acekirkpatrick changed the title 23723 [File Explorer] Added an overall toggle switch (#23723) Jun 21, 2024
@htcfreek
Copy link
Collaborator

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

@acekirkpatrick acekirkpatrick marked this pull request as ready for review June 25, 2024 22:12
@acekirkpatrick
Copy link
Contributor Author

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

@htcfreek
Copy link
Collaborator

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

@acekirkpatrick
Copy link
Contributor Author

@htcfreek
I put some screenshots in the description.

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

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

private bool fileExplorerPreview = true;

[JsonPropertyName("File Explorer Preview")]
[JsonPropertyName("File Explorer")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this change breake existing settings that users made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<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)"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will be 0.83.0 . Please update all occurrences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Severity="Informational" />

<controls:SettingsGroup x:Uid="FileExplorerPreview_PreviewPane" IsEnabled="{x:Bind ViewModel.IsEnabled, Mode=OneWay}">
<InfoBar
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@acekirkpatrick acekirkpatrick Jun 26, 2024

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@htcfreek htcfreek Jun 27, 2024

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used IsEnabled. Is that okay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines 150 to 151
<policy name="ConfigureEnabledUtilityFileExplorerSVGPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerSVGPreview)" explainText="$(string.ConfigureEnabledUtilityDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerSVGPreview">
<parentCategory ref="PowerToys" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<decimal value="0" />
</disabledValue>
</policy>
<policy name="ConfigureEnabledUtilityFileExplorerPreview" class="Both" displayName="$(string.ConfigureEnabledUtilityFileExplorerPreview)" explainText="$(string.ConfigureEnabledUtilityDescription)" key="Software\Policies\PowerToys" valueName="ConfigureEnabledUtilityFileExplorerPreview">
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (line 49).

@htcfreek
Copy link
Collaborator

@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
Copy link
Collaborator

htcfreek commented Jun 26, 2024

Ooh wow. You fixed #29827 too.

@acekirkpatrick
Copy link
Contributor Author

acekirkpatrick commented Jun 26, 2024

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

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 27, 2024

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.

Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

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

  1. We should rename the Policy group. (See my comment.)
  2. Tray flyout and Settings dashboard using the wrong name. The should show "File Explorer add-ons" and not "File Explorer". (See your screenshots.)
  3. 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.
    image
  4. 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.)
  5. 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:
    const auto gpo_rule = fileExplorerModule.checkModuleGPOEnabledRuleFunction();

<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" />
Copy link
Collaborator

@htcfreek htcfreek Jun 27, 2024

Choose a reason for hiding this comment

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

@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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤷

Copy link
Collaborator

@htcfreek htcfreek Jul 22, 2024

Choose a reason for hiding this comment

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

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)

bool notifyShell = false;

for (auto& fileExplorerModule : m_fileExplorerModules)
if (m_enabled)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@htcfreek htcfreek Jun 27, 2024

Choose a reason for hiding this comment

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

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:

.checkModuleGPOEnabledRuleFunction = powertoys_gpo::getConfiguredSvgPreviewEnabledValue,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what about force disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of powerpreview.cpp now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, it does look good to me now.

@htcfreek
Copy link
Collaborator

htcfreek commented Aug 2, 2024

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

@htcfreek
Copy link
Collaborator

@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.

@htcfreek
Copy link
Collaborator

@acekirkpatrick
This will have merge conflicts for gpo.h. Attention the file has been restructured.

@htcfreek
Copy link
Collaborator

@crutkas
Weeks ago @jaimecbernardo asked a question in its review. We answered and made some suggestions. But we are still waiting for ans answer and final decision of the core team.

@htcfreek htcfreek added the Needs-Team-Response An issue author responded so the team needs to follow up label Oct 15, 2024
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (11)
  • src/common/GPOWrapper/GPOWrapper.cpp: Language not supported
  • src/common/GPOWrapper/GPOWrapper.h: Language not supported
  • src/common/GPOWrapper/GPOWrapper.idl: Language not supported
  • src/common/utils/gpo.h: Language not supported
  • src/gpo/assets/PowerToys.admx: Language not supported
  • src/gpo/assets/en-US/PowerToys.adml: Language not supported
  • src/modules/previewpane/powerpreview/powerpreview.cpp: Language not supported
  • src/settings-ui/Settings.UI/SettingsXAML/Views/PowerPreviewPage.xaml: Language not supported
  • src/settings-ui/Settings.UI/Strings/en-us/Resources.resw: Language not supported
  • tools/BugReportTool/BugReportTool/ReportGPOValues.cpp: Language not supported
  • src/common/ManagedCommon/ModuleType.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

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

  • Changing the JsonPropertyName from 'File Explorer Preview' to 'File Explorer' might cause inconsistencies or bugs if other parts of the system are not updated to reflect this change.
[JsonPropertyName("File Explorer")]

@yeelam-gordon yeelam-gordon added Product-Settings The standalone PowerToys Settings application Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Apr 8, 2025
@yeelam-gordon yeelam-gordon requested a review from lei9444 April 10, 2025 00:12
@yeelam-gordon
Copy link
Contributor

yeelam-gordon commented Apr 10, 2025

@shuaiyuanxx will take care this PR.

I see the major discussion is on GPO handling, which we should not break any existing GPO experience as the voting.
i.e. the solution 2 here: #33475 (comment)

@yeelam-gordon yeelam-gordon removed the Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams label Apr 10, 2025
@yeelam-gordon yeelam-gordon added this to the PowerToys 0.92 milestone Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer. Needs-Team-Response An issue author responded so the team needs to follow up Product-Settings The standalone PowerToys Settings application
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants