platformer / typst-algorithms

MIT License
124 stars 4 forks source link

More flexible styling #14

Open ludwig-austermann opened 7 months ago

ludwig-austermann commented 7 months ago

Thank you for your package — I like how convenient and non-bloated it is to write code with it.

Right now, however, I noticed that one can only style keywords to be bold or not. Now one could say, that similar to other styling mechanisms in this package, one could instead give text parameters.

However, this is quite inflexible. For instance, what if I wanted every keyword to be highlighted with highlight or to be boxed or underlined. This would not be possible with the current design. If we instead allowed functions mapping content to content, we would allow all these cases. It would behave similar to the following

#let styled(b, s: x => x) = s(b)

#styled[hello world]

#styled(s: strong)[hello world]

#styled(s: box.with(stroke: black))[hello world]

returning grafik

If you agree on this change, then I could work on this (while also changing the other styles arguments) and make a PR.

platformer commented 7 months ago

I've actually considered doing this myself before, but there were some concerns preventing me from implementing it.

With regards to keywords, the highlighting is achieved with a regex show-rule that is applied to each line of the main algorithm text. Because this can lead to false positives, I added a no-emph function that escapes its internal content from being bolded. no-emph is also implicitly called on inline comments. I can't think of a way to generalize this functionality when keywords can be subject to any arbitrary transformation. I believe it's within Typst's roadmap to add revokable show rules, which would be a straightforward solution for this use case. Without it, I'm not sure we can allow a styling function for keywords without breaking functionality. If you can think of a more flexible way to do keyword styling, I'd be happy to hear it.

With regards to main-text-styles, line-number-styles, and comment-styles, this proposal is already possible to implement. However, the code is a bit of a mess of measure calls and argument drilling just for the sake of implementing indent guides. When I first published the package, I tried my best to compartmentalize the code, but you may or may not still have to change multiple areas of the code to implement this change. I was hesitant to do this myself because the eventual integration of tablex into native Typst would make indent guides infinitely easier and more obvious to implement (we just need arbitrary cell borders). I'd still be willing to offer help and guidance to implement this part of your proposal, however.

Separate from this issue, I also plan to entirely rewrite the package whenever Typst finally implements its types/content/context/styling reworks. If we decide not to act on this issue right now, I'll definitely get to it during my rewrite.

ludwig-austermann commented 7 months ago

You can have a look at fork (commit 32695f9) which makes generation of grafik possible with just a few tweaks.

I also added the functionality to disable the default strong styling with false, which could also be changed to none or something else.

I am also very much waiting for proper types and other future features to be implemented in typst, which is why I am also hesitant to change quite a few packages/scripts of my own. So I understand if you want to wait until this functionality is made clean and idiomatically. That way, many in between breaking changes can be avoided. However, if you decide this is the right direction for now, I can clean up my code and rewrite also the other parts as describe before.

platformer commented 7 months ago

Currently, it runs into the no-emph issue I was referring to:

image

Notice that instances of keywords in the inline comment are also displayed in blue text. The current scheme avoids this by simply disabling strong text in comments:

#let no-emph(body) = {
  ...
  set strong(delta: 0)
  body
}

#let comment(
  body,
  inline: false,
) = {
  ...

  if inline {
    _algo-comment-prefix.display(comment-prefix => {
      _algo-comment-styles.display(comment-styles => {
        set text(..comment-styles)
        comment-prefix
        no-emph(body)
      })
    })
  } else {
    ...
  }
}

To allow the user to style keywords arbitrarily, we would need to generalize no-emph to handle any arbitrary transformation on the content, or we would need to restructure the way keywords are detected and styled in the first place. The former can be done with minimal changes if/when Typst adds revokable show rules. The latter might be possible, but I suspect it would require a more complex approach than a simple regex show-rule like what is done now.

ludwig-austermann commented 7 months ago

Ah okay, I see.

But then, I think, one can solve the problem in _get-algo-lines, by either:

The second case's 'side effect' matches that of the current algo version, but I would personally say, that the first one is the way to go.

However, it is you to decide. Meanwhile, I will look into if, what I was saying, even works.

ludwig-austermann commented 7 months ago

okay, I created a branch case1 for the first case, and it seems to work as described. grafik

I also did some benchmarking, and as suspected, it was slightly better. (On my machine, example.typ reduced from 622.6 ms ± 31.3 ms to 602.2 ms ± 18.7 ms)

ludwig-austermann commented 7 months ago

Btw, both cases should be much easier to handle, as soon as the 'Type and content rework' from typst is done — especially for the second case.

platformer commented 7 months ago

I just had a look, and I think the approach is reasonable (I wish I had thought of it sooner haha). I think it's fine if the user is unable to add additional styling to certain keywords, as all keywords should probably look the same anyway. I'll maybe look back on this when revokable show rules come in.

You're free to move forward with a PR. Just some other points to consider:

ludwig-austermann commented 7 months ago

Ok perfect. One should probably document, that end users can save the transformation in a function and use that everywhere within the code.

I'll look into all mentioned points.

ludwig-austermann commented 7 months ago

Btw, I noticed, that the multi-word keyword implementation is wrong, as ':' for instance will not be matched, as the word boundaries are then messed up.

I am now trying to figure out how to deal properly with this. One idea is to have a regex like '\b(?:let|if|such that)\b|:', hence additionally adding non-word keywords.

platformer commented 7 months ago

Good catch, it's unfortunate I let that slip through. The main thing I'm trying to prevent is false positive matches against substrings (e.g. 'in' in 'interpolate'), since it's something that'll come up for users that prefer to write algorithms in "plain english".

Similar to your idea, it might make sense to match against a word boundary only where the keyword ends with a word character. So if the keywords were ("abc", "1abc", "abc1", ":"), then the regex to detect them would be "\babc\b|1abc\b|\babc1|:". What do you think?