Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for standalone diffs of images #1223

Merged
merged 37 commits into from
Jun 4, 2023

Conversation

basokant
Copy link
Contributor

@basokant basokant commented Feb 13, 2023

A continuation of the work done in #1179 , including implementing an interactive image compare widget.
This PR intends to add the feature outlined in #664 .

Current progress:

ImageDiff.mov

Tasks:

  • 2-up View
  • Onion Skin View
  • Swipe View
  • Fix colours (especially for dark theme)
  • Handle case where reference and challenger have different aspect ratios
  • Handle case where image diff is opened where the image didn't exist in the reference (it was created in the challenger commit). ie. the diff is opened from the commit where the image was created w/o compare
  • Add support for multiple image formats
  • Fix failing tests
  • Write ui tests

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch basokant/jupyterlab-git/add-image-diff

Comment on lines 96 to 100
const compareImageWidget = ReactWidget.create(
<CompareImage reference={reference} challenger={challenger} />
);

this.addWidget(compareImageWidget);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fcollonval I ended up using react-compare-image instead of the component you suggested for the image comparing functionality. I'm wondering if there's a suggested method to adjust in the height of the Panel widget. Currently, the widget fits the entire width of the panel (as shown by the video in the PR description).

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

This looks awesome 🎉

Do you think you could add a dropbox to switch between viewers 2-up | Slider | Onion (and of course add those 😉) ?

@basokant
Copy link
Contributor Author

This looks awesome 🎉

Do you think you could add a dropbox to switch between viewers 2-up | Slider | Onion (and of course add those 😉) ?

This could be difficult, perhaps if I try using the component that you recommended instead this would be quite easy 🤔.

@fcollonval
Copy link
Member

@basokant
Copy link
Contributor Author

Hi @fcollonval! I attempted to replace the react image compare component with the react image diff component (https://github.com/cezary/react-image-diff), which would give us the views that you suggested for free. I was able to add an external TypeScript declaration file, but after integrating it into our component, it broke the extension with a runtime error:

TypeError: Cannot read properties of undefined (reading 'string')
    at Object.<anonymous> (react-image-diff.js:277:1)

This is caused by the use of PropTypes to validate the types of props. Additionally, many depreciated react class component methods were used in the component, such as componentWillMount. I think that the right course of action is to either

  1. Create a completely new react component from scratch using the GitHub image diff as an inspiration
  2. Use the react-image-compare component and try and build the other desired views on top of it.

How should I progress from here?

@fcollonval
Copy link
Member

Hi @fcollonval! I attempted to replace the react image compare component with the react image diff component (https://github.com/cezary/react-image-diff), which would give us the views that you suggested for free. I was able to add an external TypeScript declaration file, but after integrating it into our component, it broke the extension with a runtime error:

TypeError: Cannot read properties of undefined (reading 'string')
    at Object.<anonymous> (react-image-diff.js:277:1)

This is caused by the use of PropTypes to validate the types of props. Additionally, many depreciated react class component methods were used in the component, such as componentWillMount. I think that the right course of action is to either

1. Create a completely new react component from scratch using the GitHub image diff as an inspiration

2. Use the react-image-compare component and try and build the other desired views on top of it.

How should I progress from here?

Thanks for trying it out @basokant - I would go for the second path. Looking at the code of react-image-diff, it is quite short. So I guess it will be possible to understand how they do the onion view.

@basokant
Copy link
Contributor Author

@fcollonval I somehow wasn't killed by the CSS! Here's a summary of my progress (See a demo in the PR description):

  • I finished minimum working Onion Skin and Swipe views (surprisingly the approach for Swipe was very similar to Onion Skin)
  • Added polish
  • Added reference and challenger labels
  • Added image dimensions and file sizes (for the 2-up view only)

I do still have some next steps (which are also in the PR description). I do have a couple of questions:

  1. I assumed that the image is a PNG. Should we add support for multiple image formats (like JPEG)? This would require some work on the back-end i believe (unless I'm missing something)
  2. (Less important) Should I change the slider width to be the same (largest) image width for Swipe and Onion Skin?
  3. Is the border a good enough indicator for which image/side is the reference vs the challenger? I was thinking of either changing the colour of the slider depending on the slider value and/or adding coloured circles to the left and right of the slider.

@fcollonval
Copy link
Member

  • I assumed that the image is a PNG. Should we add support for multiple image formats (like JPEG)? This would require some work on the back-end i believe (unless I'm missing something)

Yes we should support more formats. But actually, it is in the frontend when you set the source

src={`data:image/png;base64,${reference}`}

That function may be helpful as it allows to get the filetype that got an attribute mimeTypes that should provide you the image type:

protected _resolveFileType(path: string): DocumentRegistry.IFileType {

  • (Less important) Should I change the slider width to be the same (largest) image width for Swipe and Onion Skin?

This seems like a nice improvement

  • Is the border a good enough indicator for which image/side is the reference vs the challenger? I was thinking of either changing the colour of the slider depending on the slider value and/or adding coloured circles to the left and right of the slider.

I let you test and propose what you think more appropriate

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @basokant and sorry for the late review.

Overall it looks good. Would you be able to finish it up and add integration tests? Otherwise I'm happy to finish the PR.

jupyterlab_git/git.py Outdated Show resolved Hide resolved
src/components/diff/ImageDiff.tsx Outdated Show resolved Hide resolved
src/components/diff/ImageDiff.tsx Outdated Show resolved Hide resolved
src/components/diff/ImageDiff.tsx Outdated Show resolved Hide resolved
@fcollonval fcollonval marked this pull request as ready for review June 1, 2023 15:56
@fcollonval fcollonval dismissed their stale review June 1, 2023 15:56

Continuing the work

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

Successfully merging this pull request may close these issues.

3 participants