Enable mac upgrade MVP#1284
Merged
Merged
Conversation
jamill
force-pushed
the
enable_mac_upgrade
branch
from
f9b5c54 to
92c624a
Compare
derrickstolee
approved these changes
Jun 19, 2019
Contributor
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
I see that the only functional change is actually supporting upgrade and changing the messaging around upgrade launching separately or running inline. Feel free to ignore my style nit.
| get { return new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "GVFS.Mount", "git", "wish" }; } | ||
| } | ||
|
|
||
| public override bool SupportsInlineUpgrade { get; } = true; |
Contributor
There was a problem hiding this comment.
I'm really not a fan of this notation. Can we replace the { get; } = true with => true; and still have it work with the override and virtual members?
Member
Author
There was a problem hiding this comment.
I don't feel strongly - but I updated to use the Expression body definition.
alameenshah
reviewed
Jun 19, 2019
| UseShellExecute = true, | ||
| UseShellExecute = useShellExecute, | ||
| WorkingDirectory = Environment.SystemDirectory, | ||
| WindowStyle = ProcessWindowStyle.Normal, |
Contributor
There was a problem hiding this comment.
is this trying to change window style of the parent process's window, when running inline?
Member
Author
There was a problem hiding this comment.
No - It does not want to change the parent process's window style... WindowStyle's default value is Normal.
alameenshah
reviewed
Jun 19, 2019
| public MacPlatform() | ||
| public MacPlatform() : base( | ||
| underConstruction: new UnderConstructionFlags( | ||
| supportsGVFSUpgrade: true, |
alameenshah
reviewed
Jun 19, 2019
| } | ||
|
|
||
|
|
||
| public override bool SupportsInlineUpgrade { get; } = false; |
Contributor
There was a problem hiding this comment.
a few lines in comment would help document what an inline upgrade is.
Member
Author
There was a problem hiding this comment.
If you (or anyone) has a better term for this, please suggest it :)
Member
There was a problem hiding this comment.
Maybe SupportsUpgradeWhileRunning that's the best I can come up with. Not sure how accurate that is either?
Contributor
There was a problem hiding this comment.
I vote for SupportsInlineUpgrade. The context can be made clear with additional comments. Alternate name ideas -SupportsSingleWindowUpgrade?
Member
Author
There was a problem hiding this comment.
I tweaked the name and added comments to the base abstract property.
Contributor
|
Testing notes
Log file - |
Member
I ran into the first issue when I was helping Jameson test this. It seems like an easy enough mistake to make, should we fix it in a follow up PR? |
Member
Author
|
@alameenshah Thank you for trying this! The crash on the initial run as a regular issue is a known issue that I will fix. As you pointed out, it is because the regular user does not have permission to create this file. For the second issue, it looks (in part) like a configuration issue. But does bring up another question - how should we handle the GitHub upgrader on non-Windows platforms. |
nickgra
approved these changes
Jun 19, 2019
Contributor
|
One more testing note. After running |
jamill
force-pushed
the
enable_mac_upgrade
branch
from
92c624a to
214a173
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Final changes to enable upgrade on Mac MVP. This includes: