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

Observer plugins #1954

Closed
wants to merge 1 commit into from
Closed

Conversation

crownedgrouse
Copy link
Contributor

@crownedgrouse crownedgrouse commented Sep 11, 2018

This PR allow to add third party wx user interface plugins in Observer.
Plugins have to be declared in observer configuration.
Plugin can receive current observed node as start argument if needed.

@codeadict
Copy link
Contributor

Very nice! Will be great to have a simple example for this.

@crownedgrouse
Copy link
Contributor Author

Well, working on one. First step was to allow plugin.

@dgud dgud self-assigned this Sep 13, 2018
@hyloong
Copy link

hyloong commented Sep 15, 2018

Any one can look my question? Thanks.
https://stackoverflow.com/questions/52268503/erlang-memory-grow-up-when-cl-file

@crownedgrouse
Copy link
Contributor Author

Please send your questions on erlang mailing list, slack or bugs.erlang.org. Not the right place.

@crownedgrouse crownedgrouse force-pushed the observer_plugins branch 3 times, most recently from dbc4b0d to f360b3b Compare September 16, 2018 20:51
@crownedgrouse
Copy link
Contributor Author

crownedgrouse commented Sep 16, 2018

@codeadict I just pushed an example in commit 590eda0

@crownedgrouse crownedgrouse force-pushed the observer_plugins branch 2 times, most recently from 6713841 to 590eda0 Compare September 16, 2018 21:00

terminate(_Reason, _State = #state{frame=Frame}) ->
wxMiniFrame:destroy(Frame),
%wx:destroy() % DO NOT or observer will be closed too
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should go outside in Bold or as a warning. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like:

<warning>
  <p>To prevent the observer from being closed, please do not call
  <c>wx:destroy()</c> from your plugins.</p>
</warning>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will consider this

@codeadict
Copy link
Contributor

Hey, it looks very clear to me, will try compiling and adding this sample plugin tomorrow.

@essen
Copy link
Contributor

essen commented Sep 17, 2018

Why the limitation on 10 plugins? If a limitation is needed, maybe it should be 10 active plugins, with some UI to enable/disable plugins. If there is already this and I missed it, please disregard.

Otherwise I like this idea very much, being able to add Cowboy/Ranch-specific info to Observer has been on the back of my mind for a while.

@dgud
Copy link
Contributor

dgud commented Sep 17, 2018

Please fix travis errors above: (dialyzer, doc)
Remove whitespace fixes on otherwise untouched lines, causes unnecessary diffs.
Use try-catch instead of catch, catch usage is discouraged and should be avoided in new code.

@crownedgrouse
Copy link
Contributor Author

crownedgrouse commented Sep 17, 2018

@essen menu entries in wx are handled as integers and event match needs to checks that this ID integer is a plugin menu (and not another ID for another menu), so needs to limit the number between a lower and upper bound (declared as defines in beginning of code). 10 is not a limit, it was arbitrary chosen. I can increase this, but maybe need @dgud opinion. As plugins are declared in observer config, I'm not sure how I can add an UI to tell that this plugin is active or not. Simply add only useful plugin in config for your node ?

@crownedgrouse
Copy link
Contributor Author

@dgud thanks for the feedback. Indeed my editor has a diff tool that do not care of trailing whitespace... need to fix this config.

@crownedgrouse crownedgrouse force-pushed the observer_plugins branch 5 times, most recently from 95454f8 to 451e13c Compare September 23, 2018 16:02
@crownedgrouse
Copy link
Contributor Author

@dgud fixes are squashed. CI seems OK.

@dgud
Copy link
Contributor

dgud commented Jan 22, 2019

I don't like (nor understand) the usage of runtime_tools env, why can't you use observers environment.

@crownedgrouse
Copy link
Contributor Author

Maybe I miss something but observer is not an application but comes with runtime_tools? Tell me if I m wrong

@dgud
Copy link
Contributor

dgud commented Jan 22, 2019

The observer gui is part of the observer application, observer uses runtime_tools but it also uses stdlib and kernel and wx.

@crownedgrouse
Copy link
Contributor Author

Hum. I missed that observer is an application indeed. OK I will change that like it would have been since the beginning

@crownedgrouse
Copy link
Contributor Author

I was confused with the fact that runtime_tools need to be part of a release for node to be observed by observer. Sorry

	Add documentation
	Add simple example of plugin
@crownedgrouse
Copy link
Contributor Author

crownedgrouse commented Jan 27, 2019

Fixed and squashed

@dgud
Copy link
Contributor

dgud commented Aug 9, 2019

I have thought some on this PR and reached the conclusion that I want something better where
the plugin api allows to add new tabs and not just pop up a new window.

I'm not against a plugin-api but want something better.

I'm closing this for now.

@dgud dgud closed this Aug 9, 2019
@essen
Copy link
Contributor

essen commented Aug 9, 2019

Agree, need tabs! Let's make this happen.

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.

None yet

6 participants