astoff / devdocs.el

Emacs viewer for DevDocs
289 stars 17 forks source link

Fontify pre tags #1

Closed kljohann closed 3 years ago

kljohann commented 3 years ago

Thanks for the nice package! I added a simple implementation for the fontification of code blocks. Thanks to the consistent structure of the devdocs.io documents, it seems to be sufficient (devdocs.io uses pre tags with data-language attributes to annotate code blocks that should be syntax highlighted). I tested it with both C++ and Python documentation.

org-src-font-lock-fontify-block is included with Emacs 27.1, which is already listed as a requirement, so that should also be fine.

image

So far I did not include an option to toggle this feature; please point out any improvements you want me to make (if you decide to include sth. like this).

astoff commented 3 years ago

Thanks for the suggestion and pull request. I was afraid it was going to come sooner or later.

So, I'm a bit reluctant to add this feature, because not all code examples are valid code. For instance, in the Python manual lots of examples include the REPL prompt >>>. The syntax highlighting displayed here is completely bogus, isn't it? How does this example look like with your code? How about other corner cases, of which there must be plenty?

On the other hand, it seems quite clear that lots of people would enjoy this feature. So let me think what to do here.

Lastly, you may have noticed that this package in on GNU ELPA. This means you would need to sign the FSF copyright assignment paperwork before I could merge your code. Is this possible for you?

kljohann commented 3 years ago

So far I have not seen cases where the fontification does not work, but I can keep using it for some time and report back. W.r.t. your specific example, devdocs.io uses a dedicated data-language value for interactive/"console" Python code (pycon or pycon3, which I map to python since fontification still seems to work fine). Also, there are shorter snippets (code tags), which aren't highlighted at all. Still, it would probably be a good idea to make it possible to disable fontification for certain languages (e.g., via the introduced defcustom). image Thanks for pointing out that the package is on ELPA, I missed that. I'll have to look into the copyright assignment.

astoff commented 3 years ago

In this second screenshot, some strings are green, some are yellow. Why?

kljohann commented 3 years ago

The green strings are font-lock-doc-face instead of font-lock-string-face. I still consider this good enough / better than nothing, but maybe the default settings should be made stricter (by removing the pycon{3,} → python mapping).

astoff commented 3 years ago

I still consider this good enough / better than nothing

Fine. This is as good syntax highlighting gets, I guess.

So, I noticed that org-src already has a defcustom mapping language names to major mode symbols, namely org-src-lang-modes. This means we don't need to maintain a list of modes in this package.

As a beneficial side-effect, by removing devdocs-language-normalization-alist you can make the PR below 15 lines, which means that the FSF paperwork can be waived. (Note that this total is cumulative. Is this your first contribution to Emacs or a GNU ELPA package?)

kljohann commented 3 years ago

Thanks for the review, I just made the changes you suggested, which pushes the PR below the 15 lines limit. I have not made contributions to Emacs (apart from bug reports) and the packages for which I sent patches weren't part of ELPA (as far as I know — at least I haven't been asked to do a FSF copyright assignment until now).

astoff commented 3 years ago

All right, I merged your patch in 57689994a0008c8d2ee45e34f832bfe12bebb202. I slightly edited the commit message, and Github can't deal with that, so I it seems have to "close" your PR...

There are still some glitches to investigate; for instance, Lua doesn't work at al for me, and in Java the fontification is sometimes incomplete. Compare

image

with the same snippet as an Org code block:

image

But I'm afraid you have exhausted your 15 lines :-). I encourage you to get that paperwork done, if you would like to send more patches (which would be very welcome!).

kljohann commented 3 years ago

Thanks, it's certainly a fun (and quite useful) package to hack on, so I'll look into the paperwork. :) (And I'll also play around with the other languages, now that you mention these issues.)

astoff commented 3 years ago

Lua seems fine (I just didn't have the mode installed). I think I solved the issue I had in Java, let me know if get the chance to test.