Page MenuHomePhabricator

Convert SyntaxHighlight_Geshi from Geshi to Pygments (was: Don't register 250+ modules)
Closed, ResolvedPublic

Description

This is overkill. Unfortunately, there is no clear solution at hand without changing the way the extension itself work. It is a symptom of a wider flawed design of Geshi. There should be no need for each language to have its own stylesheet.

Switch to Pygments:

Details

Related Gerrit Patches:
mediawiki/extensions/SyntaxHighlight_GeSHi : masterAvoid displaying double borders for inline code snippets
mediawiki/extensions/SyntaxHighlight_GeSHi : masterAdd 'direction: ltr;' to .mw-highlight
mediawiki/extensions/SyntaxHighlight_GeSHi : masterCorrect whitespace around <syntaxhighlight> blocks
mediawiki/extensions/SyntaxHighlight_GeSHi : masterUnbreak <syntaxhighlight enclose="none">
mediawiki/extensions/SyntaxHighlight_GeSHi : masterCorrect behavior of <syntaxhighlight highlight="[lines]"> with more than one line
mediawiki/extensions/SyntaxHighlight_GeSHi : masterHighlight using Pygments rather than Geshi

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
matmarex updated the task description. (Show Details)Jan 5 2015, 3:03 PM
Ltrlg added a subscriber: Ltrlg.May 31 2015, 11:14 PM

Change 217899 had a related patch set uploaded (by Ori.livneh):
Rewrite to use Pygments instead of GeSHi

https://gerrit.wikimedia.org/r/217899

Change 217899 abandoned by Ori.livneh:
Rewrite to use Pygments instead of GeSHi

Reason:
OK, doing this in SyntaxHighlight_GeSHi instead. See I07446ec98

https://gerrit.wikimedia.org/r/217899

Change 218584 had a related patch set uploaded (by Legoktm):
Highlight using Pygments rather than Geshi

https://gerrit.wikimedia.org/r/218584

This comment was removed by Krinkle.

Change 218584 merged by jenkins-bot:
Highlight using Pygments rather than Geshi

https://gerrit.wikimedia.org/r/218584

Krinkle renamed this task from SyntaxHighlight_Geshi extension should not register 250+ modules for stylesheets to Convert SyntaxHighlight_Geshi from Geshi to Pygments (was: Don't register 250+ modules).Jun 22 2015, 11:11 PM
Krinkle updated the task description. (Show Details)Jun 22 2015, 11:14 PM

In https://gerrit.wikimedia.org/r/217899, @Legoktm asked why not rename the extension to "SyntaxHighlight"? It looks like a decision was made to keep the extension name "SyntaxHighlight_GeSHi" but use Pygments... why?

In https://gerrit.wikimedia.org/r/217899, @Legoktm asked why not rename the extension to "SyntaxHighlight"? It looks like a decision was made to keep the extension name "SyntaxHighlight_GeSHi" but use Pygments... why?

If we were going to be creating a separate repository, I preferred using a generic name like "SyntaxHighlight". But using the existing repository is even better IMO, while it is technically wrong, it means there is a lot less shuffling around, updating paths, etc.

Hi, it's fine with me to move towards a system with upstream maintenance alive.

I don't know the current state of implementation here, but I noticed that it has already been marked for 1.26wmf11 deployment. I am afraid some work needs to be done before, otherwise wiki arcticles will be broken:

  • enclose not yet working
  • highlight= lines not yet highlighted
  • style= not yet in effect, class= id= ready?

Please compare

Just some HTML juggling. Enjoy.

TheDJ added a subscriber: TheDJ.Jun 23 2015, 10:13 AM

also

  • why are we dumping mw-code class ?
  • shouldn't we set dir and mw-content-ltr ? I think we would currently be breaking code fragments in rtl wiki's this way.
onei added a subscriber: onei.Jun 23 2015, 10:14 AM

chechil is dead :(

I don't know the current state of implementation here, but I noticed that it has already been marked for 1.26wmf11 deployment. I am afraid some work needs to be done before, otherwise wiki arcticles will be broken:

  • enclose not yet working
  • highlight= lines not yet highlighted
  • style= not yet in effect, class= id= ready?

I was scared for a second there, but it seems that both enclose and highlight are just bugs that should be easy to fix (and not forgotten features), and additional HTML attributes should be easy to handle too. Thanks for pointing this out, they would have probably gone unnoticed otherwise. I'll submit a patch soon.

Change 220135 had a related patch set uploaded (by Bartosz Dziewoński):
Correct behavior of <syntaxhighlight highlight="[lines]"> with more than one line

https://gerrit.wikimedia.org/r/220135

When investigating the above I also discovered (and fixed) an upstream issue, which luckily will not be affecting us after the above bugfix: https://github.com/kzykhys/Pygments.php/issues/4

Change 220137 had a related patch set uploaded (by Bartosz Dziewoński):
Unbreak <syntaxhighlight enclose="none">

https://gerrit.wikimedia.org/r/220137

Change 220148 had a related patch set uploaded (by Bartosz Dziewoński):
Correct whitespace around <syntaxhighlight> blocks

https://gerrit.wikimedia.org/r/220148

Change 220135 merged by jenkins-bot:
Correct behavior of <syntaxhighlight highlight="[lines]"> with more than one line

https://gerrit.wikimedia.org/r/220135

Change 220137 merged by jenkins-bot:
Unbreak <syntaxhighlight enclose="none">

https://gerrit.wikimedia.org/r/220137

matmarex added a comment.EditedJun 23 2015, 3:24 PM

@PerfektesChaos, enclose="none" and more complicated highlight syntaxes will work correctly now. However, additional HTML attributes on the <syntaxhighlight> tag (id, class, style, etc.) will not yet, there seems to be no easy way to make Pygments render them. I don't think this is important enough to block the deployment? enclose=div also doesn't behave as described on https://www.mediawiki.org/wiki/Extension:SyntaxHighlight_GeSHi#Parameters, which is also probably not terribly important.

Change 220148 merged by jenkins-bot:
Correct whitespace around <syntaxhighlight> blocks

https://gerrit.wikimedia.org/r/220148

shouldn't we set dir and mw-content-ltr ? I think we would currently be breaking code fragments in rtl wiki's this way.

We should, but Pygments makes it difficult (same issue as with additional HTML attributes above). We should try filing this upstream, but in the meantime, setting direction: ltr in CSS should do the trick probably?

Change 220159 had a related patch set uploaded (by Bartosz Dziewoński):
Add 'direction: ltr;' to .mw-highlight

https://gerrit.wikimedia.org/r/220159

Thank you for quick reaction.

I don’t think enclose="div" needs further support. If I recall correctly, this has been a workaround in early 2010s, when moving forth and back from <pre> and <div>. Two major modes are interesting:

  1. block mode (default)
  2. inline mode, triggered by enclose="none"

The style= issue is not blocking, nor does it break pages entirely, but should be completed quite soon, since pages might rely on features like:

  • margin-left: (rather often and obvious in block mode, within list item indentation) as well as
  • white-space:nowrap in enclose="none" context (should be default, btw)
  • and more decorative presentation like border: background: margin-top: margin-bottom: padding:.

Missing class could drop some general styling and id is supposed to provide a URL fragment, addressing particular code examples.

If the tag cannot be equipped, perhaps the output could be wrapped into <div> and <span> respectively, until upstream moves – which has been the rationale for changing the package?

I've used enclose="div" in indented blocks sometimes.

Change 220159 merged by jenkins-bot:
Add 'direction: ltr;' to .mw-highlight

https://gerrit.wikimedia.org/r/220159

Jdforrester-WMF closed this task as Resolved.Jun 24 2015, 10:37 PM
Jdforrester-WMF claimed this task.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Looks to be done?

The new palette for Lua looks ugly. Please restore the previous one.

The new palette for Lua looks ugly. Please restore the previous one.

It looks nice enough for me… but I'm sure we can change it. We're using the default one right now. Pygments supports a few dozen "styles", we could easily switch to a different one (by changing the $pygments->getCss( 'default', … ) call in maintenance/updateCSS.php).

They have a nice demo where you can highlight any code with any style, here's an example: http://pygments.org/demo/1469027/?style=default – if you find any that looks a lot better, then let's use that one, sure. (But bear in mind that the same style is used for all languages that can be highlighted.)

We could even implement a custom style if we want to be special (http://pygments.org/docs/styles/), but that would require some work.

Change 220744 had a related patch set uploaded (by Bartosz Dziewoński):
Avoid displaying double borders for inline code snippets

https://gerrit.wikimedia.org/r/220744

Change 220744 merged by jenkins-bot:
Avoid displaying double borders for inline code snippets

https://gerrit.wikimedia.org/r/220744

(Further regression fixing is tracked on T103780 and T103964.)