iframe-proxy

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

+
+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

+/**
+ * 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

+            <input type="hidden" id="lineSeparator" name="lineSeparator" value="@content.map(_.lineSeparator).getOrElse("LF")"/>
            <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

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


This method shold be private or protected at least.

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

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


This method can be implemented using contentBytes.

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

+    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

+            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

-              } 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

+    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

+/**
+ * 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

+            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

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

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:

Uh oh!

takezoe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kounoike commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takezoe commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ExtArtQ commented Jan 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

All files rendered by `EnhancedRenderer`.

Allow read file as string or binary for `EnhancedRenderer`

Diffs rendered by `EnhancedRenderer`.

takezoe left a comment •

edited

Loading

takezoe commented Jun 23, 2018 •

edited

Loading