add initial ShellRun support for JsonRPC #742

spedrickson · 2021-10-09T09:41:11Z

Saw here #244 (comment) that ShellRun (among other JsonRPC methods) wasn't supported yet.

Didn't see it being worked on elsewhere, so here's a very basic attempt at supporting it.

I went with no window creation just because it fits the plugin I'm building, but that can easily be changed/exposed as a parameter.

taooceros · 2021-10-09T23:09:36Z

Thanks for pull request! It is a missed api.
Though I am wondering why you just directly access the cmd with python or other language 🧐.

I also agree with we shouldn't have window displayed here.

spedrickson · 2021-10-09T23:26:05Z

Though I am wondering why you just directly access the cmd with python or other language 🧐

Hmm, maybe I'm misunderstanding the Flow -> Plugin interaction, but doesn't the plugin only receive a single string? Or is there a way for Python plugins to receive an event when the user selects/activates one of the results? The FlowLauncher class the example plugins inherit from only seems to have the query method

For context, the plugin I'm working on that required it is similar to file search, where I want the result opened with the default file handler when the user activates it, so it would be just as easy to launch from Python if those events can be received.

taooceros · 2021-10-09T23:30:12Z

Though I am wondering why you just directly access the cmd with python or other language 🧐

Hmm, maybe I'm misunderstanding the Flow -> Plugin interaction, but doesn't the plugin only receive a single string? Or is there a way for Python plugins to receive an event when the user selects/activates one of the results? The FlowLauncher class the example plugins inherit from only seems to have the query method

You can actually. Flow will query again when user click the result, and this time it will go to the method registered in the callback message, and pass the ContextData of the result to the method.

For context, the plugin I'm working on that required it is similar to file search, where I want the result opened with the default file handler when the user activates it, so it would be just as easy to launch from Python if those events can be received.

It is just as easy to launch from python actually 🤣

taooceros · 2021-10-09T23:30:41Z

I know our document is not quite clear, so if you have encountered any issue, feel free to open a discussion for help.

Flow.Launcher/PublicAPIInstance.cs

taooceros · 2021-10-18T21:54:20Z

Actually I am wondering why we need this method, which is just a wrapper. Why can't plugin just manager the shell method in their own code? @Flow-Launcher/team thoughts?

spedrickson · 2021-10-18T22:22:02Z

Actually I am wondering why we need this method, which is just a wrapper

Just my opinion as a user, but it makes sense to be able to associate a simple shell command with each result returned by a plugin. The same way I might want a user-selected result to open settings or modify the existing query.

Doing it locally in the plugin requires handling the user-selection event, which isn't yet documented AFAIK (at least for Python plugins).

Here's the use-case I've been using locally, where it works quite well. The query provides a string, my plugin queries the relevant database, and returns the music metadata + filepath.

When user clicks one of the results, it just launches the file with whatever the default program is.

This also keeps it simple for plugin developers. Handling user selection events on the plugin-side will be nice once it's documented, but this is perfect for simple one-line shell commands that are different for each result.

For reference, this is what my plugin is sending to use this RPC API:

{
  "Title": "ABBA - The Definitive Collection - People Need Love",
  "SubTitle": "F:\\Media\\Music\\ABBA\\The Definitive Collection\\1.01 - People Need Love.flac", 
  "IcoPath": "assets/favicon.ico", 
  "JsonRPCAction": {
      "method": "Flow.Launcher.ShellRun", 
      "parameters": ["\"F:\\Media\\Music\\ABBA\\The Definitive Collection\\1.01 - People Need Love.flac\""]
  }
}

Zeroto521 · 2021-10-19T01:44:48Z

So to clarify this PR, I repeat again.

This PR does:

This PR aims to let flow run cmd, then let this cmd.exe run other plugins since we have Language and ExecuteFileName via Language ExecuteFileName parameters.

Before there did:

flow run other plugins, so there had to need to know where the path of the interpreter.

Flow.Launcher/Flow.Launcher.Core/Plugin/PythonPlugin.cs

Lines 35 to 40 in f6f306e

    
           protected override Task<Stream> RequestAsync(JsonRPCRequestModel request, CancellationToken token = default) 
        
           { 
        
               _startInfo.ArgumentList[2] = request.ToString(); 
        
               return ExecuteAsync(_startInfo, token); 
        
           }

Flow.Launcher/Flow.Launcher.Core/Plugin/ExecutablePlugin.cs

Lines 27 to 31 in f6f306e

    
           protected override Task<Stream> RequestAsync(JsonRPCRequestModel request, CancellationToken token = default) 
        
           { 
        
               _startInfo.Arguments = $"\"{request}\""; 
        
               return ExecuteAsync(_startInfo, token); 
        
           }

Flow.Launcher/Flow.Launcher.Core/Plugin/PythonPlugin.cs

Lines 28 to 29 in f6f306e

    
           var path = Path.Combine(Constant.ProgramDirectory, JsonRPC); 
        
           _startInfo.EnvironmentVariables["PYTHONPATH"] = path;

Flow.Launcher/Flow.Launcher.Core/Plugin/ExecutablePlugin.cs

Line 19 in f6f306e

FileName = filename,

taooceros · 2021-10-19T01:50:33Z

So to clarify this PR, I repeat again.

This PR does:

This PR aims to let flow run cmd, then let this cmd.exe run other plugins since we have Language and ExecuteFileName via Language ExecuteFileName parameters.

Before there did:

flow run other plugins, so there had to need to know where the path of the interpreter.

Flow.Launcher/Flow.Launcher.Core/Plugin/PythonPlugin.cs

Lines 35 to 40 in f6f306e

protected override Task<Stream> RequestAsync(JsonRPCRequestModel request, CancellationToken token = default)

{

_startInfo.ArgumentList[2] = request.ToString();

return ExecuteAsync(_startInfo, token);

}

Flow.Launcher/Flow.Launcher.Core/Plugin/ExecutablePlugin.cs

Lines 27 to 31 in f6f306e

protected override Task<Stream> RequestAsync(JsonRPCRequestModel request, CancellationToken token = default)

{

_startInfo.Arguments = $"\"{request}\"";

return ExecuteAsync(_startInfo, token);

}

Flow.Launcher/Flow.Launcher.Core/Plugin/PythonPlugin.cs

Lines 28 to 29 in f6f306e

var path = Path.Combine(Constant.ProgramDirectory, JsonRPC);

_startInfo.EnvironmentVariables["PYTHONPATH"] = path;

Flow.Launcher/Flow.Launcher.Core/Plugin/ExecutablePlugin.cs

Line 19 in f6f306e

FileName = filename,

Sorry I don't quite get your point. Python plugin can run those command in cmd easily as well, because cmd.exe is in environments path, which can be accessed anywhere.
I think his issue is mainly that our document is not sufficient enough. They don't understand how flow handle the callback of the result, so using these cmd method may be easy. Therefore, I would consider the docs is the place issue.
I don't understand this PR is that complex. What he is trying to do is let flow provide them a wrapper to execute command in cmd. No relate to running other plugin.

spedrickson · 2021-10-19T01:57:05Z

I don't understand this PR is that complex. What he is trying to do is let flow provide them a wrapper to execute command in cmd. No relate to running other plugin.

Correct, this PR only seeks to implement a JsonRPC method that is specified in the docs (here). This method call only has two parts, the method name (Flow.Launcher.ShellRun) and a single param (whatever [cmd] you want passed to the shell).

It also makes sense for some other languages that aren't really designed for interacting with the OS, like a JS plugin. Much easier to just have Flow Launcher handle that execution once people start making plugins there.

Garulf · 2021-10-19T03:18:59Z

Does this launch the shell as a separate process? Could help with some niche issues Python/shell has with Flow.

Zeroto521 · 2021-10-19T03:24:57Z

For example os.system("command") could run like the cmd.

# cmd.py
import os

os.system("python hello.py")

# hello.py

print("hello")

result as following

> python cmd.py
hello

For C#, it could do in some way.
It could fix the multi-language plugin problem.

spedrickson · 2021-10-19T04:35:17Z

Does this launch the shell as a separate process? Could help with some niche issues Python/shell has with Flow.

Yes, System.Diagnostic.Process should start as a separate process, although the output of the shell command currently isn't received anywhere either.

I see two RPC methods in the docs that seem to be getters, so presumably there's a way to receive input back from an RPC method, but I haven't seen it described anywhere.

taooceros · 2021-10-29T19:56:15Z

I see two RPC methods in the docs that seem to be getters, so presumably there's a way to receive input back from an RPC method, but I haven't seen it described anywhere.

There is. Just type he method name in the method property in the response for the result. If the method is not started with Flow.Launcher., it will be invoked back to the python plugin with the method name. On that way, the Flow Launcher python template will find a method with that name and execute it.

jjw24 · 2021-11-02T20:30:23Z

I think this is ok to add in for:

query again when user click the result, and this time it will go to the method registered in the callback message, and pass the ContextData of the result to the method.

You can run os cmd in any language but i think the intention here is to allow running of the cmd when the user selects the result, which is tied to the method registered in the callback message.

It also makes sense for some other languages that aren't really designed for interacting with the OS, like a JS plugin. Much easier to just have Flow Launcher handle that execution once people start making plugins there.

yep.

Any additional concerns @taooceros @Zeroto521 @Garulf ?

jjw24 · 2021-11-02T20:32:14Z

Flow.Launcher/PublicAPIInstance.cs

+            startInfo.FileName = "cmd.exe";
+            startInfo.Arguments = $"/C {cmd}";
+            startInfo.CreateNoWindow = true;
+            process.Start();


@spedrickson have you tested this with your plugin? the method is not returning the result from the command

Yes it works for my plugin, but I'm only using it to launch files (identical to Browser plugin), not receive data back on the plugin side.

One concern with returning results: right now the commands are launched in a separate process to prevent locking up the Flow UI, so even long-running commands won't impact user experience. Maintaining a list of running processes is easy enough, but then we get into design decisions like whether return order should be maintained.

Might be worth adding a separate function like GetShellRun ,that works the same way but keeps the process and regularly polls it for output?

Garulf · 2021-11-04T17:34:39Z

I think this is ok to add in for:

query again when user click the result, and this time it will go to the method registered in the callback message, and pass the ContextData of the result to the method.

You can run os cmd in any language but i think the intention here is to allow running of the cmd when the user selects the result, which is tied to the method registered in the callback message.

It also makes sense for some other languages that aren't really designed for interacting with the OS, like a JS plugin. Much easier to just have Flow Launcher handle that execution once people start making plugins there.

yep.

Any additional concerns @taooceros @Zeroto521 @Garulf ?

I havent been able to test this as I was busy debugging a plugin. I'll take a look tonight.

jjw24 · 2021-11-17T10:06:11Z

hey @spedrickson your changes are good, no issues. I just took the opportunity to do a quick mini refactor so we can remove duplicate command execute code, hope it's ok :)

@taooceros when you get a chance please review and merge in if ok.

taooceros · 2021-11-17T16:20:10Z

ok, I am thinking about custom terminal host (like the windows terminal, powershell, etc). Probably it will be better to have this command executed in a place that can access setting?

taooceros · 2021-11-17T16:20:42Z

Other things look great for me, so after checking the place to place the command, we can merge it.

jjw24 · 2021-11-17T19:01:35Z

ok, I am thinking about custom terminal host (like the windows terminal, powershell, etc). Probably it will be better to have this command executed in a place that can access setting?

I exposed filename, so now you can pass in the shell program you want to run with, i.e. powershell.exe for running with powershell instead of cmd.exe

spedrickson · 2021-11-17T19:03:09Z

I exposed filename, so now you can pass in the shell program you want to run with, i.e. powershell.exe for running with powershell instead of cmd.exe

One concern with this change as-is, the /C argument is still hard-coded. Most other shells won't support that flag (Bash and Powershell use -c pretty sure).

Not sure what the best solution is, just pointing out a potential bug.

Garulf · 2021-11-17T19:08:51Z

I exposed filename, so now you can pass in the shell program you want to run with, i.e. powershell.exe for running with powershell instead of cmd.exe

One concern with this change as-is, the /C argument is still hard-coded. Most other shells won't support that flag (Bash and Powershell use -c pretty sure).

Not sure what the best solution is, just pointing out a potential bug.

Powershell and cmd both accept /C. Though not being hardcoded would allow for more options.

jjw24 · 2021-11-17T19:17:14Z

good pickup, i have added a condition to append /C to argument when cmd.exe

outdated, no longer using the code block

add initial ShellRun support for JsonRPC

d41dd78

taooceros assigned spedrickson Oct 11, 2021

taooceros previously requested changes Oct 18, 2021

View reviewed changes

Flow.Launcher/PublicAPIInstance.cs Outdated Show resolved Hide resolved

jjw24 reviewed Nov 2, 2021

View reviewed changes

jjw24 added 3 commits November 17, 2021 21:00

refactor to use ShellCommand

ae7de8d

Merge branch 'dev' into JsonRPCShellRun

a6ddaf9

fix formatting

02d101a

jjw24 enabled auto-merge November 17, 2021 10:06

version bump plugins

3e38a90

jjw24 added 2 commits November 18, 2021 05:56

add filename param to ShellRun command for running other shell types

ef56d91

update comment

8cf9390

update StartInfo arguments if cmd shell type

a7ef589

jjw24 added the enhancement New feature or request label Nov 28, 2021

jjw24 added this to the 1.9.0 milestone Nov 28, 2021

jjw24 approved these changes Nov 28, 2021

View reviewed changes

jjw24 merged commit e7198c6 into Flow-Launcher:dev Nov 28, 2021

jjw24 mentioned this pull request Dec 5, 2021

Release 1.9.0 | Plugin 2.1.0 #697

Merged

Uh oh!

add initial ShellRun support for JsonRPC #742

add initial ShellRun support for JsonRPC #742

Uh oh!

Conversation

spedrickson commented Oct 9, 2021

Uh oh!

taooceros commented Oct 9, 2021

Uh oh!

spedrickson commented Oct 9, 2021

Uh oh!

taooceros commented Oct 9, 2021

Uh oh!

taooceros commented Oct 9, 2021

Uh oh!

Uh oh!

taooceros commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spedrickson commented Oct 18, 2021

Uh oh!

Zeroto521 commented Oct 19, 2021

Uh oh!

taooceros commented Oct 19, 2021

Uh oh!

spedrickson commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Garulf commented Oct 19, 2021

Uh oh!

Zeroto521 commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spedrickson commented Oct 19, 2021

Uh oh!

taooceros commented Oct 29, 2021

Uh oh!

jjw24 commented Nov 2, 2021

Uh oh!

jjw24 Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

spedrickson Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

Garulf commented Nov 4, 2021

Uh oh!

jjw24 commented Nov 17, 2021

Uh oh!

taooceros commented Nov 17, 2021

Uh oh!

taooceros commented Nov 17, 2021

Uh oh!

jjw24 commented Nov 17, 2021

Uh oh!

spedrickson commented Nov 17, 2021

Uh oh!

Garulf commented Nov 17, 2021

Uh oh!

jjw24 commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

taooceros commented Oct 18, 2021 •

edited

Loading

spedrickson commented Oct 19, 2021 •

edited

Loading

Zeroto521 commented Oct 19, 2021 •

edited

Loading