Skip to content

GitLab

Merged
Created by Administrator@rootOwner

Factor out Emoji parsing using html-pipeline-gitlab

Created by: Razer6

This PR is one of the first PRs refactoring the markup handling. As discussed with @randx, I'll do this with small PRs, to keep changes detectable.

What it does?

It removes parsing emoji references, such as :+1:, from GitLab and calls instead HTML::Pipeline::GitLab::MarkdownPipeline. This pipeline currently only contains the EmojiFilter.

In future we need to separate the markdown handling, by introducing different pipelines for different context. Eg. for issues/MRs, dashboard...

Loading
Loading
  • Administrator
    Administrator @root ·
    Owner

    Created by: TeatroIO

    I've prepared a stage. Click to open.

    By Administrator on 2014-09-25T12:42:20 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    @Razer6 please add me as an owner to html-pipeline-gitlab gem

    By Administrator on 2014-09-25T12:44:47 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    @randx I fixed the hound issues. In future, all markup will be wrapped in one separate div - as far as I see today, this should not make any difference. Is there any concern for this from you?

    By Administrator on 2014-09-25T23:34:50 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    @Razer6 please add me to owners at http://rubygems.org/gems/html-pipeline-gitlab

    By Administrator on 2014-10-01T16:33:55 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit 390183a4
    lib/gitlab/markdown.rb
    62 65 insert_piece($1)
    63 66 end
    64 67
    68 # Context passed to the markdoqwn pipeline
    69 markdown_context = {
    70 asset_root: File.join(root_url,
    • Administrator
      Administrator @root ·
      Owner

      Created by: dzaporozhets

      does this support GitLab using relative url?

      By Administrator on 2014-10-01T16:36:51 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    Looks good

    By Administrator on 2014-10-01T16:38:02 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit 390183a4
    lib/gitlab/markdown.rb
    62 65 insert_piece($1)
    63 66 end
    64 67
    68 # Context passed to the markdoqwn pipeline
    69 markdown_context = {
    70 asset_root: File.join(root_url,
    • Administrator
      Administrator @root ·
      Owner

      Created by: Razer6

      I just tested this locally with relative url /gitlab - works :)

      By Administrator on 2014-10-01T17:50:43 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    Added you as owner

    By Administrator on 2014-10-01T17:50:59 (imported from GitLab project)

  • Administrator
    Administrator @root started a thread on commit 390183a4
    lib/gitlab/markdown.rb
    62 65 insert_piece($1)
    63 66 end
    64 67
    68 # Context passed to the markdoqwn pipeline
    69 markdown_context = {
    70 asset_root: File.join(root_url,
  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    @randx Awesome!

    By Administrator on 2014-10-02T08:19:45 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: maxlazio

    @Razer6 This change caused a problem, emoji doesn't get rendered correctly. This is the html that gets generated:

    <img class="emoji" title=":+1:" alt=":+1:" src="https://dev.gitlab.org/assets/emoji/%2B1.png" height="20" width="20">

    %2B1.png doesn't exist

    By Administrator on 2014-10-03T08:31:02 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    I'll take a look and make a fix!

    By Administrator on 2014-10-03T08:31:38 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    @maxlazio Does this apply to all emojis, or only to :+1 ? I could think of a problem because assets get precompiled?

    By Administrator on 2014-10-03T08:54:33 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: maxlazio

    @Razer6 It applies to all emojis, looks like because of the asset precompilation. The paths are without the sha:

    /assets/emoji/Aquarius.png or /assets/emoji/new_moon.png, etc

    By Administrator on 2014-10-03T09:22:15 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    Ok. That makes sense. That's the reason it's why https://dev.gitlab.org/assets/emoji/%2B1.png is working in dev mode (asset-pipeline is disabled). I take a look.

    By Administrator on 2014-10-03T09:26:34 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    @maxlazio What's the reason to move emojis to the asset pipeline? I'm just curious.

    By Administrator on 2014-10-03T09:39:02 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    @Razer6 after this commit emoji is broken for production environment. So I must ask you for a fix or revert this PR

    By Administrator on 2014-10-03T10:29:08 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    I'll Provide a fix this evening.

    By Administrator on 2014-10-03T10:31:03 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: dzaporozhets

    @Razer6 thank you

    By Administrator on 2014-10-03T10:31:49 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    @randx @maxlazio I've tested a bit, but did not come to a solution. The problem is, I can't access assets from the asset-pipeline from another gem. However, if we move the filters from the gem to GitLab's codebase it would work. For filters in future, I think it would be better anyway because these filters are tightly closed to GitLab itself.

    Would that be a solution you would accept?

    By Administrator on 2014-10-05T10:29:23 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: Razer6

    Does anyone know, we can use for e.g. image_path (a function from ActionView::Helpers::AssetUrlHelper) in module scope, but not in class scope?

    We can use this helper in Gitlab::Markdown.gfm but I can't use it in a method from Gitlab::Markdown::EmojiFilter.

    That's one mystery to me. Seems some rails magic is behind, but I didn't get it yet 😰

    By Administrator on 2014-10-05T16:10:21 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: mr-vinn

    @Razer6 One option might be to use emoji/something.png as the src attribute in the pipeline filter instead of trying to get the full asset path (I also had to remove the call to ::CGI#escape to make this work):

    diff --git a/lib/html/pipeline/gitlab/gitlab_emoji_filter.rb b/lib/html/pipeline/gitlab/gitlab_emoji_filter.rb
index 384db29..6131e02 100644
--- a/lib/html/pipeline/gitlab/gitlab_emoji_filter.rb
+++ b/lib/html/pipeline/gitlab/gitlab_emoji_filter.rb
@@ -57,7 +57,7 @@ module HTML
         private

         def emoji_url(name)
-          File.join(asset_root, 'emoji', emoji_filename(name))
+          File.join('emoji', emoji_filename(name))
         end

         # Build a regexp that matches all valid :emoji: names.
@@ -74,7 +74,7 @@ module HTML
         end

         def emoji_filename(name)
-          "#{::CGI.escape(name)}.png"
+          "#{name}.png"
         end
       end
     end

    Then you can replace the src attributes in Gitlab::Markdown#gfm before calling #to_html on the output:

    diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index 17512a5..db9c0d5 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -78,6 +78,10 @@ module Gitlab

       result = HTML::Pipeline::Gitlab::MarkdownPipeline.call(text,
                                                              markdown_context)
+      result[:output].css('img').each do |img|
+        img.attribute('src').content = image_path(img.attribute('src'))
+      end
+
       text = result[:output].to_html(save_with: 0)

       allowed_attributes = ActionView::Base.sanitized_allowed_attributes

    This approach feels a little awkward, but at least you don't have to worry about the asset path in the emoji filter.

    By Administrator on 2014-10-08T03:07:05 (imported from GitLab project)

  • Administrator
    Administrator @root ·
    Owner

    Created by: maxlazio

    This has been resolved in the version 0.1.3 of html-pipeline-gitlab gem. Gitlab is using this version since this commit

    By Administrator on 2014-10-08T12:18:57 (imported from GitLab project)

Please register or sign in to reply

Menu

Help