-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
[IMP] Add QRcode in printer_zpl2 module #105
Conversation
1fec5a0
to
80a92a6
Compare
Hey @alnslang, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
dabcbcd
to
1b12155
Compare
(zpl2.QRMODEL_ENHANCED, 'Enhanced'), | ||
], required=True, default=zpl2.QRMODEL_ENHANCED, | ||
help='Model') | ||
qrmagnification = fields.Selection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace by a simple Integer field and rename to magnification_factor
.
@@ -84,6 +84,7 @@ def _generate_zpl2_components_data( | |||
for (component, data, offset_x, offset_y) in to_print: | |||
component_offset_x = component.origin_x + offset_x | |||
component_offset_y = component.origin_y + offset_y | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unneeded empty line.
@@ -127,6 +128,23 @@ def _generate_zpl2_components_data( | |||
label_data, data, | |||
label_offset_x=component_offset_x, | |||
label_offset_y=component_offset_y) | |||
elif component.component_type == zpl2.BARCODE_QRCODE: | |||
# Adding Control Arguments to QRCode data Label | |||
data = 'MM,A{}'.format(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The section (else
) already calls the barcode_data
method.
Move the custom data change into the else
to avoid duplicating code.
You will simply have to add the new arguments added by the QR Code format in the arguments list, as the barcode_data
already filters the arguments to take in account depending on the barcode type.
@@ -142,3 +143,36 @@ class PrintingLabelZpl2Component(models.Model): | |||
block_left_margin = fields.Integer( | |||
string='Left Margin', | |||
help='Left margin for the second and other lines in the block.') | |||
qrmodel = fields.Selection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to model
.
(zpl2.QRMAGNIFICATION_1000DPI, '1000 Dpi'), | ||
], required=True, default=zpl2.QRMAGNIFICATION_300DPI, | ||
help='Magnification Factor') | ||
qrerrorcorrection = fields.Selection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to error_correction
.
(zpl2.QRERRORCORRECTION_HIGH_DENSITY, 'High density level'), | ||
], required=True, default=zpl2.QRERRORCORRECTION_HIGH, | ||
help='Error Correction') | ||
qrmask = fields.Integer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to mask_value
.
@@ -81,6 +81,15 @@ | |||
<field name="truncate"/> | |||
</group> | |||
</page> | |||
<!-- QRCode arguments --> | |||
<page string="QRCode Arguments" attrs="{'invisible': [('component_type', '!=', 'qrcode')]}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the string to 2D Barcode Arguments
to be more generic.
1b12155
to
ac5862a
Compare
All changes done. @pedrobaeza The CLA should be OK, can the bot recheck ? |
With a new commit from the person a message will be put about the validity |
ac5862a
to
8316da5
Compare
Hey @alnslang, Appreciation of efforts, |
8316da5
to
8dd56b9
Compare
@pedrobaeza could you validate this pull request as well please ? |
Sorry, but I don't know about this module. Other should be the reviewer. |
@JosDeGraeve Maybe you can look at this and test the PR ? |
@sylvain-garancher i'll try - please remind me if it takes too long :) |
Thanks @fmdl |
Hello @pedrobaeza can we merge the request please ? |
The problem I see with this is that current working installations with zpl2 version 1.0 won't work after this update, which goes against same version stability. What do you think, @sylvain-garancher ? |
@pedrobaeza The requirements file has been updated, so this is an issue only if the install doesn't respect the requirements, is this still blocking for you ? |
Yes. Someone that has a deployment working, updating this repository will find that it's not working anymore. |
@pedrobaeza What solution do you propose to accept this PR? |
@pedrobaeza It's a nice improvement, we must find a way to not block this kind of change. IMO it's not blocking but the change of the minimal version required for the module should be mentioned into the README. |
@alnslang Can you also modify the setup.py file of this module to enforce the minimal version of spl2 plz as follow: setuptools.setup(
setup_requires=['setuptools-odoo'],
odoo_addon={
'external_dependencies_override': {
'python': {
'zpl2': 'zpl2>=1.1',
},
},
},
) With this change the upgrade will be automatic for those using pip to install the addons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the improvemenet @alnslang
In the case of a library version mismatch, can you explain to me what the error is? From reading the code, I am guessing that the error would trigger any time that a label is printed? Am I correct in that guess?
OK, then go ahead. I'm not a direct user of this module, but I was trying to avoid breaking installations. If you say that there's no problem or it's an acceptable risk, then I won't block anymore. |
printer_zpl2/README.rst
Outdated
@@ -15,6 +15,7 @@ See below for more details. | |||
Installation | |||
============ | |||
|
|||
This module requires zpl2 >= version 1.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of just noting the requirement, we need to state this as an upgrade note. Users upgrading pre-existing installations are the ones that this affects, not new users. As such, I recommend:
**Upgrade Note:** If upgrading from a version prior to 10.0.2.0.0, you must make sure to update the `zpl2` Python package to at least version 1.1.
Also, please bump the module version by a major version: 10.0.2.0.0
071befc
to
f821f51
Compare
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Syncing from upstream OCA/report-print-send (15.0)
Add new barcode (QR Code format) in printer_zpl2 module