-
Notifications
You must be signed in to change notification settings - Fork 917
Using HTML based UI for Change method parameters refactoring #3361
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
Using HTML based UI for Change method parameters refactoring #3361
Conversation
| </fileset> | ||
| <fileset> | ||
| <file>src/org/netbeans/modules/java/lsp/server/refactoring/ui/codicon.ttf</file> | ||
| <file>src/org/netbeans/modules/java/lsp/server/refactoring/ui/codicon.css</file> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files are taken from Microsoft codicons project. Readme mentions it is MIT licensed and points to standard MIT license.
| supr java.lang.Object | ||
| hfds initial | ||
|
|
||
| CLSS protected final org.netbeans.modules.java.lsp.server.ui.AbstractLspHtmlViewer$View |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sigtest doesn't like private View - making it protected to fix ant gen-sigtests-release problem introduced in 9e27e19
| + "var button = document.createElement('button');\n" | ||
| + "button.id = id;\n" | ||
| + "button.onclick = function() {;\n" | ||
| + " @org.netbeans.modules.java.lsp.server.htmlui.Buttons::clickButton0(Ljava/lang/String;Ljava/lang/Object;)(id, callback);\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTML/Java API infrastructure generates a callback class $JsCallbacks$ in the same package to be able to call from JavaScript to JVM. We don't want that class in the API - as such I am introducing new Buttons class and moving all @JavaScriptBody methods in here.
| + "var button = document.createElement('button');\n" | ||
| + "button.id = id;\n" | ||
| + "button.onclick = function() {;\n" | ||
| + " @org.netbeans.modules.java.lsp.server.ui.AbstractLspHtmlViewer::clickButton0(Ljava/lang/String;Ljava/lang/Object;)(id, callback);\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved into Buttons class in non-API package.
| @Override | ||
| public <C> C component(View view, Class<C> type) { | ||
| if (type == Void.class) { | ||
| load(view); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @dbalek , loading the view after newView is finished was one of recent changes. Previously the initialization started sooner then reference to View was returned and that lead to various races. Sometimes it worked, sometimes it deadlocked depending on the order of threads execution. Creating the instance first and initializing later when component(Void.class) is called fixes these races. I have probably forgotten to update the test - but it actually passed fine this morning (probably too positive order of execution).
sdedic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sane, seems to work (change method parameters refactoring)
This PR builds on top of #3357 and 9e27e19 (accidentally integrated into
master) to deliver the final vision of #3349 - e.g. to display rich HTML user interface for change method parameters refactoring. Most of the changes (like 54ddd6f) were done by Dušan. I might have polished them a bit, however my main task is to squash them into a PR with simpler change history.Should there be any problems with 9e27e19 I am ready to resolve it as part of this PR as soon as possible.