Introduce new Renderer #1995

kounoike · 2018-05-04T05:33:16Z

Introduce new rendering approach

This PR introduce EnhancedRenderer and some extended classes.

Changes

All files rendered by `EnhancedRenderer`.

It makes more simple logic for blob/diff view.
Previous Renderer assumes used for render Markup. So, image/video/audio renderer is tricky.

correct handling image files

Previously, GitBucket detects image files by MIME-type. But, some image/xxx files can't render by browser. for example tiff file can't render by img tag.

EnhancedRenderer provides ImageRenderer trait. It represents "file can render by img tag".

Image file is not limited to binary

For example, SVG file is text but it can rendered by img tag. So this PR introduce TextImageRenderer. It extends TextRenderer and ImageRenderer. For TextImageRenderer repository viewer and diff viewer render both image and source text .

Allow read file as string or binary for `EnhancedRenderer`

EnhancedRenderer can read file as binary. So, it can render with format converter (for example, tiff to png).

It enables direct serving image file with data-URI. It reduce number of connections.

Diffs rendered by `EnhancedRenderer`.

If plugin has new renderer which extends ImageRenderer, automatically rendered with Image Diff tool.

or, If renderer extends DiffRenderer, diffs are rendered with custom diff rendering.

for example: JSON diff by benjamine/JsonDiffPatch

Compatibility

The RendererWrapper wraps old-style Renderer to new-style EnhancedRenderer. So, no changes required for old-style Renderer plugins.

Before submitting a pull-request to GitBucket I have first:

read the contribution guidelines
rebased my branch over master
verified that project is compiling
verified that tests are passing
squashed my commits as appropriate (keep several commits if it is relevant to understand the PR)
marked as closed using commit message all issue ID that this PR should correct

takezoe

Basically, this extension looks good to enrich GitBucket presentation.

Since this is a much big pull request, I can't review perfectly at once. So I commented based on my first impression as a first review.

takezoe · 2018-05-12T09:20:08Z

src/main/scala/gitbucket/core/plugin/EnhancedRenderer.scala

+
+trait DiffRenderer extends EnhancedRenderer {
+  def renderDiff(oldRequest: EnhancedRenderRequest, newRequest: EnhancedRenderRequest, diffIndex: Int): Html
+}


I'm not sure why DiffRenderer is extending EnhancedRenderer.

takezoe · 2018-05-12T09:21:55Z

src/main/scala/gitbucket/core/plugin/EnhancedRenderer.scala

+/**
+ * New style Renderer
+ */
+trait EnhancedRenderer {


I think it's might better that EnhancedRenderer has only renderContent method and implement the method in concrete classes. While it looks to switch diff presentation in the template by checking type of renderer, I wonder if it can be generalized instead of branching off in the template.

On the other hand, it's understandable that TextRenderer has further rendererMarkup method.

Well... My opinion is the a parent class shouldn't know sub classes.

renderContent() isn't necessary to be implemented at EnhancedRenderer. This method should be only declared at EnhancedRenderer, and you can give implementation at TextRemderer, ImageRenderer and ObjectRenderer.

takezoe · 2018-05-12T10:20:40Z

src/main/twirl/gitbucket/core/repo/editor.scala.html

            <input type="hidden" id="content" name="content" value=""/>
-            <input type="hidden" id="initial" value="@content.content"/>
+            <input type="hidden" id="initial" value="@content.map(_.content).getOrElse("")"/>


.getOrElse("") is unnecessary. Twirl renderes None as an empty string.

takezoe · 2018-05-12T10:39:25Z

src/main/scala/gitbucket/core/util/JGitUtil.scala

+      }
+    }
+
+    def openFile[T](default: T)(f: InputStream => T): T = {


This method shold be private or protected at least.

takezoe · 2018-05-12T10:40:30Z

src/main/scala/gitbucket/core/util/JGitUtil.scala

+      }
+    }
+
+    def content: String = {


This method can be implemented using contentBytes.

takezoe · 2018-05-12T11:32:51Z

src/main/scala/gitbucket/core/view/helpers.scala

+    val fileName = filePath.last.toLowerCase
+    val extension = FileUtil.getExtension(fileName)
+    val renderer = PluginRegistry().getRenderer(extension)
+    if (renderer.isInstanceOf[TextRenderer]) {


Basically, pattern match is better than the combination of isInstanceOf and cast, but adding a method to return TextRenderer might be more better.

takezoe · 2018-05-12T11:38:10Z

src/main/twirl/gitbucket/core/helper/diff.scala.html

+            case (None, None) => {
+              <div>Error???</div>
+            }
+            case (Some(oldCI: ContentInfo), Some(newCI: ContentInfo)) if oldCI.renderer.isInstanceOf[DiffRenderer] && newCI.renderer.isInstanceOf[DiffRenderer] => {


oldCI and newCI are a little confusing because I didn't know what is CI at first. In my opinion, oldContentInfo and newContentInfo are better in this case.

takezoe · 2018-05-12T11:41:00Z

src/main/twirl/gitbucket/core/helper/diff.scala.html

-              } else {
-                @if(diff.changeType != ChangeType.ADD){
-                  <div style="padding: 12px;">Not supported</div>
+          @* TODO: too Large check *@


While TODO comment is remaining, is this pull request ready to be merged?

Sorry, I didn't remove it. This TODO is fixed by introducing LargeFileRenderer

kounoike · 2018-05-12T14:16:56Z

@takezoe Thanks for your review comments. Sorry for large PR. But I can't split to smaller PR.

I fixed this PR with your comments except this comment. It is essential part of this PR. I think we need to discuss it carefully. I think renderAsXXX is easily understandable for plugin developers. Especially its target file format can render as both text and image, like SVG.

takezoe · 2018-06-23T01:23:23Z

src/main/scala/gitbucket/core/util/JGitUtil.scala

+    loader: ObjectLoader,
+    branch: String,
+    filePath: String
+  ) {


Basically, case classes shouldn't cause (or depend on) side effect in the constructor. These properties should be set at the outside of case classes (e.g. You can create a factory method in the companion object to do it).

This also makes possible to extract a renderer using pattern matching, not cast of the renderer property.

takezoe · 2018-06-23T01:31:04Z

src/main/scala/gitbucket/core/plugin/EnhancedRenderer.scala

+/**
+ * New style Renderer
+ */
+trait EnhancedRenderer {


Well... My opinion is the a parent class shouldn't know sub classes.

renderContent() isn't necessary to be implemented at EnhancedRenderer. This method should be only declared at EnhancedRenderer, and you can give implementation at TextRemderer, ImageRenderer and ObjectRenderer.

takezoe · 2018-06-23T01:40:50Z

src/main/twirl/gitbucket/core/helper/diff.scala.html

+            case (None, None) => {
+              <div>Error???</div>
+            }
+            case (Some(oldContentInfo: ContentInfo), Some(newContentInfo: ContentInfo)) if oldContentInfo.renderer.isInstanceOf[DiffRenderer] && newContentInfo.renderer.isInstanceOf[DiffRenderer] => {


I wonder if renderer can be extracted from ContentInfo using pattern matching, not type casting. See also above commet for ContentInfo.

takezoe · 2018-06-23T01:42:25Z

@kounoike I added some comments to the design of your new rederer framework. I hope that you will check them.

ExtArtQ · 2023-01-24T07:14:30Z

And how did it all end?

kounoike added 2 commits May 4, 2018 08:59

Introduce new Renderer: EnhancedRenderer

3fe2dda

add diffIndex option

66da0c9

takezoe requested changes May 12, 2018

View reviewed changes

takezoe added the feature label May 12, 2018

fixed for requested changes

1e36c7d

kounoike mentioned this pull request May 27, 2018

Display SVG images instead of blob by default #729

Open

takezoe requested changes Jun 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new Renderer #1995

Introduce new Renderer #1995

kounoike commented May 4, 2018

takezoe left a comment •

edited

takezoe May 12, 2018

takezoe May 12, 2018

takezoe Jun 23, 2018

takezoe May 12, 2018

takezoe May 12, 2018

takezoe May 12, 2018

takezoe May 12, 2018

takezoe May 12, 2018

takezoe May 12, 2018

kounoike May 12, 2018

kounoike commented May 12, 2018

takezoe Jun 23, 2018

takezoe Jun 23, 2018

takezoe Jun 23, 2018

takezoe commented Jun 23, 2018 •

edited

ExtArtQ commented Jan 24, 2023

Introduce new Renderer #1995

Are you sure you want to change the base?

Introduce new Renderer #1995

Conversation

kounoike commented May 4, 2018

Introduce new rendering approach

Changes

All files rendered by EnhancedRenderer.

correct handling image files

Image file is not limited to binary

Allow read file as string or binary for EnhancedRenderer

Diffs rendered by EnhancedRenderer.

Compatibility

Before submitting a pull-request to GitBucket I have first:

takezoe left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kounoike commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe commented Jun 23, 2018 • edited

ExtArtQ commented Jan 24, 2023

All files rendered by `EnhancedRenderer`.

Allow read file as string or binary for `EnhancedRenderer`

Diffs rendered by `EnhancedRenderer`.

takezoe left a comment •

edited

takezoe commented Jun 23, 2018 •

edited