Skip to content
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

Enable starting apps with command line arguments when run as admin is selected #6923

Merged
merged 5 commits into from
Oct 21, 2020

Conversation

royvou
Copy link
Contributor

@royvou royvou commented Sep 30, 2020

Summary of the Pull Request

This enables the command line arguments to also be passed to the "Run as admin" command, and also unifies both methods to create ProcessStartInfo which fixes another bug.

Implementations follows the same logic as Microsoft.Plugin.Folder where the state is maintained in the Result.ContextData Property (That way we also don't need to extend any of the Wox.* Projects).

PR Checklist

Info on Pull Request

See summary, fixes the issue mentioned in #6916 which was a feature not implemented / overseen at #5791.

Validation Steps Performed

  • Part 1
    • Ran all tests
    • Manually type wt -- ping 1.1.1.1 -t
    • Validated a UAC prompt happen, Windows Terminal Launches and the ping starts to happen.
  • Part 2
    • Add YouTube as PWA in Microsoft Edge (Chromium based one)
    • Search for YouTube
    • Open YouTube normally
    • Validate the YouTube PWA opened
    • Search for YouTube
    • Open YouTube as Admin (Context Menu)
    • Validate a UAC Prompt happened and the YouTube PWA openedd

…sStartInfo fixes missing LnkResolvedPath missing in contextmenu (E.g. Shortcuts / PWA's).
@crutkas
Copy link
Member

crutkas commented Oct 1, 2020

this change broke the unit tests

@royvou
Copy link
Contributor Author

royvou commented Oct 1, 2020

(Hmmm Github should notify me for that, @crutkas thanks for the headsup!)

The file is checked in now and running :)

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 7, 2020

@royvou
Is this PR ready to review or do we waiting for something?

@royvou
Copy link
Contributor Author

royvou commented Oct 7, 2020

@royvou
Is this PR ready to review or do we waiting for something?

It is ready!

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 7, 2020

@somil55, @ryanbodrug-microsoft
Please review this PR.

@crutkas crutkas requested a review from a team October 9, 2020 04:28

namespace Microsoft.Plugin.Program
{
internal class SearchResult
Copy link
Contributor

@dsrivastavv dsrivastavv Oct 9, 2020

Choose a reason for hiding this comment

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

I think it would be better to make ProgramArguments part of Result class. SearchResult here seems to have the same meaning as Result. @ryanbodrug-microsoft Thoughts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning for not doing that was I did not want to change any of the wox namespaces, it was my fist idea as well. But it is something to consider.

Copy link
Contributor

Choose a reason for hiding this comment

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

@royvou It is fine to make changes in wox namespace. And I feel this would be a cleaner way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update the PR later this week !

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a note the PR is updated 👍

@dsrivastavv
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsrivastavv
Copy link
Contributor

LGTM! Thanks for this PR @royvou

@crutkas crutkas merged commit 29ed39c into microsoft:master Oct 21, 2020
@royvou royvou deleted the feature/enablerunasadminarguments branch October 21, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants