Skip to content

Conversation

@rumpelsepp
Copy link
Contributor

A few day ago I have opened this issue here #8404. I am neither a ruby nor a rails pro but I have managed to create some kind of demo. It is not ready for merge. You could consider it as a proof of concept and for comparing the results between highlight.js and rouge

Again some explanations

  • highlight.js is really buggy in getting the correct synthax. The solution is guessing the language with taking the filename, shebang, ... into consideration.
  • pygments handles this very well but it is a python library
  • rouge is a highlighter in ruby which is compatible to pygments themes. It is able to do this kind of guessing, too.
  • f4b9a65 reactivates highlight.js language autodetection which is buggy by design.
  • look here for some more discussion: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/232#note_622601, #8404

TODO

  • Blob
  • Blame
  • Wiki
  • Code Blocks
  • Snippets
  • Themes - find a better white and dark theme!
  • Check color definitions in themes again
  • Code Cleanup
  • Line number links
  • Line number colors
  • add new theme screenshots

Demo

Checkout my private Gitlab instance. I have included this pull request there already:
https://gitlab.sevenbyte.org/u/stefan

Instructions for changing the theme without an account are here: #8425 (comment)

@TeatroIO
Copy link

TeatroIO commented Dec 3, 2014

I've prepared a stage. Click to open.

@dzaporozhets
Copy link
Contributor

Cool. Anyone else willing to help with this PR? I would like too but I am busy this week.

Copy link

Choose a reason for hiding this comment

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

Keep in mind that this can throw Rouge::Lexer::AmbiguousGuess in some edge cases. It's unlikely since you've passed both the filename and the source (and if it does it's likely I'll consider it a bug in rouge), but it's something you may want to handle and report.

Copy link

Choose a reason for hiding this comment

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

You can also call Rouge::Lexer.guesses with the same arguments and it will return a list (possibly empty) of lexer classes, so you could pick one and default to Rouge::Lexers::Text.

@jneen
Copy link

jneen commented Dec 5, 2014

👍 awesome! Let me know if you have any questions about rouge, I'm happy to help.

@jvanbaarsen jvanbaarsen changed the title DO NOT MERGE: Replace highlight.js with rouge gem [wip] Replace highlight.js with rouge gem Dec 13, 2014
@jvanbaarsen
Copy link
Contributor

@rumpelsepp Are there certain places you need help with?

@rumpelsepp
Copy link
Contributor Author

I had a surgery in hospital this week. So unfortunately this pull request will take some more time as I want to fully recover first...

But to answer your question a few thoughts:

  • How should we deal with the themes? We'll have to create some css files I think...
  • It would be helpful if somebody has a pointer where the rendering of comments is to be handled. Rouge has to be included there for handling code blocks as well.
  • @jneen Is it possible to create line numbers with anchors in the html formatter like pygments does? (http://pygments.org/docs/formatters/#HtmlFormatter have a look at lineos and lineanchors)

Without looking at my code these are the major problems I remember. Sorry for the delay... :)

@jvanbaarsen
Copy link
Contributor

Ok! No problem, hope it was not something serious? Have a good recovery!

On Sat, Dec 13, 2014 at 11:03 PM, Stefan Tatschner
notifications@github.com wrote:

I had a surgery in hospital this week. So unfortunately this pull request will take some more time as I want to fully recover first...
But to answer your question a few thoughts:

  • How should we deal with the themes? We'll have to create some css files I think...
  • It would be helpful if somebody has a pointer where the rendering of comments is to be handled. Rouge has to be included there for handling code blocks as well.
  • @jneen Is it possible to create line numbers with anchors in the html formatter like pygments does? (http://pygments.org/docs/formatters/#HtmlFormatter have a look at lineos and lineanchors)
    Without looking at my code these are the major problems I remember. Sorry for the delay... :)

    Reply to this email directly or view it on GitHub:
    Replace highlight.js with rouge-fork rugments #8425 (comment)

@jneen
Copy link

jneen commented Dec 14, 2014

You should be able to make an HTMLFormatter with the :line_numbers => true option set.

@rumpelsepp
Copy link
Contributor Author

Ok! No problem, hope it was not something serious? Have a good recovery!

No, nothing life-threatening. :) Thanks!

You should be able to make an HTMLFormatter with the :line_numbers => true option set.

As a single line is not wrapped with an html tag with an ID creating anchors might get difficult. Correct me if I'm wrong. Maybe a topic for rouge-ruby/rouge#192?

Choose a reason for hiding this comment

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

Line is too long. [82/80]
Use the new Ruby 1.9 hash syntax.

@rumpelsepp
Copy link
Contributor Author

I have continued my work here

Changelog

  • Theme Support
  • Added solarized light theme as well
  • Code Block Support (omitting a language does not highlight anything!)
  • Blame Support
  • Removed highlight JS

screenshot 2014-12-15 13 08 16
screenshot 2014-12-15 13 08 41
screenshot 2014-12-15 13 08 55
screenshot 2014-12-15 13 09 19
screenshot 2014-12-15 13 09 36
screenshot 2014-12-15 13 10 14
screenshot 2014-12-15 13 10 55

@Razer6
Copy link
Member

Razer6 commented Dec 15, 2014

@rumpelsepp Awesome!

@rumpelsepp
Copy link
Contributor Author

thx. :)

@jvanbaarsen
Copy link
Contributor

Nice! 🍰

@rumpelsepp
Copy link
Contributor Author

:)
@jvanbaarsen Should I make one huge commit or should I split them into several ones when I am ready?

@jvanbaarsen
Copy link
Contributor

If you mean pull-request, just one is good in this case, its a major change, so better to have that in one single PR. If you mean squashing the commits, just keep working on this the way you like it. At the end i'll review it and then decide if its better to squash or leave it like this :)

@rumpelsepp
Copy link
Contributor Author

So. The this pull request should be pretty much complete. There is only one issue left with Rouge's HTML formatter as it is not able to wrap lines with something like <span id="L10" class="line">CODE</span>. So source code anchors do not work yet until we find a solution for this. I don't like hacking something ugly in gitlab for this purpose. cc @jneen

Everything else is ready for testing!

@jneen
Copy link

jneen commented Dec 15, 2014

I don't have time to work on it right now, but I'd be happy to review a design and PR for it.

@jvanbaarsen
Copy link
Contributor

@rumpelsepp Thanks for your work on this :) Can you please make sure the tests are passing?

@rumpelsepp
Copy link
Contributor Author

Can you please make sure the tests are passing?

I have already made sure that all possible tests are passing. These last tests are failing because of the missing line anchors I explained above. So when we are able to make this work again the tests should pass.

@jvanbaarsen
Copy link
Contributor

@rumpelsepp Ok, missed that part :) Thanks for explaining!

@rumpelsepp
Copy link
Contributor Author

I have chosen a better white and dark theme. You can check out this pull request on my private gitlab instance. If you want to see another theme you can e.g. change the css class in your browser inspector. The CSS classes for the available themes are: white, dark, monokai, solarized-dark, solarized-light. The default is white.

bildschirmfoto65

https://gitlab.sevenbyte.org/u/stefan

@jvanbaarsen
Copy link
Contributor

@rumpelsepp I like the dark theme :) Feels a bit like my vim environment :)

@rumpelsepp
Copy link
Contributor Author

thx. :) I updated the screenshots in the profile settings.

bildschirmfoto66

@dzaporozhets
Copy link
Contributor

It looks really good

@dzaporozhets
Copy link
Contributor

You did good job with this PR. I really would like to merge this one but we need to fix Line number links before. We cant merge with red CI and broken feature. Is it possible to do something with it?

@rumpelsepp
Copy link
Contributor Author

Thank you. :)
I think the best solution for the anchors is to include this in Rouge::Formatters::HTMLFormatter here: https://github.com/jneen/rouge/blob/master/lib/rouge/formatters/html.rb

So I think here is some work left... :(

@dzaporozhets
Copy link
Contributor

Please ping me when you get tests green so we can merge it :)

I did this commit in an earlier revision of my pull request. As
reverting this commit later caused failing tests I decided to
include it again.
I decided to create a fork of rouge as rouge lacks a HTML formatter with
the required options such as wrapping a line with <span> tags.
Furthermore I was not really convinced about the clarity of rouge's
source code.

Rugments 1.0.0beta3 for now only includes some basic linting and a new
HTML formatter. Everything else should behave the same.
@rumpelsepp rumpelsepp changed the title [wip] Replace highlight.js with rouge gem Replace highlight.js with rouge-fork rugments Jan 15, 2015
@rumpelsepp
Copy link
Contributor Author

The tests pass on my machine and the pull request should be complete for now. I have edited the commit messages and rebased on master. As explained in the commit messages I have created a fork of rouge (rugments) 'cause there are problems with the html formatter. When I release the next version of rugments I will open another pull request which cleans up a bit more stuff. But for now I am quite happy with the results. If there are any further problems please let me know!

What's the matter with semaphoreci? I just edited the commit description and now my tests fail somewhere else?

@rumpelsepp
Copy link
Contributor Author

Very reliable; retriggered the tests, now it's green!

@dzaporozhets
Copy link
Contributor

@rumpelsepp thank you a lot. I wlll merge it after release!

@dzaporozhets dzaporozhets self-assigned this Jan 15, 2015
@dzaporozhets dzaporozhets added this to the 7.8 milestone Jan 15, 2015
@dzaporozhets
Copy link
Contributor

Looks like Semaphore starts new build on every PR change (assignee, label etc) Looks like a bug to me

@Razer6
Copy link
Member

Razer6 commented Jan 15, 2015

@randx Yes they have. I already reported that issue. They are aware of that.

dzaporozhets added a commit that referenced this pull request Jan 15, 2015
Replace highlight.js with rouge-fork rugments
@dzaporozhets dzaporozhets merged commit 892be53 into gitlabhq:master Jan 15, 2015
@dzaporozhets
Copy link
Contributor

@rumpelsepp Thank you for such huge work done!

@rumpelsepp
Copy link
Contributor Author

:)

@rumpelsepp rumpelsepp deleted the rouge branch January 16, 2015 17:17
@jneen
Copy link

jneen commented Feb 1, 2015

Hey @rumpelsepp I just became aware of your fork - do you wanna talk about merging some of that work upstream?

@jneen
Copy link

jneen commented Feb 1, 2015

The HTML formatter is a bit of a mess right now. We're open for suggestions - it's under a lot of pressure to be a lot of different things, so it'll likely be split into several formatters. Additionally, it'd be pretty easy to create a formatter without a fork. Just subclass Rouge::Formatter, tag it, and define #stream.

@rumpelsepp
Copy link
Contributor Author

@jneen Yeah, I absolutely want to merge some of my work back -- when it's ready. I want to play around a bit (it's really great for improving my ruby skills!) and I introduced a few backwards breaking changes (the master branch is quite broken at the moment). So I thought it would be much simpler to do this stuff on my own git tree first rather than wasting your time with reviewing broken stuff. :)

@jneen
Copy link

jneen commented Feb 1, 2015

ok cool :)

@dannykopping
Copy link

Any news on this? Is this in the latest CE edition?

@forki
Copy link

forki commented Apr 11, 2015

Question: @kfuglsang created a new C/AL language formatter in highlight.js which we thought will eventually come to gitlab. Where do we need to send a pull request to get such a highlighter in?

@zertrin
Copy link
Contributor

zertrin commented Apr 11, 2015

@forki highlight.js is not used anymore in Gitlab since this pull request was merged. If you need a C/AL language formatter in Gitlab, see if you can get it in rouge and/or rugments, and get it then updated in Gitlab.

@forki
Copy link

forki commented Apr 11, 2015

Thanks. Could you please point me in the right direction? I'm looking for a
pascal or Delphi highlighter that I can fork and edit to highlight C/AL
On Apr 11, 2015 2:55 PM, "zertrin" notifications@github.com wrote:

@forki https://github.com/forki highlight.js is not used anymore in
Gitlab since this pull request was merged. If you need a C/AL language
formatter in Gitlab, see if you can get it in rouge and/or rugments, and
get it then updated in Gitlab.


Reply to this email directly or view it on GitHub
#8425 (comment).

@jneen
Copy link

jneen commented Apr 11, 2015

Unfortuntately rouge doesn't have pascal or delphi support right now, but i'd merge a new lexer in a heartbeat!

@forki
Copy link

forki commented Apr 11, 2015

Ok now I know why syntax highlighting stopped working for us ;-)
On Apr 11, 2015 6:50 PM, "Jeanine Adkisson" notifications@github.com
wrote:

Unfortuntately rouge doesn't have pascal or delphi support right now, but
i'd merge a new lexer in a heartbeat!


Reply to this email directly or view it on GitHub
#8425 (comment).

@jneen
Copy link

jneen commented Apr 11, 2015

Rouge also has a lexer subclassing thing which makes it pretty easy to maintain similar lexers (C/C++/Objective C, Javascript/QML/ActionScript)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

10 participants