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

Experimental java agent support #42

Merged
merged 2 commits into from
Sep 3, 2020
Merged

Experimental java agent support #42

merged 2 commits into from
Sep 3, 2020

Conversation

soisetu
Copy link
Contributor

@soisetu soisetu commented Sep 2, 2020

JavaFXLibrary can now be attached to process as java agent.

@soisetu
Copy link
Contributor Author

soisetu commented Sep 2, 2020

I can revert formatter changes if you want to. Forgot that our company formatter does all kinds of things.

JavaFXLibrary can now be attached to process as java agent.
Copy link
Contributor

@sampeson sampeson left a comment

Choose a reason for hiding this comment

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

Looks good to me

@psaikkonen
Copy link
Contributor

Looks good!

Using launch keyword is still required but instead of starting new application keyword initializes Stage for library.

Was thinking if we should add a separate keyword for this, like Attach To JavaFX Application?

We should also update the docs (both readme and keyword documentation) about how to get started with the library, I think currently there is no mention about the required usage of Launch JavaFX Application elsewhere at all. But this can be done outside of this PR.

@sampeson
Copy link
Contributor

sampeson commented Sep 2, 2020

We should have the same as with README.md about the java agent -mode in JavaFXLibrary\src\main\java\libdoc-documentation.txt, it goes in the general section of documentation.

Copy link
Contributor

@sampeson sampeson left a comment

Choose a reason for hiding this comment

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

Excellent

@soisetu
Copy link
Contributor Author

soisetu commented Sep 2, 2020

Looks good!

Using launch keyword is still required but instead of starting new application keyword initializes Stage for library.

Was thinking if we should add a separate keyword for this, like Attach To JavaFX Application?

We should also update the docs (both readme and keyword documentation) about how to get started with the library, I think currently there is no mention about the required usage of Launch JavaFX Application elsewhere at all. But this can be done outside of this PR.

I agree with new keyword for attaching. Maybe keyword should be Initialize JavaFXLibrary since it is already attached and just needs to get reference to Stage? Another option is to make initializing automatic when started in agent mode: try to find stage automatically and wait for stage. Or try to find stage if not initialized when executing any other keyword?

I would also add support for attaching to process via Java Attach API. I investigated that it could be done pretty easily but build tools need to have tools.jar in classpath.

@sampeson sampeson merged commit bcdf4c1 into eficode:development Sep 3, 2020
@psaikkonen
Copy link
Contributor

Automatic init would make sense since the user is already telling that I want to attach to this process by setting the library as an agent. Then again a separate keyword would give the user more control.

Or try to find stage if not initialized when executing any other keyword?

This could be the best option since the user would still have control over when the initialization happens, but it would just work automatically. I'll create an issue for this.

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.

3 participants