Skip to content

Overhaul code highlighting #278

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

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Conversation

Totto16
Copy link
Collaborator

@Totto16 Totto16 commented Apr 20, 2024

Overhaul code highlighting

Description

As described in #271 code highlighting should become optional, so that user need to install an additional package or something similar to make it work.

There are a few options, I'll test all of them and list benefits from them and shortcomings

Overview

We need an option that can do these things:

  • detect languages, say how "confident" (highlight.js calls it relevance) the guess is, based on content only (no file extension or similar things, or shebangs)
  • format languages to Pango markup or something similar, that we can convert to it
  • provide custom styles or use a set of predefined styles or be easy to parse / and or add custom styles to it
  • support a few languages, not just 1,2

Our options are:

  • 1: Use a command-line highlighter, that comes with many distributions, it can be installed and is then detected by Pano, to use it
  • 2: use a JavaScript package as before, the user needs to install files into a correct location, and then Pano can detect it.
  • 3: make a custom deb / rpm / arch etc. package with Pano and built-in highlighting, that installs the needed package automatically

1. Option

Pros:

  • the packages are quite robust and have many languages
  • it is easy to install for the enduser.

Cons:

  • we need to "shell out" e.g. spawn a shell process, that is more complex and error-prone than other methods

Feature Table

package name detection many languages markdown output styleable
source-highlight ✔️ ⚠️ ⚠️
pygments ⚠️ ✔️ ✔️ ✔️

Notes

source-highlight:

It can not detect languages without filename or shebang, it also can't tell you, how confident it was (there is a option to infer the language, but it only works on file extensions / shebangs or similar metadata)
It can't output to markdown, but to simple html, which would be styleable in some way
You can specify custom css or similar things, but it's not that easy, but maybe doable.

pygments(python3-pygments)

It is good at detecting creating languages (like python) but when I tried JS, or HTML or some other ones it failed ... i didn't test 100 files, but it wasn't that good, the one good thing was, that it's better than highlight.js in many cases and the detected languages all seem to look similar, when highlighted.
It has support for many many languages, it is easy styleable and can output pango markdown out of the box.

update: after some modification to the detection logic, it seems really stable and robust when working with more than ~ +- 200 characters. The shortcoming of it is, that it has no real relevance value, but we can emulate that, by using that charaacter length. After that it is really good.

2. Option

Pros:

  • it is easy to integrate
  • it has many options
  • many highlighter have many languages and are quite good

Cons:

  • If we don't ship them, it's not easy for the end-user to install

3. Option

Pros:

  • We can pre package everything we need, the cons of the 2. Option or the previous implementation go away
  • We can select option 1 or 2 as core implementation

Cons:

  • We would stop publishing on EGO or the package published there is not complete and maybe, depending on the use option (1 or 2) it might be not easy to add the code highlighting to Pano again by hand

Current state

I would focus on option 1, since it's the best choice atm. I tested source-highlight but it's quite bad. So I'll be using pygments(python3-pygments) for now.

TODOs:

  • fix all bugs with the new implementation
  • fix slider (Gtk4.Scale) Size in Settings
    grafik
  • more testing
  • fix search of styles in settings

optional:

  • Improve pano theme for pygments

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • My commits follow the commit standards of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have checked my code and corrected any misspellings

@Totto16
Copy link
Collaborator Author

Totto16 commented Apr 20, 2024

@oae Do you have an auto branch protection rule in some way, if the branch isn't named feat/ (is that even possible)
Since I am currently unable to push (it fails with the reason, that this is a protected branch ❓)
grafik

@oae
Copy link
Owner

oae commented Apr 20, 2024

The first option with pygments sounds good to me too

@oae
Copy link
Owner

oae commented Apr 20, 2024

@oae Do you have an auto branch protection rule in some way, if the branch isn't named feat/ (is that even possible) Since I am currently unable to push (it fails with the reason, that this is a protected branch ❓) grafik

I have changed some settings now. Can you try again?

@Totto16
Copy link
Collaborator Author

Totto16 commented Apr 20, 2024

The first option with pygments sounds good to me too

@oae Do you have an auto branch protection rule in some way, if the branch isn't named feat/ (is that even possible) Since I am currently unable to push (it fails with the reason, that this is a protected branch ❓) grafik

I have changed some settings now. Can you try again?

Same error, strange, it seems, that I can't push to any branch on this repo, but I could earlier this day 🤔

Edit: indeed, it is fixed ❤️

@oae
Copy link
Owner

oae commented Apr 20, 2024

I think it should be fixed now

@Totto16 Totto16 force-pushed the overhaul-code-highlighting branch from 3c71dc7 to 141b7d7 Compare April 20, 2024 23:53
- use interface that permits multiple highlighters in the future
@Totto16 Totto16 force-pushed the overhaul-code-highlighting branch from 925b926 to 3001330 Compare April 21, 2024 13:20
Totto16 added 17 commits April 21, 2024 21:04
- fix many settings errors, so that they behave correctly
- use setter of `sensitive` instead of `set_sensitive`
- correctly connect settings and the core `_markdownDetector` of the extension
- display code items as text items, if there is no code highlighter, or he is disabled
- change paths, that now require async routes in the code flow
- add LoadingPanoItem, that shows up, before an item is loaded
…another way:

 make an not highlighted version of the code item, and after the highlighting returns, set the correct markdown
 - type metaData of all items explicitly with new TS wrapper methods to JSOn.parse and JSON.stringify
- always catch errors
- add load status to PangoMarkdown
- add mor logger types and use them
- add a slider(Gtk4.Scale) to set the relevance threshold for the highlighter
- fix Promisified types
- add some more inline comments about (not needed atm) not implemented features
- chnage CodeItem handling of later initialization
…nous scanning

- fix createDropdownRowForHighlighter, it had some errors in using some outer scope variables incorrectly
- fix scale issue, when resetting
- fix small issue in panoItemFactor
- refactor how teh style in codePanoItem is handled, it now also listens to text events and can mimic text style, only if it's set to code it display the code
Totto16 added 2 commits May 5, 2024 19:09
- completely reinstall the dependencies, to get newer sub-dependencies
@Totto16 Totto16 mentioned this pull request May 8, 2024
9 tasks
@Totto16 Totto16 added the Priority High Priority High label May 8, 2024
@HearseDev
Copy link

HearseDev commented May 31, 2024

Not really experienced with anything gnome, but was wondering if this would be a viable and much cleaner solution. It could be an optional lib install and also has bindings for js. Or it could be required and could be used as a default text view?(think it might be preinstalled with gnome, not too sure) I am just throwing out an idea, never really worked with anything remotely close to this.
https://wiki.gnome.org/Projects/GtkSourceView
https://gnome.pages.gitlab.gnome.org/gtksourceview/gtksourceview5/class.PrintCompositor.html

@Totto16
Copy link
Collaborator Author

Totto16 commented May 31, 2024

Not really experienced with anything gnome, but was wondering if this would be a viable and much cleaner solution. It could be an optional lib install and also has bindings for js. Or it could be required and could be used as a default text view?(think it might be preinstalled with gnome, not too sure) I am just throwing out an idea, never really worked with anything remotely close to this. https://wiki.gnome.org/Projects/GtkSourceView https://gnome.pages.gitlab.gnome.org/gtksourceview/gtksourceview5/class.PrintCompositor.html

I think I saw this in my research, but It isn't half as good as pygments in autodetecting languages and it's ballpark is nearly the same as source-highlight.

It has integrated GTK support, but it's not themeable (at least easily) and it has no "real" autodetection feature, you have to create a widget to get the language, which is not suitable for our purpose 😓

@PiotrGrobelak
Copy link

Hi @oae and @Totto16, what can I pick up to help you with for the full release of GNOME 46?

@Malix-Labs
Copy link

Malix-Labs commented Nov 3, 2024

I don't know if it's relevant and quite frankly it could really totally not be, but have you thought about using Deno instead of Node ?

It would help drastically reduce the number of dependencies required by EGO

I don't know if Pano need to ship a whole runtime or not, and haven't developed a GNOME extension myself, I apologize if it was a dumb msg

@Totto16
Copy link
Collaborator Author

Totto16 commented Nov 3, 2024

I don't know if it's relevant, but have you thought about using Deno instead of Node ?

It would help drastically reduce the number of dependencies required by EGO

We don't use node.js, this is a gnome extension, so it has to use GJS

@Malix-Labs
Copy link

Thanks for the clarification

Also, have you noticed the kind support offered by @PiotrGrobelak : #278 (comment) ?

@Totto16
Copy link
Collaborator Author

Totto16 commented Nov 3, 2024

Thanks for the clarification

Also, have you noticed the kind support offered by @PiotrGrobelak : #278 (comment) ?

I did, but this I rather complicated and I have to clean a few things up 😓
It is not easy to help here, as this is a complicated thing to do, since it requires some really complex things to do, where you have to be familiar with many things regarding pano and how they work internally 😓

@Malix-Labs
Copy link

Understandable

So only you @Totto16 and @oae could ship this PR and make this extension mainstream again ?

@teohhanhui
Copy link

I think option 2 is viable.

We'll just have to instruct the user to do something like:

sudo dnf install nodejs
cd ~/.local/share/[email protected]
npm install highlight.js@^11.9.0

@Malix-Labs
Copy link

Malix-Labs commented Nov 3, 2024

I strongly advocate against using nodejs as a dependency, which would mean Pano would rely on a different runtime that the one shipped with GNOME (GJS, apparently)

Also, it basically would mean to rely on a third party (APT, RPM, nixpkgs, etc…) third party (npm) package manager

Dropping support for highlighting for now seems like a way more acceptable option

@teohhanhui
Copy link

teohhanhui commented Nov 3, 2024

I strongly advocate against using nodejs as a dependency, which would mean Pano would rely on a different runtime that the one shipped with GNOME (GJS, apparently)

That's not the case at all. It's just a way for the user to install the package from npm.

Also, it basically would mean to rely on a third party (APT, RPM, nixpkgs, etc…) third party (npm) package manager

Dropping support for highlighting for now seems like a way more acceptable option

In case it wasn't clear, this should be entirely optional.

@Malix-Labs
Copy link

Malix-Labs commented Nov 3, 2024

The optional part would be understandable indeed, but it would probably be simpler (both for the end-user and for the docs team) to ship a non-EGO release with dependencies for the added features instead

A rewrite using something else than highlight would be optimal, for the future (but would take time)

Shipping a non-system-agnostic DE extension, even optional, seems like a burden to use and maintain

It already partially is the case with libgda and libgda-sqlite dependencies though

@teohhanhui
Copy link

A rewrite using something else than highlight would be optimal, for the future (but would take time)

Honestly I don't understand this position. It seems to me that the problem is EGO's review process. But it is how it is.

@Malix-Labs
Copy link

Malix-Labs commented Nov 3, 2024

The problem is specifically losing support for EGO which transitively means losing an integrated GNOME experience (install, update, etc)

@teohhanhui
Copy link

teohhanhui commented Nov 3, 2024

The problem is specifically losing support for EGO which transitively means losing an integrated GNOME experience (install, update, etc)

Absolutely agreed. Having an optional dependency shouldn't run into that problem though. And also Pano currently already requires the user to manually install many dependencies.

However

but it would probably be simpler (both for the end-user and for the docs team) to ship a non-EGO release with dependencies for the added features instead

This suffers from the problem of no auto updates as well. 😞

@Malix-Labs
Copy link

many dependencies

To my knowledge, only libgda and libgda-sqlite, which are sometimes already shipped by default in some Linux distributions, and are both combined nowhere close to node + highlightjs dependency bundle size

This suffers from the problem of no auto updates as well. 😞

This is correct, yep

Don't you think that, for the long run, it would be preferable to rewrite to a more system-agnostic + EGO compliant extension ?

In the meantime, documenting to add node and highlights as optional dependencies (with a size warning) could be a viable option ; The other being dropping total support for highlighting

@teohhanhui
Copy link

teohhanhui commented Nov 3, 2024

Don't you think that, for the long run, it would be preferable to rewrite to a more system-agnostic + EGO compliant extension ?

Option 1 also does not do that. The user would have to manually install another system dependency (pygments) through their package manager.

I don't think there are any realistic good options with the current EGO stance...

(Personally I don't care about code highlighting. I don't even use the URL preview because it's a major security and privacy risk.)

@teohhanhui
Copy link

teohhanhui commented Nov 29, 2024

For option 2, there's also tree-sitter highlight: https://tree-sitter.github.io/tree-sitter/syntax-highlighting

But I guess it's not practical for the use of this extension...

@Totto16 Totto16 mentioned this pull request Feb 20, 2025
8 tasks
@ArthurValada
Copy link

Correct me if I'm wrong, but the EGO Guideline refers to entire applications, in order to facilitate the review process and not add unnecessary tools for the user. Would a library for accessing information available on the Internet with the user's authorisation fall under this heading?

Highlight being disabled by default is the right way to go, but for the user to enable it manually would add a complicated step for many. In order to maintain the structure of the project, wouldn't it be prudent to add a more modular character to highlight - prismjs too, if appropriate - and add the package pacote to the pano dependencies and through it, with the user's authorisation, download and enable highlight?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority High Priority High
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants