Skip to content

Conversation

vemv
Copy link
Member

@vemv vemv commented Oct 21, 2023

Fixes #3546

Demo:

image

Cheers - V

@vemv vemv requested a review from bbatsov October 21, 2023 20:16
((stringp el) (insert (if cider-inspector-looking-at-java-p
(cider--to-java-string el)
(propertize el 'font-lock-face (if header-p
'font-lock-comment-face
Copy link
Member Author

Choose a reason for hiding this comment

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

Headers now are rendered as comments - else it's all too homogeneous.

@rrudakov
Copy link
Contributor

Starting from emacs-29, build-in java-ts-mode is also available. Maybe this could be customizable, to let users pick the desired mode?

@vemv
Copy link
Member Author

vemv commented Oct 21, 2023

Good eye!

It seems most desirable to simply pick java-ts-mode if available, improving performance I guess (or in any case having color consistency, in case there's a discrepancy between both modes. Assuming that the user uses java-ts-mode regularly).

Would you be able to try this diff locally and confirm that it works well with java-ts-mode?

@rrudakov
Copy link
Contributor

Good eye!

It seems most desirable to simply pick java-ts-mode if available, improving performance I guess (or in any case having color consistency, in case there's a discrepancy between both modes. Assuming that the user uses java-ts-mode regularly).

Would you be able to try this diff locally and confirm that it works well with java-ts-mode?

Sure, I can try it out tomorrow. The only concern I have is that emacs can be built without the tree-sitter support or the grammar can be missing, I guess java-ts-mode won't work properly in this case.

@vemv
Copy link
Member Author

vemv commented Oct 22, 2023

Is that condition detectable?

@rrudakov
Copy link
Contributor

@vemv, yes, it's possible:

(require 'treesit)
(treesit-available-p)                   ;check if Emacs is built with tree-sitter library
(treesit-language-available-p 'java)    ;Make sure Emacs can find the language grammar you want to use

@bbatsov
Copy link
Member

bbatsov commented Oct 22, 2023

Given the simple nature of this highlighting in the inspector, it seems to me that using java-mode unconditionally will be perfectly fine.

I'd suggest a defcustom to enable/disable this, and perhaps a second one about the preferred java-mode to use, if someone feels strongly that using java-ts-mode will result in any meaningful improvements.

(dolist (el elements)
(cider-inspector-render-el* el)))

(defun cider--to-java-string (s)
Copy link
Member

Choose a reason for hiding this comment

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

I think we had a similar function in cider-util.el already that you can reuse (it takes the mode to use as its param).

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed! I got to also kill a similar defun in cider-docstring.el.

@vemv
Copy link
Member Author

vemv commented Oct 23, 2023

@rrudakov although your suggestion was a fair call, it seems best addressed later, and also backed by a specific problem statement (not that I don't like the idea, but we'd like to see some performance or presentational difference first).

My main reasoning is that we now have 5 cider-font-lock-as 'java-mode calls, so it would be a broader refactoring than initially thought.

@vemv vemv merged commit ded24d8 into master Oct 23, 2023
@vemv vemv deleted the 3546 branch October 23, 2023 18:38
@rrudakov
Copy link
Contributor

rrudakov commented Oct 24, 2023 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.

Inspector: (feature request) Syntax highlighting for the java methods

3 participants