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

Adds side-by-side diff for images #6784

Merged
merged 42 commits into from
Sep 16, 2019
Merged

Conversation

saitho
Copy link
Contributor

@saitho saitho commented Apr 27, 2019

This adds a basic image diff and prints the width of the images (before and after). This is part of what was requested in #6255 .

Screenshot:
image-diff

The side-by-side view for images will be active regardless whether split view is active or not.

Signed-off-by: Mario Lubenka <[email protected]>
@codecov-io
Copy link

codecov-io commented Apr 27, 2019

Codecov Report

Merging #6784 into master will decrease coverage by 0.04%.
The diff coverage is 8.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6784      +/-   ##
==========================================
- Coverage   41.64%   41.59%   -0.05%     
==========================================
  Files         483      483              
  Lines       64631    64741     +110     
==========================================
+ Hits        26916    26932      +16     
- Misses      34274    34360      +86     
- Partials     3441     3449       +8
Impacted Files Coverage Δ
modules/git/commit.go 51.89% <0%> (-5.31%) ⬇️
routers/repo/commit.go 27.03% <0%> (-2%) ⬇️
routers/repo/pull.go 30.74% <0%> (-0.88%) ⬇️
routers/repo/compare.go 48.74% <21.27%> (-5.11%) ⬇️
routers/repo/view.go 43.54% <0%> (+1.01%) ⬆️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️
models/unit.go 67.56% <0%> (+5.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5f87fe...69083b1. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 27, 2019
@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Apr 28, 2019
@lafriks lafriks added this to the 1.9.0 milestone Apr 28, 2019
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Apr 28, 2019
@lafriks
Copy link
Member

lafriks commented Apr 28, 2019

Probably better would be to have Width: 1156, Height: 412 instead of W: 1156, H: 412

Also what will happen with very large images?

@saitho
Copy link
Contributor Author

saitho commented Apr 28, 2019

The images are embedded how they were uploaded in the repository, i.e. in their full dimensions.
The width is limited by width: 100% in .diff-file-box.file-content img which is already present.

ctx.Data["RawPath"] = setting.AppSubURL + "/" + path.Join(headTarget, "raw", "commit", endCommitID)
ctx.Data["BeforeSourcePath"] = setting.AppSubURL + "/" + path.Join(baseTarget, "src", "commit", startCommitID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I changed "headTarget" to "baseTarget", as the "before" paths should point to the forked repository.

modules/git/commit.go Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

If it's not too hard, I'd request adding a size below each image. Would be useful to compare size optimizations:

Screenshot 2019-05-02 at 09 10 06

@saitho
Copy link
Contributor Author

saitho commented May 2, 2019

If it's not too hard, I'd request adding a size below each image. Would be useful to compare size optimizations

I'll look into that. :) I assume that the filesystem library is able to do that.

Signed-off-by: Mario Lubenka <[email protected]>
@saitho
Copy link
Contributor Author

saitho commented May 4, 2019

image-diff-filesize

There we go. :)

Signed-off-by: Mario Lubenka <[email protected]>
modules/git/commit.go Outdated Show resolved Hide resolved
routers/repo/pull.go Outdated Show resolved Hide resolved
@saitho
Copy link
Contributor Author

saitho commented Jul 10, 2019 via email

@saitho
Copy link
Contributor Author

saitho commented Aug 10, 2019

Hey there. Sorry for the long delay.
Wasn't really motivated to work on Gitea - now I am. :)

2. When I create a new empty repository and commit an image in the first commit, there will  be a corrupted old image. (see screenshot and code)
   ![Screenshot_2019-07-09 geissler](https://user-images.githubusercontent.com/5757182/60919844-063e0500-a297-11e9-9304-4f0343286b00.png)

This happens when the image is newly added to the repository (i.e. is not in the index of the base commit). Fixed by only displaying the image if it exists in the base commit index.

@saitho
Copy link
Contributor Author

saitho commented Aug 10, 2019

Ready for review @jonasfranz

@guillep2k
Copy link
Member

Could this PR have latest master merged in and CI restarted? Looks like CI problems are not from this PR.

@lunny
Copy link
Member

lunny commented Sep 14, 2019

Since @jonasfranz didn't response this one for a month and there are two approvals on this, I think we can merge this one when CI PASS.

@guillep2k
Copy link
Member

@lunny @lafriks It looks like the tests I've added to the UTF functions are quite problematic. They fail only from time to time. I guess it's a LOCALE setting. Can a specific LOCALE be forced in the Makefile for the tests? Something like LOCALE=en_US.UTF-8 or any other, as long as it is always the same.

@guillep2k
Copy link
Member

Otherwise, I'd remove most of the UTF8 tests. I don't think they're that much important.

@lunny
Copy link
Member

lunny commented Sep 16, 2019

make L-G-T-M work

@lunny lunny merged commit a372363 into go-gitea:master Sep 16, 2019
@saitho saitho deleted the feature/6255-Image_diff branch December 18, 2019 18:53
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.