search

huysentruitw / barcoder

Lightweight Barcode Encoding Library for .NET Framework, .NET Standard and .NET Core.
MIT License
157 stars 37 forks source link

Optionally include textual EAN content in rendered output #19

Closed edweij closed 4 years ago

edweij commented 4 years ago

Possibility to choose if string content also should be rendered on EAN8 and EAN13 barcodes.

huysentruitw commented 4 years ago

Thank you for this pull request.

There are a few things I'd like to discuss:

  1. Can we get rid of the embedded font and let the user use a system font? F.e. let them pass the font-family name. We should avoid font-embedding, it's not flexible and we don't want to care about font licenses.
  2. Can we remove the duplicated render code? Currently the render methods you have added are 99% a duplicate, can we render the barcode as is and render the text on top (so first draw a rect with background color fill to make room for the text)?
edweij commented 4 years ago

Hi @huysentruitw, thank you for your good comments.

I'll update to use system fonts instead and remove the embedded font. I did't like the extra barcode types introduced to let the renderer know if content should be rendered but I couldn't come up with a better idea? Specifying a font name has the same problem. Maybe there is a more elegant way to just send this to the renderer, even if it is only relevant to EAN barcodes? Suggestions?

I can use your idea with a white rect to make room for the text and remove the duplicate code.

huysentruitw commented 4 years ago

I'd add two arguments to the ImageRenderer and to a new constructor for the SvgRenderer:

Later, I can choose to obsolete those constructor in favor for ones with an options object instead.

By doing the change this way, we do not need to touch the barcode core code.

edweij commented 4 years ago

Ok, that sounds good.

I have replaced the embedded font with string eanFontFamily. I have removed the extra barcode types and added the constructor parameters you suggested.

I still need to look at the render methods. I let you know when it's ready.,

edweij commented 4 years ago

I think the code is updated as you wanted now .

With the new parameter includeEanContentAsText in the new constructor for SvgRenderer the methods are no longer static. Is this as you suggested?

huysentruitw commented 4 years ago

Thank you for this implementation. 🎉

I will merge it to the develop branch as I want to make some changes to the code-layout before releasing to master.