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

Allow jumping to java source when the jar is already avaible in classpath. #607

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

markx
Copy link
Contributor

@markx markx commented Sep 15, 2024

This improves and potentially fixes #142. I haven't done thorough testing though.

In my experiment with clojure-emacs/enrich-classpath setup, the info includes javadoc, file and line, then without returning early for javadoc, it can actually jump to java file correctly.

Example log of running def_word on java symbol:

; def (word): Raylib/InitWindow

;;; below is the `info` I printed locally.
{↵  ["annotated-arglists"] = { "^void [^int width, ^int height, ^org.bytedeco.javacpp.BytePointer title]", "^void [^int width, ^int height, ^String title]" },↵  ["arglists-str"] = "[width height title]\n[width height title]",↵  argtypes = { "int", "int", "org.bytedeco.javacpp.BytePointer" },↵  class = "com.raylib.Raylib",↵  column = 1,↵  file = "jar:file:/Users/ke/.m2/repository/uk/co/electronstudio/jaylib/jaylib/5.0.0-0/jaylib-5.0.0-0-sources.jar!/com/raylib/Raylib.java",↵  id = "a4d21c95-e7ef-415b-920e-1e1f961d30a6",↵  javadoc = "com/raylib/Raylib.html#InitWindow(int,int,org.bytedeco.javacpp.BytePointer)",↵  line = 1544,↵  member = "InitWindow",↵  modifiers = "#{:public :static :native}",↵  ["parameter-types"] = { "int", "int", "org.bytedeco.javacpp.BytePointer" },↵  resource = "com/raylib/Raylib.java",↵  returns = "void",↵  session = "dafe2155-2eb4-4ee0-ac4c-919bfde22066",↵  status = {↵    done = true↵  },↵  throws = {},↵  type = "void"↵}


;;; below is the java source file.
; zipfile:///Users/ke/.m2/repository/uk/co/electronstudio/jaylib/jaylib/5.0.0-0/jaylib-5.0.0-0-sources.jar::com/raylib/Raylib.java [1544 1]

I assume there's no good reason to early return for special-form either.

@Olical
Copy link
Owner

Olical commented Sep 17, 2024

Oh this looks like a great change! Thank you very much!

The only issue here isn't your fault, we're in the middle of a project wide refactor over on the main branch (which will soon-ish be the default branch). So what we need to make this change is to have nfnl installed then modify the file in the same way. Once the .fnl is written the nfnl plugin compiles the .lua file automatically. There is no make step anymore. This PR would need retargeting to the main branch as well, I kinda recommend people start swapping over to using main and reporting how it goes on #605 (you can read all about the migration there too)

If you don't want to jump through those hoops I'm happy to make the change on your behalf. I just want to give you the chance to make the actual commit and own it rather than just having a mention from me in a commit 😃

I also worry having nfnl installed will be a barrier to contributors so I might need a script people can run to compile a file without it as well...

@markx
Copy link
Contributor Author

markx commented Sep 18, 2024

Wait... This PR is already targeting main, and I already had nfnl installed when making this change.

@Olical
Copy link
Owner

Olical commented Sep 18, 2024

Ah! My apologies! I had a very long day and thought the makefile change signaled master I think.

I'll get this merged ASAP ☺️ sorry for the false alarm!

@Olical
Copy link
Owner

Olical commented Sep 18, 2024

Hmm trying to test this out locally, do you know which kind of symbols this should help with? Because I think I can't see a difference in behaviour between main and this change right now. Both seem to print the Java source location. I guess I need to find a Java dependency and add it to my classpath for this to work? 🤔

I think I've ben trying to jump to core Java files which just print the URL of the docs online rather than jumping to source. I'm at Heart of Clojure this week so a little busy but will try to get it confirmed and merged.

Thank you for the effort and sorry for not reading the PR carefully enough the first time.

@markx
Copy link
Contributor Author

markx commented Sep 19, 2024

I guess I need to find a Java dependency and add it to my classpath for this to work?

Yes exactly. This can be easily done with clojure-emacs/enrich-classpath.

I think any java symbol should do.

@Olical
Copy link
Owner

Olical commented Sep 20, 2024

So I've tried this out on main and your branch and I get the same output:

; Can't open source, it's Java
; cider/enrich_classpath/Calc72.html

Swapping trains now, will take another look in a sec! Finally on my way back from Heart of Clojure and I want to get to the bottom of this and get it working if I can :) not sure if this change itself changes things for me, but maybe there's something more we can do that will get it working.

@Olical
Copy link
Owner

Olical commented Sep 20, 2024

Because we can only open the Java if it's on disk right? If it's not we just get the doc HTML URL from CIDER I think? I'll use debug mode and inspect some of the messages for what we have available.

Here's the full debug output when looking up this symbol:

; --------------------------------------------------------------------------------
; def (word): Calc72
; debug: send
{:id "04334167-fc2c-4c9e-b86e-9e02d6f55104"
 :ns "dev.sandbox"
 :op "info"
 :session "c230ae99-24d1-4a03-91f5-6463feaa35e6"
 :symbol "Calc72"}
; debug: receive
{:class "cider.enrich_classpath.Calc72"
 :id "04334167-fc2c-4c9e-b86e-9e02d6f55104"
 :interfaces {}
 :javadoc "cider/enrich_classpath/Calc72.html"
 :modifiers "#{:public}"
 :name "Calc72"
 :package "cider.enrich_classpath"
 :session "c230ae99-24d1-4a03-91f5-6463feaa35e6"
 :status ["done"]
 :super "java.lang.Object"}
; Can't open source, it's Java
; cider/enrich_classpath/Calc72.html

So we don't get a Java file path, we do get some HTML but that's not super useful I guess 🤔 we get the same on either branch.

@Olical
Copy link
Owner

Olical commented Sep 20, 2024

OH! I have to somehow use enrich-classpath in order to pull the Java sources that I can then set up Conjure to access. This is harder than I thought and the integration seems a little involved. Still thinking about it though.

@Olical
Copy link
Owner

Olical commented Sep 20, 2024

Okay, I'm going to merge this even though in my setup I have no .java files which are locally accessible. So I can't see it working for me right now, but the change now makes more sense to me. I'm happy with it even if it doesn't work for me but does work for a particular REPL setup 😄 thank you for this, and sorry it took so long to merge, I've been all over the place his week. Mentally and physically!

@Olical Olical merged commit 67ae716 into Olical:main Sep 20, 2024
2 checks passed
@Akeboshiwind
Copy link

Akeboshiwind commented Sep 20, 2024

Would someone mind writing up what steps you need to go through to get this working? 🙏

@Olical
Copy link
Owner

Olical commented Sep 20, 2024 via email

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