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

Implement "Show Plugin Output" checkbox. #2722

Merged
merged 1 commit into from Dec 17, 2019

Conversation

@rwasef1830
Copy link
Contributor

rwasef1830 commented Dec 10, 2019

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Compiled the code with Visual Studio

What is the purpose of your pull request?

  • Bug fix
  • Improvement
  • New feature

This is a simple improvement that allows viewing the plugin output window instead of always hiding it. It can be very useful to debug plugin issues instead of being forced to duplicate the config and run the plugin standalone just to see its output.

@chenshaoju

This comment has been minimized.

Copy link
Collaborator

chenshaoju commented Dec 11, 2019

Thanks for the send pull request, Please wait for the developer review the code. 😇

@studentmain

This comment has been minimized.

Copy link
Collaborator

studentmain commented Dec 11, 2019

Pass all test first

@rwasef1830 rwasef1830 force-pushed the rwasef1830:show-plugin-output branch from 33d5e6a to f65fb5f Dec 11, 2019
@rwasef1830

This comment has been minimized.

Copy link
Contributor Author

rwasef1830 commented Dec 11, 2019

@studentmain all tests pass now, my apologies. now it is failing due to unrelated problem with mbedtls test.

@studentmain

This comment has been minimized.

Copy link
Collaborator

studentmain commented Dec 12, 2019

Maybe redirect plugin's stdout into log? (If you did it, you can also replace the checkbox with a menu item in right-click menu)

Plugin's window can closed by user, which cause plugin exit.

@rwasef1830

This comment has been minimized.

Copy link
Contributor Author

rwasef1830 commented Dec 12, 2019

Each plugin may have its own way of logging which can be set using plugin arguments. Also no problem if the user closed the plugin window, it will be automatically restarted, so it's not a problem in my opinion.

This is just a simple switch to see the output of the plugin just in case the plugin displayed any messages useful for troubleshooting. It is meant to be used temporarily. And it can accomodate any complicated console output of any plugin.

To make it log to file will complicate things a little (need to log stdout and stderr), stdout or stderr might use non printable characters ... plugin may have its own logging... etc.

What do you think ?

@studentmain

This comment has been minimized.

Copy link
Collaborator

studentmain commented Dec 12, 2019

it's not a problem in my opinion.

Yes, it's not a big problem. The feature is ok (if user know what they did).

Consider what happened in plugin command line inputbox (#2212), I suggest put the switch in right click menu > "Help", just like verbose logging switch.

@rwasef1830

This comment has been minimized.

Copy link
Contributor Author

rwasef1830 commented Dec 12, 2019

@studentmain The reason why I put the checkbox in that dialog is to allow the user to press Apply and cause restart of the plugin with the restart of the server. If I put it in right-click, and the user choose it, it will not do any effect, and I don't want to cause restart of the server unnecessarily by right-click menu.

This way, all the options that cause restart of the server and plugin are in 1 place (the server config window) and it makes more sense for the user without any surprises. Because the user already expects that the server will be restarted when they press Apply or Ok and we want this to happen when they change the "Show plugin output" setting.

What do you think ?

@studentmain

This comment has been minimized.

Copy link
Collaborator

studentmain commented Dec 12, 2019

If I put it in right-click, and the user choose it, it will not do any effect

Just reload controller in callback function.

all the option that cause restart of the server and plugin are in 1 place

This feature is for debugging, so I'd like put it near other debugging option. BTW, server also restart when we toggle system proxy mode......

@rwasef1830 rwasef1830 force-pushed the rwasef1830:show-plugin-output branch from f65fb5f to b83f5b3 Dec 12, 2019
@rwasef1830

This comment has been minimized.

Copy link
Contributor Author

rwasef1830 commented Dec 12, 2019

@studentmain Done. It is now as you described to me. Help > Show Plugin Output

@studentmain

This comment has been minimized.

Copy link
Collaborator

studentmain commented Dec 12, 2019

Looks good to me, thank you.

@celeron533

This comment has been minimized.

Copy link
Collaborator

celeron533 commented Dec 13, 2019

A very good way for debugging and tracing error

@studentmain

This comment has been minimized.

Copy link
Collaborator

studentmain commented Dec 13, 2019

@celeron533 Let's merge them?

@studentmain studentmain merged commit 2f2c3c3 into shadowsocks:master Dec 17, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.