search

barnhill / barcodelib

C# Barcode Image Generation Library
Apache License 2.0
733 stars 238 forks source link

Skia sharp drawing lib #152

Closed barnhill closed 1 year ago

barnhill commented 1 year ago

@binki This will remove the dependency on System.Drawing.Common which isnt cross platform compatible. To replace it this will transition to utilizing SkiaSharp as the drawing lib for cross platform support.

UPCE and EAN8 barcodes are broken but I checked prior to this and they were broken prior to this and generate the same barcode so I will consider it a separate issue.

This will also introduce a lot of tests for various symbologies. However the coverage is incomplete but adds more than we have today.

tlogik commented 1 year ago

Hi @binki and @barnhill What is required for the PR to go thru in relation to review?

barnhill commented 1 year ago

I could merge it today but wouldn't mind some eyes on it to make sure I didn't break something. I think labels are kinda messed up but those need either removed or fixed as they don't work right even before this change

tlogik commented 1 year ago

I could merge it today but wouldn't mind some eyes on it to make sure I didn't break something. I think labels are kinda messed up but those need either removed or fixed as they don't work right even before this change

I there something i could do here? I do not have the overview of the code as you do, i just found it, but i might be able to do some assistance but for me to review you change it would require me to really dive into the code and that will take some time no matter what.

Also this is a breaking change right Will it just be a Major version update? Lastly it seems the documentation might need to be updated as well (I could update the PR with an updated Readme)

Let me know if i can assist :-)

ozymandias13 commented 1 year ago

I'm very interested in this update as well, so I'll give it a review and make sure nothing jumps out at me.

barnhill commented 1 year ago

The big changes were around migrating to SkiaSharp to do the drawing but there were some issues with labels (which are totally screwed up) but it looks like they pre-dated this update. Im not a huge fan of how the labels were written originally (by me). Also the UPC-E barcode seems to not be encoding properly to read but I tested that and it pre-dated this as well. I will take a look at that separately. Everything else seems to be ok.

barnhill commented 1 year ago

I could merge it today but wouldn't mind some eyes on it to make sure I didn't break something. I think labels are kinda messed up but those need either removed or fixed as they don't work right even before this change

I there something i could do here? I do not have the overview of the code as you do, i just found it, but i might be able to do some assistance but for me to review you change it would require me to really dive into the code and that will take some time no matter what.

Also this is a breaking change right Will it just be a Major version update? Lastly it seems the documentation might need to be updated as well (I could update the PR with an updated Readme)

Let me know if i can assist :-)

I think this large of a change warrants a major version number update. :)

barnhill commented 1 year ago

Also added builds on windows and linux and .net 6 and 7 for verification :)

barnhill commented 1 year ago

I still see a slight deviation from center on the generic label. Need to fix this before merging.

barnhill commented 1 year ago

Ok I think I have the centering of the generic symbology labels fixed, also fixed the EAN-13 labels. Anyone wanna give this a whirl to see if its ready to go?

barnhill commented 1 year ago

Im going to do some more spot checking and if all still looks good Im going to deploy it. It does contain some breaking changes with the API but thats why its a major version bump too.

ozymandias13 commented 1 year ago

Im going to do some more spot checking and if all still looks good Im going to deploy it. It does contain some breaking changes with the API but thats why its a major version bump too.

For what it's worth, as another data point: we pulled this branch down and built the lib and pulled it into our project and updated our usage to test the new API for the standard 2 of 5 and it worked just fine.

barnhill commented 1 year ago

Just went through and generated a bunch of different types and scanned them in 2 online barcode scanners and it detected them all correctly. I think we are good to go.