-
Notifications
You must be signed in to change notification settings - Fork 451
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
Correct Upgrade Messages for Mac #1452
Conversation
|
||
public override string StartServiceCommand | ||
{ | ||
get { return "`launchctl start org.vfsforgit.service`"; } |
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.
@alameenshah / @jamill can you confirm this is the correct command. I grabbed this from the start scripts.
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.
Spoke with @alameenshah on this and instead we should have "launchctl load /Library/LaunchAgents/org.vfsforgit.service.plist' . We also tested it out and it worked as expected. Updating now.
|
||
public override string UpgradeConfirmCommand | ||
{ | ||
get { return "`gvfs upgrade --confirm`"; } |
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.
If you moved gvfs upgrade --confirm
into a string constant in WindowsPlatform.Shared.cs
you could use it in GetUpgradeReminderNotificationImplementation
as well:
public static string GetUpgradeReminderNotificationImplementation()
{
return "A new version of GVFS is available. Run `gvfs upgrade --confirm` from an elevated command prompt to upgrade.";
}
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.
@jeschu1 I unresolved this conversation. It looks like this code can be updated to return UpgradeConfirmMessage
rather than duplicating "'gvfs upgrade --confirm'"
57c04a7
to
48338c6
Compare
@wilbaker thanks for the feedback, it should be addressed now. |
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.
Looks good to me, but please get an OK from @alameenshah or @jamill on the Mac StartServiceCommandMessage
before merging
Did we consider if we could simplify / streamline any of the messages? To keep the exact structure as we had previously, we now have to build the messages from multiple fragments, some of which start with a space, some don't, etc.. This makes it harder to determine the exact message the user is going to see and ensure it is correct. |
48338c6
to
f8ed2de
Compare
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 know we came in expecting a simple change, but I am worried that we are adding a bit of complexity here to keep the existing structure... I included some suggestions - I am curious about your thoughts on if we can simplify this.
@@ -202,7 +202,9 @@ private bool IsGVFSUpgradeAllowed(out string consoleError) | |||
|
|||
if (this.IsServiceInstalledAndNotRunning()) | |||
{ | |||
adviceText = isConfirmed ? $"Run `sc start GVFS.Service` and run {this.CommandToRerun} again from an elevated command prompt." : $"To install, run `sc start GVFS.Service` and run {GVFSConstants.UpgradeVerbMessages.GVFSUpgradeConfirm} from an elevated command prompt."; | |||
adviceText = isConfirmed |
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.
Instead of building the sentence from multiple fragments, can we simplify this by just instructing the user to start the service and run the command again? We have already checked they are running from an elevated command prompt.
To start the service, run {command} and run the command again
f8ed2de
to
03fbd2f
Compare
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.
Thanks!
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.
With the most recent changes I'm not sure that we still need ElevatedCommandPromptMessage
and UpgradeInstallAdviceMessage
in the platform.
get { return $"`sc start GVFS.Service`"; } | ||
} | ||
|
||
public override string ElevatedCommandPromptMessage |
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 still need ElevatedCommandPromptMessage
?
If UpgradeConfirmCommandMessage
were updated to return:
"'gvfs upgrade --confirm' from an elevated command prompt."
Similar to how it returns the following on Mac:
"'sudo gvfs upgrade --confirm'"
Then we could probably remove both ElevatedCommandPromptMessage
and UpgradeInstallAdviceMessage
from the platform.
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.
If we do still need ElevatedCommandPromptMessage
can you update the name? I think this property used to return something like "from an elevated command prompt" and now it's returning a different 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.
UpgradeConfirmCommandMessage
is also used in an error message for checking if the service is installed and not running, so we can't update that one.
The windows message does refer to an elevated prompt. I'm updating it to be RunUpdateMessage
which should be more clear.
Also as an FYI, potentially it's a bit of scope creep but @jamill and I went thru the messages and simplified what was there. This should make life easier in the future.
03fbd2f
to
890bba4
Compare
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.
Just minor nits. Take them or leave them.
consoleError = string.Join( | ||
Environment.NewLine, | ||
"The installer needs to be run from an elevated command prompt.", | ||
"The installer needs to be run elevated.", |
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.
Nit: I had trouble parsing this. Perhaps "The installer needs to be run with elevated permissions"?
@@ -121,6 +122,11 @@ public static string GetUpgradeHighestAvailableVersionDirectoryImplementation() | |||
return GetUpgradeProtectedDataDirectoryImplementation(); | |||
} | |||
|
|||
public static string GetUpgradeReminderNotificationImplementation() | |||
{ | |||
return $"A new version of GVFS is available. Run {UpgradeConfirmMessage} from an elevated command prompt to upgrade."; |
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.
GVFS -> VFS for Git?
|
||
public override string UpgradeInstallAdviceMessage | ||
{ | ||
get { return "When ready, run `" + this.UpgradeConfirmCommandMessage + "` from an elevated command prompt."; } |
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 not an interpolated string here? $"When ready run
{this.UpgradeConfirmCommandMessage}`..."
890bba4
to
f6895e3
Compare
@@ -202,7 +202,9 @@ private bool IsGVFSUpgradeAllowed(out string consoleError) | |||
|
|||
if (this.IsServiceInstalledAndNotRunning()) | |||
{ | |||
adviceText = isConfirmed ? $"Run `sc start GVFS.Service` and run {this.CommandToRerun} again from an elevated command prompt." : $"To install, run `sc start GVFS.Service` and run {GVFSConstants.UpgradeVerbMessages.GVFSUpgradeConfirm} from an elevated command prompt."; | |||
adviceText = isConfirmed | |||
? $"Run {GVFSPlatform.Instance.Constants.StartServiceCommandMessage} and run {this.CommandToRerun} again." |
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 two different messages here?
if isConfirmed
is true, then CommandToRerun
is the same as UpgradeConfirmCommandMessage
and so the difference between messages is minimal:
$"Run {GVFSPlatform.Instance.Constants.StartServiceCommandMessage} and run {UpgradeConfirmCommandMessage} again."
vs
$"To install, run {GVFSPlatform.Instance.Constants.StartServiceCommandMessage} and run {GVFSPlatform.Instance.Constants.UpgradeConfirmCommandMessage}."
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.
Approved with suggestions
6e28984
to
701d79a
Compare
701d79a
to
fed1f5f
Compare
I was hoping this was an easy change, but it ended up being a bit involved.
There were several places that required string changes, including the hooks.
resolves #1442