Problem/Motivation

Follow-up from: #2825350: Fix image changes display in revisions. Image plugin should allow show an image thumbnail for having a better overview of what is being compared.

Proposed resolution

Add the thumbnail, probably add it to settings too.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

yongt9412 created an issue. See original summary.

miro_dietiker’s picture

We should also create a separate issue about exif data export diff visualisation (on exiftool, if present) and connect the issues.

miro_dietiker’s picture

Component: Code » Diff builder plugins
Related issues: +#2825763: Enrich image diff based on file metadata
johnchque’s picture

Status: Active » Needs review
StatusFileSize
new2.88 KB
new61.47 KB

First try, made images appear in the diff comparison.

miro_dietiker’s picture

+++ b/src/Plugin/diff/Field/ImageFieldBuilder.php
@@ -57,6 +57,19 @@ class ImageFieldBuilder extends FieldDiffBuilderBase {
+            '#style_name' => 'thumbnail',

Is thumbnail guaranteed to exist? I doubt.

We need to decide if we are using some style that is configured or if we need to have this configurable such as with the diff plugin setting. But i would try to avoid that.

miro_dietiker’s picture

Status: Needs review » Needs work

So checked, yeah the thumbnail image style can be deleted.

But the form display has the preview image style configured. We should simply use that one.

I guess, we should offer a new setting to display the image (default on), right?

About the check for 'thumbnail' -- shouldn't it be '#thumbnail'?

This is pretty invasive for DiffEntityComparision. I wonder how to deal with other advanced elements in diff.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new4.22 KB
new4.25 KB

Made some changes, checking the code, if might need to rework the diff process so we can also add thumbnail for other file types.

Status: Needs review » Needs work

The last submitted patch, 7: show_image_thumbnail-2825762-7.patch, failed testing.

miro_dietiker’s picture

+++ b/src/Plugin/diff/Field/ImageFieldBuilder.php
@@ -57,6 +57,21 @@ class ImageFieldBuilder extends FieldDiffBuilderBase {
+              '#style_name' => 'thumbnail',

Let's first focus on picking the right image style. As stated, the preview style is configured in manage form display.

Then we can discuss about how much alteration makes sense and is acceptable in the current lifecycle.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new4.53 KB
new1.5 KB

Using that setting from the form display.

Status: Needs review » Needs work

The last submitted patch, 10: show_image_thumbnail-2825762-10.patch, failed testing.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new5.03 KB
new439 bytes

Sorry, added schema.

miro_dietiker’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/src/Plugin/diff/Field/ImageFieldBuilder.php
    @@ -57,6 +57,22 @@ class ImageFieldBuilder extends FieldDiffBuilderBase {
    +        if ($this->configuration['compare_alt_field']) {
    

    compare_alt_field => show_thumbnail
    And this shows why we need test coverage... ;-)

  2. +++ b/src/Plugin/diff/Field/ImageFieldBuilder.php
    @@ -57,6 +57,22 @@ class ImageFieldBuilder extends FieldDiffBuilderBase {
    +              $image_style[$field_key]['#thumbnail'] = array(
    ...
    +              $result = array_merge($result, $image_style);
    

    Why not just $result[$field_key]['#thumbnail']

And then we need to discuss the APIs and how we make this clean for other advanced cases.

johnchque’s picture

Status: Needs work » Needs review
StatusFileSize
new6.5 KB
new2.19 KB

True, tests added.

miro_dietiker’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests
Related issues: +#2840566: Make rich diff pluggable

I'm kinda missing the image in the Unified fields layout.
Added some comments and committed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.