-
Notifications
You must be signed in to change notification settings - Fork 16
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
Allow user to speed up sites on Windows programmatically #211
Conversation
13fd65c
to
d0ed46c
Compare
d0ed46c
to
2306c3f
Compare
@@ -11,6 +11,7 @@ export interface UserData { | |||
[ stat: string ]: number; | |||
}; | |||
}; | |||
promptWindowsSpeedUpResult?: PromptWindowsSpeedUpResult; |
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.
The result is persisted to show only once the prompt in the onboarding.
const exeDir = path.dirname( app.getPath( 'exe' ) ); | ||
exePath = path.join( exeDir, '..', 'app-*', exeFilename ); | ||
} | ||
const command = `PowerShell -NoProfile -ExecutionPolicy Bypass -Command "Add-MpPreference -ExclusionProcess ${ exePath }"`; |
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.
For reference, here's the Windows documentation for this command.
let exePath = app.getPath( 'exe' ); | ||
// When the app is packaged, the exe path points to "%appdata%\Local\studio\app-{app-version}\Studio.exe". | ||
// To avoid updating this configuration on each update, we use a wilcard in the path to include all versions. | ||
if ( app.isPackaged ) { |
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.
The app is not packaged when it's executed from the Electron shell located in node_modules
. This value is also used to distinguish development and production environments.
src/lib/windows-helpers.ts
Outdated
buttons, | ||
title: __( 'Want to speed up sites?' ), | ||
message: __( | ||
"If the Real-Time Protection Service of Windows Defender is enabled on your machine, it may slow down the process of creating and starting a site.\n\nTo enhance site speed, it's recommended adjusting the configuration accordingly.\n\nThe app can do this automatically for you, or alternatively, you can follow the documentation." |
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.
I'd appreciate any feedback on the messaging. The content is mostly based on the Documentation.
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.
Could we maybe add the Needs Copy Review
label or ping @\Automattic/editorial
to get some extra help from the Publications team? I don't feel overly confident in my copy abilities, but feel the adjusting the configuration accordingly
line is perhaps a bit too vague in this context.
Maybe it could be updated to something like the following?
For optimal performance, we recommend disabling this service for the Studio app.
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.
Could we maybe add the
Needs Copy Review
label or ping@\Automattic/editorial
to get some extra help from the Publications team? I don't feel overly confident in my copy abilities, but feel theadjusting the configuration accordingly
line is perhaps a bit too vague in this context.
Good idea, I've just added the label.
Maybe it could be updated to something like the following?
For optimal performance, we recommend disabling this service for the Studio app.
The idea is to exclude the app from this service, not disabling as Windows Defender is meant to protect user's machine from security threats.
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.
Oh, that's what I was trying to convey with the for the Studio app
part of the suggestion, but see it wasn't clear. 😅 How about something like the following?
For optimal performance, we recommend excluding the Studio app from this service.
I'm sure the Publications team will have some better suggestions than myself, though!
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.
Oh, that's what I was trying to convey with the
for the Studio app
part of the suggestion, but see it wasn't clear. 😅
Ah, true. I've just re-read it and I found it clearer now 😅 .
[...] How about something like the following?
For optimal performance, we recommend excluding the Studio app from this service.
Sounds good, I'll update the message.
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.
I applied this suggestion in df02d63.
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.
Thank you for working on this @fluiddot! The happy path works well for me. Before allowing the change via the dialog, it took around 23 seconds to create a new site:
slow.mov
After, it took about 12 seconds:
quicker.mov
I also confirmed that the prompt was only displayed the first time and displays in the Help section. 🙌 I'll add some more thoughts on the copy separately.
src/lib/windows-helpers.ts
Outdated
const MANUAL_UPDATE = __( "I'll do it my own by following the documentation." ); | ||
const AUTOMATIC_UPDATE = __( 'Sounds good, do it for me.' ); | ||
|
||
const buttons = [ NOT_INTERESTED, MANUAL_UPDATE, AUTOMATIC_UPDATE ]; |
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.
When looking at the dialog, I think it would feel more intuitive for me to see the Sounds good, do it for me
option at the top of the list, not the bottom.
I also wondered if it'd make sense for the I'm not interested
option to follow the same Don't ask again
pattern we use for the demo update and arm64 optimisation dialogs:
![Screenshot 2024-06-10 at 12 38 42](https://private-user-images.githubusercontent.com/2998162/338168695-f624ae8e-c794-4ab8-ab65-371cd36d7fdb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjIyODU1NTUsIm5iZiI6MTcyMjI4NTI1NSwicGF0aCI6Ii8yOTk4MTYyLzMzODE2ODY5NS1mNjI0YWU4ZS1jNzk0LTRhYjgtYWI2NS0zNzFjZDM2ZDdmZGIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcyOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MjlUMjAzNDE1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YjQ4ODE4ZGM4ZmVjOTFjNTMzZDY2YTNmN2FiNzcxZmJjZDAxMTcxZWE5NjdlNDE5NWI2ZWRkZjgwZjZjYTJiYiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.4kHlWa14JpqMja3wjlUciG9yUZ2IlkRXC2WdtCfaKFE)
![Screenshot 2024-06-10 at 11 41 12](https://private-user-images.githubusercontent.com/2998162/338168888-36db16e2-5998-4ffd-b5e7-da5be2ecd40b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjIyODU1NTUsIm5iZiI6MTcyMjI4NTI1NSwicGF0aCI6Ii8yOTk4MTYyLzMzODE2ODg4OC0zNmRiMTZlMi01OTk4LTRmZmQtYjVlNy1kYTViZTJlY2Q0MGIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcyOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MjlUMjAzNDE1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NzQ3YzNiZWQ5ZjllNjY2MDgxNDYyYmZkNDcwMDYzZjgzMDZkOWRlYmY3NWZmNzY3NDQ0MTNlZmJhNGI2NmUwOSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.Ij4JQZ65xB6VYkTJmNsUbyNWxQkvYbbIXYqvNVvr_8I)
Wdyt?
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.
@matt-west, I'd be interested to hear your perspective on this, too. The three options feel a little cluttered to me:
![Screenshot 2024-06-10 at 12 46 40](https://private-user-images.githubusercontent.com/2998162/338170157-189418e5-0b6a-48b3-b409-86387b9efc9b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjIyODU1NTUsIm5iZiI6MTcyMjI4NTI1NSwicGF0aCI6Ii8yOTk4MTYyLzMzODE3MDE1Ny0xODk0MThlNS0wYjZhLTQ4YjMtYjQwOS04NjM4N2I5ZWZjOWIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcyOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MjlUMjAzNDE1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZmFlMzhmOTNjNGExYmIxYjMzZGNhN2Y1NmU2ZGU0NzZhODQxMzhhM2JlM2QxYTE0MjczZmU3MWJjZDM3ZjcwNCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.slsyXUp-Q4_KXo3evMWgphrEsWIHvVruYPskKpm6Xbk)
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.
When looking at the dialog, I think it would feel more intuitive for me to see the
Sounds good, do it for me
option at the top of the list, not the bottom.
I originally put the buttons in that order but changed them at the last moment because the default response when canceling the dialog is associated with the first element. I thought this order might be following design guidelines, but I've just checked the documentation and it's recommended to have the "Yes" action in the first position:
The "do it" action button(s) should appear as the leftmost buttons. The safe, nondestructive action should appear as the rightmost button.
I'll apply this change.
I also wondered if it'd make sense for the
I'm not interested
option to follow the sameDon't ask again
pattern we use for the demo update and arm64 optimisation dialogs:
This dialog is only prompted automatically once, so I think we wouldn't need to provide a turn-off option. WDYT?
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.
This dialog is only prompted automatically once, so I think we wouldn't need to provide a turn-off option. WDYT?
Good point, the main motivation behind the suggestion was making I'm not interested
less prominent, which I think moving to the last position in the button list would accomplish. 👍
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.
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.
Do we need to include the option for manually applying the change? I think we could simplify this dialog by removing that option.
My rationale behind adding the option was to provide a middle solution where the user could check the documentation. However, if we foresee that the option won't be helpful to the user and reduces the clarity of the dialog, I'll be happy to remove it. WDYT @matt-west ?
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.
Okay. Let's be opinionated here and remove that option. Well need to adjust the body copy to reflect this change too.
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.
Well need to adjust the body copy to reflect this change too.
Originally, I thought of adding a link to the documentation in the body but the native dialog just renders plain text 😞. Do you think we should reference the documentation or simply offering the option to run the action automatically?
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.
I don't think we need to reference the docs here. If we find that users are confused about what Windows Defender is and how it works, we can add some additional copy to the dialog.
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.
I applied this suggestion in f260f97.
message: __( | ||
'If the Real-Time Protection Service of Windows Defender is enabled on your machine, it may slow down the process of creating and starting a site.\n\nFor optimal performance, we recommend excluding the Studio app from this service.\n\nThe app can do this automatically for you, or alternatively, you can follow the documentation.' | ||
), | ||
cancelId: buttons.indexOf( NOT_INTERESTED ), |
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.
When closing the dialog, it returns by default the ID 0
, we have to set the specific button associated with canceling the process.
Hi @fluiddot, I read through the PR and I have a few questions about the modal. Here's the text on the modal:
Questions:
Your answers will help me to come up with an alternative wording. Thanks in advance! |
Thank you @kristastevens for helping us with the wording 🙇 !
Real-time protection slows down the execution of PHP files for sites served locally. This applies to all sites created on the Studio app as they are local. So excluding the app in this service speeds up sites in general. However, the most notorious items are creating and starting a site.
I checked different documentation and found references to multiple names: Windows Security, Microsoft Defender, and Windows Defender. On Windows 11, if I navigate to the screen to add exclusions the description says:
I understand that the service is Microsoft Defender and that's part of the OS feature Windows Security. Hence, we could use Microsoft Defender.
Yes, the app will automatically add an exclusion to the Real-time protection to disable this service only for the Studio app. I think it's important to convey that we only disable it for the app, so Microsoft Defender will keep protecting the user's machine. UPDATE: After f3a441a, the message has been changed to:
|
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.
Going ahead to approve, pending any copy or UI suggestions from Krista or Matt, so as not to block this PR with upcoming AFK. The main flows, including automatically speeding up the site and dismissing the dialog, work great for me. Thanks @fluiddot! 🙌
Hi @fluiddot, thanks so much for the clarification. Here's an alternative wording:
|
Great, thanks @kristastevens for your help on the wording. I applied this in af63a6e.
I'd like to note that this PR is related to the Studio app, not WordPress. |
Note
This PR is a proposal to solve 7013-gh-Automattic/dotcom-forge. I'd appreciate any feedback on the proposal and the messaging. Thanks 🙇 !
Proposed Changes
sudo-prompt
package to prompt the user for admin permission actions.Testing Instructions
Windows
No Optimizations
%appdata%\Studio
.Speed Up Sites Optimization
%appdata%\Studio
.Manual OptimizationRemove app data by deleting the folder located at%appdata%\Studio
.Remove all created sites.Open the app.Observe that the onboarding screen is shown.Click on the Add Site button.Observe a dialog is displayed with three options.Click on the second option to display documentation.Observe the browser is opened and show documentation about how to speed up sites on Windows.Observe the site is created in the meantime.Prompt Only Displayed First Time
%appdata%\Studio
.Help Section
macOS
No Dialog Displayed
$HOME/Library/Application Support/Studio
.No New Help Option
Pre-merge Checklist