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

Update to v1.0.3 SDWebImage Version #11

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tuyen-vuduc
Copy link

@tuyen-vuduc tuyen-vuduc commented Oct 9, 2019

  • Use carthage for WebP library dependency
  • Set version of the DDL to version of dependency
  • Cache the current WebP framework for future reference
  • Support tvOS
  • Change nuget package version to 1.0.8.3

Try the package before merge from here

Vũ Đức Tuyến added 4 commits October 9, 2019 23:06
…arinComponents repository

- Use libwebp from SDWebImage
- Update libwebp version to v1.0.3
- Use carthage for compiling libwebp depedency
- Use libwebp framework instead of A file
- Copy dependent frameworks to frameworks for future references
- Update Nuget package to v1.0.7.3
@@ -25,7 +25,7 @@
// The form "{Major}.{Minor}.*" will automatically update the build and revision,
// and "{Major}.{Minor}.{Build}.*" will update just the revision.

[assembly: AssemblyVersion("1.0.*")]
[assembly: AssemblyVersion("1.0.3")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason to have the Build specified?

Copy link
Author

Choose a reason for hiding this comment

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

I want the assembly to have the same version as the main library.

Any subsequence changes will be increase 1 by 1.

E.g. if native library version is 1.0.3 then our first binding version will be 1.0.3.

If there are any corrections afterward, we will increase version by the forth part -> 1.0.3.x.

That way we will know what is the actual version of the native library easily.

You might check out the way Xamarin support libraries versioned.


namespace WebP
{
internal static class CFunctions
Copy link
Collaborator

Choose a reason for hiding this comment

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

if not used you can drop it

Copy link
Author

Choose a reason for hiding this comment

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

:). I copied from the iOS proj.

[assembly: AssemblyTitle("WebP.tvOS")]
[assembly: AssemblyDescription("")]
[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCompany("NAXAM COMPANY LIMITED")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@daniel-luberda should we keep these assembly company/trademark etc ?

Copy link
Author

Choose a reason for hiding this comment

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

These are auto generated.

{
private readonly WebPDecoder _decoder;

/// <summary>
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting looks strange, maybe you can autoformat?

Copy link
Author

Choose a reason for hiding this comment

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

Weird, code is copied from iOS source.

@molinch
Copy link
Collaborator

molinch commented Oct 10, 2019

@tuyen-vuduc Fantastic work! Thanks a lot.
As I am outside of mobile developments since a couple of years now I'll let @daniel-luberda have a look

- remove trademark and company info
@tuyen-vuduc
Copy link
Author

Hi @molinch,

I made the changes to

  • remove our company trademark and info
  • reformat code as stated above

Please check it again.

@molinch
Copy link
Collaborator

molinch commented Oct 11, 2019

@tuyen-vuduc Looks perfect to me, unfortunately I cannot test it yet.
If @Benesss , @Jose7777, @daniel-luberda or @cosmin-gordea could have a look then we can merge it in :)

@tuyen-vuduc
Copy link
Author

Hi guys,

Do you all review it? I really expect it to be released ASAP 💯

Thanks.

@tuyen-vuduc
Copy link
Author

@molinch, @Benesss, @daniel-luberda,

Could we do the merge and publish new package?

If you don't have time to maintain the binding, could you invite me as a maintainer and the package distributor?

Thanks.

@daniel-luberda
Copy link
Member

Hi @tuyen-vuduc

Could you tell me what's the size of the lib after compilation? I'm worried that it would be twice as current one, because we use decoder only (WebPDecoder.framework)

@tuyen-vuduc
Copy link
Author

Hi @daniel-luberda,

The framework compiled with Carthage is 6.7MB while the previous one is ~4.5MB.

I think SDWebImage is very commonly used by native iOS developers. I won't think it'll be issue.

Additionally, by using that the same lib from SDWebImage, we could use this library as the dependency for SDWebImage package; otherwise it isn't possible.

Other than that, the reason I compiled the new A lib because when I linked this current WebP.Touch nuget package with SDWebImage nuget package in Xamarin component, it doesn't work.

Regards.

@tuyen-vuduc
Copy link
Author

@daniel-luberda How do you think?

@tuyen-vuduc
Copy link
Author

Hi guys,

Really wanted to get this merge.

Thank you.

@tuyen-vuduc
Copy link
Author

Hi guys,

Any progress here?

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

Successfully merging this pull request may close these issues.

3 participants