Add the ability to open classes from the DevUI in the IDE #15296

geoand · 2021-02-24T10:21:49Z

TODO:

Add documentation (will do once the approach have been validated by the review - can be done in another PR)

geoand · 2021-02-24T10:22:22Z

This is what it currently looks like:

geoand · 2021-02-24T10:25:07Z

@stuartwdouglas @mkouba @maxandersen WDYT?

If you think this makes sense, I can proceed further

mkouba · 2021-02-24T13:32:50Z

...ment/src/main/java/io/quarkus/vertx/http/deployment/devmode/console/DevConsoleProcessor.java

+                // TODO: determine whether IntelliJ or other IDE is being used determine the proper link
+
+                // for this to work, users need to check 'Allow unsigned requests' in the Debugger Settings
+                String link = "http://localhost:63342/api/file/" + srcMainJavaPath + '/'


So localhost:63342 is something IntelliJ-specific?

Yes, IntelliJ starts a web server that handles a few API requests

mkouba · 2021-02-24T13:35:43Z

I like the idea. Do you think we'd be able to support all the popular IDEs?

geoand · 2021-02-24T13:37:45Z

I like the idea. Do you think we'd be able to support all the popular IDEs?

I sure hope so 😃

gastaldi · 2021-02-24T16:37:55Z

...ment/src/main/java/io/quarkus/vertx/http/deployment/devmode/console/DevConsoleProcessor.java

+                // TODO: determine whether IntelliJ or other IDE is being used determine the proper link
+
+                // for this to work, users need to check 'Allow unsigned requests' in the Debugger Settings
+                String link = "http://localhost:63342/api/file/" + srcMainJavaPath + '/'


This is cool. You can also use Desktop.getDesktop().open(file) to invoke the preferred editor in the host machine

I didn't know that, thanks!

That is probably going to be a good fallback if we can't determine what IDE is being used (which I hope we can)

maxandersen · 2021-02-24T17:52:33Z

Good idea. It's only intellij that had this server thing atm.

Fyi: started https://github.com/starfixdev/starfix to handle this and other usecases more generically. Still poc though :)

But vscode has a URL we can use.

Eclipse can too - but not uniform across platfotms.

I'll take a look and see if can make it work.

geoand · 2021-02-24T18:00:32Z

Thanks for the info Max!

I had seen vscode has its own protocol for the URL so I wasn't worried about it.
For Eclipse, I had no idea.

I'll take a look at the code you linked soon and see what can be done

stuartwdouglas · 2021-02-24T23:16:38Z

If we get this working can we also add it to the error page so we can open the class directly from the stack track?

geoand · 2021-02-25T07:02:05Z

If we get this working can we also add it to the error page so we can open the class directly from the stack track?

That's a superb idea

maxandersen · 2021-02-25T07:50:00Z

okey, so looking in more detail I'm grokking this better. This approach is dependent on generating a link that gets generated into the webpage thus it will only work with IDE's that has url or link support.

Thus the only two IDE's I know that supports this well is intellij and vscode with its vscode:// url form.

The good thing is that this is also what starfix enables - it will register: ide:// and then relay to users IDE.

Thus we probably want a setting like:

quarkus.ide.kind=intellj|vscode|starfix

and then generate the appropriate url - possibly also allow "custom" and have a .ide.customurl property.

Another alternative could be we don't generate a link to a url but just a GET which then local devmode will use to execute code locally...but that has security implications if you share a url then remote users could be opening IDE's on your local machine ;)

maxandersen · 2021-02-25T07:51:16Z

Another aspect is to open at certain line number - that will also be different per IDE/URL.

geoand · 2021-02-25T08:31:33Z

Another aspect is to open at certain line number - that will also be different per IDE/URL.

Line numbers would take a lot more work to get - we'd need to actually get them from debugging info in the .class file.
~~(I don't think that Jandex stores this info currently)~~ Jandex does not currently index the LineNumberTable attribute in bytecode, although we could certainly make it do so.

geoand · 2021-02-25T08:34:05Z

okey, so looking in more detail I'm grokking this better. This approach is dependent on generating a link that gets generated into the webpage thus it will only work with IDE's that has url or link support.

It doesn't have to be like this - I just did it to get some quick off the ground.
We can easily make the Quarkus process launch whatever needs to be launched in order to open the file in the IDE

geoand · 2021-02-27T15:22:27Z

okey, so looking in more detail I'm grokking this better. This approach is dependent on generating a link that gets generated into the webpage thus it will only work with IDE's that has url or link support.

It doesn't have to be like this - I just did it to get some quick off the ground.
We can easily make the Quarkus process launch whatever needs to be launched in order to open the file in the IDE

Looking at how Eclipse doesn't seem to have a way to do this, I am thinking that we will likely need to open the IDE from the "server-side", meaning that the link in the DevUI will be handled by a Vert.x Handler that will then do whatever is necessary to open the link in the IDE.

gastaldi · 2021-02-27T15:45:00Z

@geoand my thought exactly 👍🏻

maxandersen · 2021-02-27T17:16:12Z

i would not be too excited having this be something that we open server side for two reasons:

ux flow - if you are serving out devmode to someone you want to demo something their clicks will result you seing your ~IDE pop up where it should have been the users ide
when running remote dev, which side owns it?

something like starfix would fix it for eclipse btw.

gastaldi · 2021-02-27T17:22:46Z

@maxandersen that makes sense, remote dev is a clear use case that invalidates a server side approach. It would be great if we could use Starfix here.
Is there anyone else (besides you) working on it? I may lend a hand if necessary.

geoand · 2021-02-27T17:24:42Z

If we can get this to work from a browser URL, then sure, that makes the most sense.

But startix is something a user would need to install, right?
I don't expect many people to install anything additional to be honest...
And even if we do expect them to, how does starfix solve the problem? Does it register a URL protocol handler with the Operating System?

As for the other cases, do we really think that dev Mode running on another machine is a thing?
For remote-dev, we could just disable the links.

The way I see it, both options have pros and cons.

geoand · 2021-02-27T17:29:52Z

Also to be clear, relying on starfix would make this PR relatively easy - it would just create starfix URLs

gastaldi · 2021-02-27T17:30:11Z

gastaldi · 2021-02-27T17:34:10Z

Yeah, jokes aside, maybe the best UX would be to enable links ONLY if Starfix is installed (as a chrome extension).

AFAIK there is a Jetbrains chrome extension that does that already: https://chrome.google.com/webstore/detail/jetbrains-toolbox-extensi/offnedcbhjldheanlbojaefbfbllddna

geoand · 2021-02-27T17:36:27Z

Yeah, we really need to decide what we want the user experience to be.

The technical work should be simple (as demonstrated in this PR)

maxandersen · 2021-02-28T07:37:35Z

Is there anyone else (besides you) working on it? I may lend a hand if necessary.

no, and i havent spend much time since gsoc last year.
gsoc did verify though that it is very doable and got url protcol handler working for all three major ides.

Also to be clear, relying on starfix would make this PR relatively easy - it would just create starfix URLs

to start, i dont want to suggest to target starfix only. what im suggesting is that we make it url based to get resolved client side rather than handling it server side, at least by default.

we can do a detection in path to make a best guess for what will work, but have a property guiding it, like:

quarkus.ide.target=auto|vscode|idea|starfix

and then generate accordingly.

ultimately, we probably want to enable passing in not just physical location and line number but things like git url and classname as the location on disk might not match.

maxandersen · 2021-02-28T07:41:45Z

then you would do something like

quarkus.ide.openurl=ide://openClass?git=$git&class=$class&line=$line&location=$location

for starfix, where as for vscode it would just refer to location and line.

geoand · 2021-02-28T07:45:53Z

IIUC, there is no URL that could be used for Eclipse, correct?

…

On Sun, Feb 28, 2021, 09:41 Max Rydahl Andersen ***@***.***> wrote: then you would do something like quarkus.ide.openurl=ide://openClass?git=$git&class=$class&line=$line&location=$location for starfix, where as for vscode it would just refer to location and line. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDP26MQ6OU3OFNXWZBTTTBHXUJANCNFSM4YEHNPKQ> .

maxandersen · 2021-02-28T09:40:25Z

Not at this stage no.

I've opened eclipse issues for it but not been picked up yet.

Maybe we could add a endpoint that Will launch Process locally given request comes from localhost to guard against at.least a direct exploit ?

geoand · 2021-02-28T09:51:24Z

Yeah, that sounds reasonable

…

On Sun, Feb 28, 2021, 11:40 Max Rydahl Andersen ***@***.***> wrote: Not at this stage no. I've opened eclipse issues for it but not been picked up yet. Maybe we could add a endpoint that Will launch Process locally given request comes from localhost to guard against at.least a direct exploit ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDP6SIFTJ7AQ3N22SQWLTBIFRLANCNFSM4YEHNPKQ> .

geoand · 2021-03-11T10:27:21Z

I would prefer writing docs after this PR is in as I have already had to rebase 3 times since yesterday alone.

geoand · 2021-03-11T13:10:26Z

The failures are totally unrelated

maxandersen · 2021-03-11T19:28:28Z

I'll see if I can try it - where should I see the links be added ? In exceptions or just on arc bean Dev list?

geoand · 2021-03-11T19:30:24Z

Only in the Arc beans page.

But the plumbing is there to later on add it wherever we like

geoand · 2021-03-11T20:23:45Z

Unless there are objections, I'd like to merge this and not have to deal with any more conflicts (which are certain to arise)

maxandersen · 2021-03-12T00:23:57Z

is there a reason why the links aren't copyable ?

maxandersen · 2021-03-11T00:30:31Z

core/deployment/src/main/java/io/quarkus/deployment/ide/IdeProcessor.java

+            // the idea here is to auto-detect the special files that IDEs create
+            // and also the running IDE process if need be
+
+            if (ideFile.getDetectedIDEs().size() == 1) {


A nice approach to know which of potential many ide files that should win is to sort them by last modification time. Right now it seems old idea filled would win over more recent code artifacts. For example.

Good point, but it still doesn't help us determine Eclipse vs VSCode - which is why this whole dance is necessary.

This detects runtime IDEs, but doesn't even copy their path, so it also doesn't look at executable modification date, which was my issue, because my IDE was not in my path. I figured this could be a further improvement.

Even then, if you decide to do this, watch out that often executables are links and I'm not sure their modification dates can be trusted without resolving the link.

I am absolutely sure there will be improvements to be done in this area

maxandersen · 2021-03-12T00:26:18Z

core/deployment/src/main/java/io/quarkus/deployment/ide/IdeConfig.java

+    @ConfigItem(defaultValue = "auto")
+    public Target target;
+
+    enum Target {


any way for us to allow user to specify a custom command ?

maxandersen · 2021-03-12T00:27:48Z

core/deployment/src/main/java/io/quarkus/deployment/ide/IdeProcessor.java

+                } else {
+                    List<Ide> matches = new ArrayList<>();
+                    for (Ide file : ideFile.getDetectedIDEs()) {
+                        for (Ide process : runningIdes) {


oh wow - just realized you detecting running ides ....thats awesome :)

one caveat - this requires the IDE to be running when we start quarkus:dev, right?

also - while testing this having no IDE running i stopped getting links highlighted since no changes made and thus we never relooked for running processes ?

feels a bit weird.

and changing code to try trigger recalculation of running ides does not seem to be enough.

People close their IDE?

I do constantly :)

The IDE detection is necessary because there from the project files we can't really diferentiate if VSCode is Eclipse is being used.

If there is another way to differentiate an Eclise project from a VSCode project, then I'll gladly use it.

rigger recalculation of runni

When the changes result in a Quarkus dev restart, the processes are re-detected.

Weird - it stopped showing links for me :/

maxandersen · 2021-03-12T00:47:04Z

core/deployment/src/main/java/io/quarkus/deployment/ide/IdeConfig.java

+import io.quarkus.runtime.annotations.ConfigRoot;
+
+@ConfigRoot
+public class IdeConfig {


not sure whre the bug is but using dev config editor it makesthis "quarkus.ide.target=code' which gives an error.

also i believe the current property is "quarkus.ide" which i think is limiting our options.

i would suggest "quarkus.ide.command" or similar allowing us to do i.e

quarkus.ide.command=custom quarukus.ide.custom.command=open -a /Applications/Eclipse.app {filename}"

or similar in the future.

maxandersen

i really like where this is going but for some reason it stops working for me eventually. no linking happens.worked fine the first time.

but that is probably something we can fix in future update.

the only thing i would say to reconsider is to not use toplevel quarkus.ide property name space as the single value to set.

geoand · 2021-03-12T06:04:20Z

@maxandersen I don't see what the problem with the ide config is. Currently you set quarkus.ide.target to set an IDE from the enum values.
We could certainly add an optional command option later.
Am I missing something?

geoand · 2021-03-12T06:12:56Z

is there a reason why the links aren't copyable ?

The aren't actual links in the HTML sense. They are just spans that are clickable and what happens when you click on them is done in Javascript (in order to make it trivial for extensions to be able to use those functions and not have to worry about any of the internals of opening in the IDE)

maxandersen · 2021-03-12T07:18:17Z

@maxandersen I don't see what the problem with the ide config is. Currently you set quarkus.ide.target to set an IDE from the enum values.

We could certainly add an optional command option later.

Am I missing something?

Hmm Okey then I misread the code - what I got was that when I set .target I got an exception.

geoand · 2021-03-12T07:28:05Z

Hmm Okey then I misread the code - what I got was that when I set .target I got an exception.

Let me try it and see.

geoand · 2021-03-12T07:33:47Z

Hmm Okey then I misread the code - what I got was that when I set .target I got an exception.

Let me try it and see.

I just tried it and when I set vscode as the value of quarkus.ide.target, everything works as expected.
I tried both application.properties and setting the value from the Config UI.

geoand · 2021-03-12T08:38:33Z

@maxandersen and I discussed and agreed to get this in now and start using it. Any shortcomings or alternatives approaches can certainly be taken up as we get more hands on experience with DevUI entries using this feature.

@phillip-kruger would you be willing to integrate this with the Log output to allow users to open classes from exception stacktraces?

phillip-kruger · 2021-03-12T08:42:11Z

@geoand - yes, happy to help. As a separate PR though right ?

geoand · 2021-03-12T08:43:24Z

Thanks!

Definitely a separate PR. I'll merge this once CI completes

Comments addressed

phillip-kruger · 2021-03-16T10:01:21Z

@geoand - #15761

^^^ PR to add this to the log ^^^

quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/vertx labels Feb 24, 2021

mkouba reviewed Feb 24, 2021

View reviewed changes

gastaldi reviewed Feb 24, 2021

View reviewed changes

geoand force-pushed the open-in-ide branch from 6d79711 to c4d862b Compare March 11, 2021 10:27

FroMage approved these changes Mar 11, 2021

View reviewed changes

stuartwdouglas approved these changes Mar 11, 2021

View reviewed changes

maxandersen reviewed Mar 12, 2021

View reviewed changes

maxandersen previously requested changes Mar 12, 2021

View reviewed changes

geoand added 2 commits March 12, 2021 08:02

Add the plumbing needed to open classes from DevUI in IDE

8ea444f

Add the ability to open bean classes from DevUI in IDE

94f715b

geoand force-pushed the open-in-ide branch from c4d862b to 94f715b Compare March 12, 2021 06:11

geoand requested a review from maxandersen March 12, 2021 06:14

geoand merged commit f1bbb73 into quarkusio:master Mar 12, 2021

quarkus-bot bot added this to the 1.13 - master milestone Mar 12, 2021

geoand deleted the open-in-ide branch March 12, 2021 09:00

Add the ability to open classes from the DevUI in the IDE #15296

Add the ability to open classes from the DevUI in the IDE #15296

Conversation

geoand commented Feb 24, 2021 • edited Loading

geoand commented Feb 24, 2021

geoand commented Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkouba commented Feb 24, 2021

geoand commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxandersen commented Feb 24, 2021

geoand commented Feb 24, 2021

stuartwdouglas commented Feb 24, 2021

geoand commented Feb 25, 2021

maxandersen commented Feb 25, 2021

maxandersen commented Feb 25, 2021 • edited Loading

geoand commented Feb 25, 2021 • edited Loading

geoand commented Feb 25, 2021

geoand commented Feb 27, 2021

gastaldi commented Feb 27, 2021

maxandersen commented Feb 27, 2021

gastaldi commented Feb 27, 2021

geoand commented Feb 27, 2021 • edited Loading

geoand commented Feb 27, 2021

gastaldi commented Feb 27, 2021

gastaldi commented Feb 27, 2021

geoand commented Feb 27, 2021

maxandersen commented Feb 28, 2021

maxandersen commented Feb 28, 2021

geoand commented Feb 28, 2021 via email

maxandersen commented Feb 28, 2021

geoand commented Feb 28, 2021 via email

geoand commented Mar 11, 2021 • edited Loading

geoand commented Mar 11, 2021

maxandersen commented Mar 11, 2021

geoand commented Mar 11, 2021

geoand commented Mar 11, 2021

maxandersen commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxandersen Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

maxandersen left a comment

Choose a reason for hiding this comment

geoand commented Mar 12, 2021

geoand commented Mar 12, 2021 • edited Loading

maxandersen commented Mar 12, 2021

geoand commented Mar 12, 2021 • edited Loading

geoand commented Mar 12, 2021

geoand commented Mar 12, 2021

phillip-kruger commented Mar 12, 2021

geoand commented Mar 12, 2021

phillip-kruger commented Mar 16, 2021

geoand commented Feb 24, 2021 •

edited

Loading

geoand commented Feb 24, 2021 •

edited

Loading

maxandersen commented Feb 25, 2021 •

edited

Loading

geoand commented Feb 25, 2021 •

edited

Loading

geoand commented Feb 27, 2021 •

edited

Loading

geoand commented Mar 11, 2021 •

edited

Loading

maxandersen Mar 12, 2021 •

edited

Loading

geoand commented Mar 12, 2021 •

edited

Loading

geoand commented Mar 12, 2021 •

edited

Loading