SDWebImage / SDWebImageAVIFCoder

A SDWebImage coder plugin to support AVIF(AV1 Image File Format) image
MIT License
55 stars 14 forks source link

Refactoring & fix a bug & add tests to increase code coverage. #18

Closed ledyba-z closed 4 years ago

ledyba-z commented 4 years ago

Hi.

I separate SDWebImageAVIFCoder.m to 3 files. And also, I write some new tests to increase code coverage. After that, I found a crash bug about ICC profile and fix it.

Could you take a look?

codecov[bot] commented 4 years ago

Codecov Report

Merging #18 into master will increase coverage by 18.87%. The diff coverage is 86.74%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #18       +/-   ##
===========================================
+ Coverage   67.13%   86.01%   +18.87%     
===========================================
  Files           4        6        +2     
  Lines        1494     1494               
===========================================
+ Hits         1003     1285      +282     
+ Misses        491      209      -282     
Impacted Files Coverage Δ
SDWebImageAVIFCoder/Classes/ColorSpace.m 84.97% <84.97%> (ø)
SDWebImageAVIFCoder/Classes/Conversion.m 87.62% <87.62%> (ø)
SDWebImageAVIFCoder/Classes/SDImageAVIFCoder.m 81.75% <100.00%> (+14.85%) :arrow_up:

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 501b702...27372a5. Read the comment docs.

dreampiggy commented 4 years ago

Wait me for review...

Does these method should be public APIs ? If not, you should move them into a private folder (for example, make folder strcuture looks SDWebImage/Classes/Private and update it for 3 different Packages:

Modify Podspec and use s.private_header_files = 'SDWebImageAVIFCoder/Classes/Private/*.{h,m}'

Update the Xcode's File Attribute and mark the ColorSpace.h into Project or Private.

Update the Package.swift to only expose the publicHeadersPath arg point to the SDWebImageAVIFCoder/Classes/Public/*.

dreampiggy commented 4 years ago

Or I can do this myself after merging, since I'm more familiar with all these suck Package Manager :)

ledyba-z commented 4 years ago

Yeah, ColorSpace.h and Conversion.h must be private.

Or I can do this myself after merging, since I'm more familiar with all these suck Package Manager :)

Haha, thanks! I tried to do it myself... Could you check it?

ledyba-z commented 4 years ago

I think I can make ColorSpace.h and Conversion.h private, but tests became to fail because of it...

How should I do?

dreampiggy commented 4 years ago

Because you access the private header outside framework. In the test case:

image

You should use some tricks on the Test Case target, update the HEADER_SEARCH_PATH in SDWebImageAVIFCoder_Tests target (Open that Example/SDWebImageAVIFCoder.xcworkspace and choose the Test Target):

HEADER_SEARCH_PATH = $(inherited) ${PODS_ROOT}/Headers/Private

I can merge it and check each Package Manager and Project Settings again, to avoid some extra issues.

ledyba-z commented 4 years ago

Thanks to approve!

I will merge after CI passed.