-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[File Explorer] Added an overall toggle switch (#23723) #33475
Open
acekirkpatrick
wants to merge
33
commits into
microsoft:main
Choose a base branch
from
acekirkpatrick:23723
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
7e4e12b
Added stuff for an enabled toggle
acekirkpatrick 79919c9
Removed incorrect summary
acekirkpatrick 2b513b6
Added most of a GPO
acekirkpatrick 9dabee3
Added to BugReportTool
acekirkpatrick d6bd83b
Updated .adml and .admx
acekirkpatrick b65f633
Added a card to the Dashboard
acekirkpatrick a40d5c5
Fixed adml
acekirkpatrick cd3172c
Reordered things so File Explorer appears in the right place
acekirkpatrick 40707c4
Finished copying from other modules
acekirkpatrick 75e8ed9
Made sure it was using its own GPO
acekirkpatrick 43074da
Allowed it to save the setting
acekirkpatrick 877d12c
Disabled settings when module is not enabled
acekirkpatrick e5798ee
Reformatted .adml and .admx
acekirkpatrick 9e85b63
Added a check to stop it from starting the handlers if the module is …
acekirkpatrick b35df52
Made sure enable() called apply_settings so that the modules start
acekirkpatrick f197ee3
Updated .adml and .admx
acekirkpatrick 4f15ce6
Reordered .adml and .admx
acekirkpatrick 7d1f213
Renamed File Previewer to File Explorer add-on
acekirkpatrick b905634
Improved C++ code
acekirkpatrick e3e9583
Hid the info bars when the module is disabled
acekirkpatrick 1470338
Changed the dashboard text
acekirkpatrick 4c42c90
Updated for consistent behavior
acekirkpatrick c9174f9
Stopped the global utility policy from affecting individual previewers
acekirkpatrick 7a4052d
Turned off previewers if module is disabled
acekirkpatrick 779214e
Updated .adml
acekirkpatrick 115d3e8
Merge branch 'main' into 23723
jaimecbernardo 9c83638
Update .adml for clarification
acekirkpatrick b31bcab
Update .adml for clarification
acekirkpatrick 7f6ab6f
Updated version information in the .admx and .adml
acekirkpatrick 893e5de
Updated .adml and .admx to version 0.85.0
acekirkpatrick 34381ad
Change to revision comment
acekirkpatrick dc1746a
Merged main into 23723
acekirkpatrick 304ca76
Merged main into 23723
acekirkpatrick File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we making these utilities no longer respond to the "All utilities" GPO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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:
(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:
(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.