Skip to content

Conversation

@taooceros
Copy link
Member

@taooceros taooceros commented Jun 4, 2021

It seems that System.Text.Json won't parse base class property? Add property name fix that. This change fix the broken python plugin callback.
#459

Do we want a hotfix for that (though we don't have much python plugin)?

@taooceros
Copy link
Member Author

It is also possible that this is caused by the fact that System.Text.Json is case sensitive, but it is supposed to work with camalcase with property.

@taooceros taooceros requested a review from jjw24 June 4, 2021 05:06
@taooceros taooceros self-assigned this Jun 4, 2021
@taooceros taooceros added the bug Something isn't working label Jun 4, 2021
@jjw24 jjw24 merged commit fc695c5 into dev Jun 4, 2021
@jjw24 jjw24 deleted the fixJsonRPCCallback branch June 4, 2021 07:54
@jjw24
Copy link
Member

jjw24 commented Jun 4, 2021

I think we can do a hot fix as @AnthonMS is relying on this right?

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

I can just make my functions camelCase if that's the problem. I'm not quite sure what the problem is, since other python plugins work.

@taooceros
Copy link
Member Author

I can just make my functions camelCase if that's the problem. I'm not quite sure what the problem is, since other python plugins work.

No camelCase is not the problem. Would you please check the log?

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

That's the problem, the logs don't show anything for the JsonRPCAction callback or anything. If I make a syntax error in the function I give through the JsonRPCAction, JsonRPCAction will send some error logs to the log file. But when I just execute the plugin I have linked a pastebin to on my issue, no errors occure in the logs.

@taooceros
Copy link
Member Author

Are you able to get the result showing on the list? This patch fix the unexpected loss of the action callback stored in the result item. Sorry we didn't find that before.

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

I get the results in flow, but the function is never called when I click on it. I test it by creating a file and writing a line to it. And it never does that. I dont know how I can test a function beyond that approach. My thought was to create a log file for my plugin where I could "Print" stuff, so I could debug a little easier.

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

Also, when I set the JsonRPCAction "dontHideAfterAction": True. It still closes the Flow Launcher. So it's like the JsonRPCAction is being ignored completely.

@taooceros
Copy link
Member Author

I get the results in flow, but the function is never called when I click on it. I test it by creating a file and writing a line to it. And it never does that. I dont know how I can test a function beyond that approach. My thought was to create a log file for my plugin where I could "Print" stuff, so I could debug a little easier.

Also, when I set the JsonRPCAction "dontHideAfterAction": True. It still closes the Flow Launcher. So it's like the JsonRPCAction is being ignored completely.

Yeah that's a bug, and this patch will fix it. Sorry for the bug.

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

I tried using the PythonPluginTemplate test.py script, and there the result was this https://pastebin.com/zGYZ1tm7

So that's an example of what is being sent to flow and the result I get back.

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

I get the results in flow, but the function is never called when I click on it. I test it by creating a file and writing a line to it. And it never does that. I dont know how I can test a function beyond that approach. My thought was to create a log file for my plugin where I could "Print" stuff, so I could debug a little easier.

Also, when I set the JsonRPCAction "dontHideAfterAction": True. It still closes the Flow Launcher. So it's like the JsonRPCAction is being ignored completely.

Yeah that's a bug, and this patch will fix it. Sorry for the bug.

You don't have to say sorry, I'm a developer myself, so I know there will always be bugs until someone finds them :)
And I really appreciate all the help from all of you guys, it gives me hope, that it will be possible to get my plugin working.

Bit if I install the latest release 1.8.0, this issue will be fixed? And to update flow launcher, do I just download the new release and run it, or do I have to uninstall the old one?
I wish I was not at the office, so I could test it. But I will try it out tonight (In about 10 hours, I say it like this because timezones)

@taooceros
Copy link
Member Author

Bit if I install the latest release 1.8.0, this issue will be fixed? And to update flow launcher, do I just download the new release and run it, or do I have to uninstall the old one?

Just download it and run it. No need for uninstall the old one.

@jjw24
Copy link
Member

jjw24 commented Jun 4, 2021

Bit if I install the latest release 1.8.0, this issue will be fixed? And to update flow launcher, do I just download the new release and run it, or do I have to uninstall the old one?
I wish I was not at the office, so I could test it. But I will try it out tonight (In about 10 hours, I say it like this because timezones)

@AnthonMS Yeah give the 1.8.0 pr build artifact a go or the one here in this pr, see if it fixes the problem, you can access it right?

@taooceros
Copy link
Member Author

I don't believe 1.8.0 will be released by tonight, but you can try out our development branch.
https://ci.appveyor.com/api/buildjobs/0ewdwb8ehjqrnv04/artifacts/Output%2FPackages%2FFlow-Launcher-v1.7.2.exe

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

Bit if I install the latest release 1.8.0, this issue will be fixed? And to update flow launcher, do I just download the new release and run it, or do I have to uninstall the old one?
I wish I was not at the office, so I could test it. But I will try it out tonight (In about 10 hours, I say it like this because timezones)

@AnthonMS Yeah give the 1.8.0 pr build artifact a go or the one here in this pr, see if it fixes the problem, you can access it right?

I can access the Release log and I can see the merge on the dev branch, but I'm uncertain how I will update it. Do I just download the .exe @taooceros have linked to? Because it's still names 1.7.2

@taooceros
Copy link
Member Author

@jjw24 Also I think there's a potential bug. When the action doesn't return anything, an error will be logged. Do we want to allow empty message when executing callback method?
@AnthonMS I have tried your program with this branch and it works. Although an error will be logged because the callback message doesn't provide any message, that won't be a large issue for now.

I can access the Release log and I can see the merge on the dev branch, but I'm uncertain how I will update it. Do I just download the .exe @taooceros have linked to?

yes

@AnthonMS
Copy link

AnthonMS commented Jun 4, 2021

@AnthonMS I have tried your program with this branch and it works. Although an error will be logged because the callback message doesn't provide any message, that won't be a large issue for now.

The callback message, is that something I am doing wrong in my program, or is that a bug from FlowLauncher?

I can access the Release log and I can see the merge on the dev branch, but I'm uncertain how I will update it. Do I just download the .exe @taooceros have linked to?

yes

Okay, thanks for all the help!

@taooceros
Copy link
Member Author

taooceros commented Jun 4, 2021

The callback message, is that something I am doing wrong in my program, or is that a bug from FlowLauncher?

a bug from Flow, or a design limitation for executing api call.

@jjw24
Copy link
Member

jjw24 commented Jun 4, 2021

@jjw24 Also I think there's a potential bug. When the action doesn't return anything, an error will be logged. Do we want to allow empty message when executing callback method?

Maybe be consistent with c# plugin design where you can return empty list right? we do not need to log anything or treat it as an error, just return an empty list.

@taooceros
Copy link
Member Author

@jjw24 Also I think there's a potential bug. When the action doesn't return anything, an error will be logged. Do we want to allow empty message when executing callback method?

Maybe be consistent with c# plugin design where you can return empty list right? we do not need to log anything or treat it as an error, just return an empty list.

No it is allowing empty list. I am talking about the message that should be print out for action callback. If the message is empty, Flow will consider something wrong happens.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

can you point me to the code please

@taooceros
Copy link
Member Author

can you point me to the code please

Sure, I am afk now. Will do that soon.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Actually, you probably have more knowledge with this part of the code than I do, I will leave this to you how flow handles the empty message in this scenario

@taooceros
Copy link
Member Author

protected string Execute(ProcessStartInfo startInfo)
        {
            try
            {
                using var process = Process.Start(startInfo);
                if (process == null) return string.Empty;

                using var standardOutput = process.StandardOutput;
                var result = standardOutput.ReadToEnd();

                if (string.IsNullOrEmpty(result))
                {
                    using var standardError = process.StandardError;
                    var error = standardError.ReadToEnd();
                    if (!string.IsNullOrEmpty(error))
                    {
                        Log.Error($"|JsonRPCPlugin.Execute|{error}");
                        return string.Empty;
                    }

                    Log.Error("|JsonRPCPlugin.Execute|Empty standard output and standard error.");
                    return string.Empty;
                }

                if (result.StartsWith("DEBUG:"))
                {
                    MessageBox.Show(new Form { TopMost = true }, result.Substring(6));
                    return string.Empty;
                }

                return result;

            }
            catch (Exception e)
            {
                Log.Exception(
                    $"|JsonRPCPlugin.Execute|Exception for filename <{startInfo.FileName}> with argument <{startInfo.Arguments}>",
                    e);
                return string.Empty;
            }
        }

The callback will call the exercute method, which will Log an error when the resulted print out is empty.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Actually why would the result be empty? Shouldnt the plugin return an empty result object string(assuming that's what is returned)?

@taooceros
Copy link
Member Author

Actually why would the result be empty? Shouldnt the plugin return an empty result object string(assuming that's what is returned)?

Yeah they are supposed to return a template.

@jjw24
Copy link
Member

jjw24 commented Jun 5, 2021

Maybe we can mention that in the error log to make it clear?

@AnthonMS
Copy link

AnthonMS commented Jun 5, 2021

@jjw24 @taooceros I finally had time to test the hot fixed version of Flow Launcher. And now when I click on a action message I get this Exception Message Popup and the logs here. Can one of you decipher this error message for me? I don't seem to completely understand what the problem is. I have attached the log file, the exception message and I have run the test.py on it to see the result it sends back.

Just did some more testing, I can see, even though I send paramters, the result it sends back does not contain any of the parameters. And it looks like the arguments aren't passed neither. So I'm pretty sure there's still some parsing errors. But now there's at least some error messages to go from. I'm not sure if this is the error you guys are talking about above.
test result.txt
2021-06-05.txt
Exception Message.txt

@taooceros
Copy link
Member Author

Hmmm for now, would you please check out the template for action callback and add that to the callback method? This exception means that Flow is trying to parse an empty string.
I am not quite sure what's the template, but I believe there's some information listed in the python template repo.

@AnthonMS
Copy link

AnthonMS commented Jun 5, 2021

Hmmm for now, would you please check out the template for action callback and add that to the callback method? This exception means that Flow is trying to parse an empty string.
I am not quite sure what's the template, but I believe there's some information listed in the python template repo.

I'm not quite sure I am following what I need to test. Do you mean use the sendActionMess function from here?

But I think the empty string must be in the args, since the result I get back, has the arguments passed in the array, but it only have an empty string inside it in the callback method. That is the test result.txt I have linked to above.
As you can see in the bottom json objects, The args I pass is "ha", "light", "test_entity" but args I get in the callback is ""
And it's the exact same with the parameters in the JsonRPCAction. If I send some parameters to JsonRPCAction, the parameters array I get back in the callback is empty.

@taooceros
Copy link
Member Author

Sorry I was tired yesterday. Let me take a look on the code and I will reach out soon.

@taooceros
Copy link
Member Author

taooceros commented Jun 6, 2021

The code suggests that you are supposed to return a JsonRPCRequestModel, which is the base class of the same ReponseModel that you are supposed to return with the Query, but just without the DontHideAfterAction, but I guess you can just use the same template, and that property will be ignored. So, you can either choose to use SendNormalMessage or SendActionMessage (when you want to execute Flow's api after the action).
@AnthonMS Would you please provide the python code so that I can directly check the parameters transfer with debugging Flow? The args shouldn't lose.

@AnthonMS
Copy link

AnthonMS commented Jun 6, 2021

The code suggests that you are supposed to return a JsonRPCRequestModel, which is the base class of the same ReponseModel that you are supposed to return with the Query, but just without the DontHideAfterAction, but I guess you can just use the same template, and that property will be ignored. So, you can either choose to use SendNormalMessage or SendActionMessage (when you want to execute Flow's api after the action).
@AnthonMS Would you please provide the python code so that I can directly check the parameters transfer with debugging Flow? The args shouldn't lose.

https://pastebin.com/dSU9sE7b

That is basically my code. I'm afk right now, so can't give det 100% correct. But the changes are only, in the query function I only call the sendActionMess and I pass the args as the only parameters. I then use the test.py from the python template. I hav only changed the query args in the test.py script to something like ["ha", "light", "entiry_id"]

@AnthonMS
Copy link

AnthonMS commented Jun 6, 2021

@taooceros Here you go, sorry for the wait, haven't had time to sit down at my computer all day.

That is the ui.py (Main class). So I send the args in the subtitle string and in the parameters of the function I am calling. And both of the places in the result callback, it is empty lists.

You can see the result json object I get in the callback here.

@taooceros
Copy link
Member Author

I found the issue. It is because you have return the JSON parameters as Array>, which is like [["1","2","3"]], which makes Flow unable to correctly escape the string inside the inner array. I will try to replace Flow's self-built ToString method with the Json library and see whether that can handle the issue.

@taooceros
Copy link
Member Author

Let me try editing it and check whether it should work.

@taooceros
Copy link
Member Author

Hmmm There's a bug in flow causing this not working. System.Text.Json wraps the value with a JsonElement, which makes Flow unable to check it as string..... This is unexpected, and I will try fix it soon.

@taooceros
Copy link
Member Author

@AnthonMS I add a fix to that. You can take a try if you want. I test your file, and it works both with [['']] and [''].

@taooceros
Copy link
Member Author

I will suppress the JsonError in a moment.

@AnthonMS
Copy link

AnthonMS commented Jun 7, 2021

@AnthonMS I add a fix to that. You can take a try if you want. I test your file, and it works both with [['']] and [''].

I'm not at my computer at the moment and won't be until tonight again. But if you throw in a link to an artefact with the fix, I will give it a go when I have time.

@taooceros
Copy link
Member Author

@AnthonMS Here you go
https://ci.appveyor.com/api/buildjobs/92xbue5mtiiyk33i/artifacts/Output%2FPackages%2FFlow-Launcher-v1.7.2.exe

@taooceros taooceros added this to the 1.8.0 milestone Jun 7, 2021
@Garulf
Copy link
Member

Garulf commented Jul 10, 2021

@jjw24 @taooceros I finally had time to test the hot fixed version of Flow Launcher. And now when I click on a action message I get this Exception Message Popup and the logs here. Can one of you decipher this error message for me? I don't seem to completely understand what the problem is. I have attached the log file, the exception message and I have run the test.py on it to see the result it sends back.

Just did some more testing, I can see, even though I send paramters, the result it sends back does not contain any of the parameters. And it looks like the arguments aren't passed neither. So I'm pretty sure there's still some parsing errors. But now there's at least some error messages to go from. I'm not sure if this is the error you guys are talking about above.
test result.txt
2021-06-05.txt
Exception Message.txt

Getting the same exception message when using JsonRPCAction, is there a work-around?

@taooceros
Copy link
Member Author

You can check the prerelease in the pinned discussion. That should include a fix for this.

@Garulf
Copy link
Member

Garulf commented Jul 10, 2021

You can check the prerelease in the pinned discussion. That should include a fix for this.

Thank you! Didn't realize the binary was there.

That fixed all the issues I was having.

@taooceros
Copy link
Member Author

No problem😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants