Bug 14468 - Remove warnings when creating Labels
Summary: Remove warnings when creating Labels
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Bernardo Gonzalez Kriegel
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-28 02:05 UTC by Bernardo Gonzalez Kriegel
Modified: 2016-06-21 21:39 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14468: Remove warnings when creating Labels (4.05 KB, patch)
2015-06-28 02:09 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14468: Remove warnings when creating Labels (4.11 KB, patch)
2015-07-01 00:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14468: Remove warnings when creating Labels (4.17 KB, patch)
2015-08-05 10:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14468: followup to fix tests (1.65 KB, patch)
2015-10-23 16:44 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 14468: followup to fix tests (1.79 KB, patch)
2015-10-26 19:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14468: followup to fix tests (1.85 KB, patch)
2015-10-27 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14468: (QA followup) remove useless diags (8.61 KB, patch)
2015-10-27 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernardo Gonzalez Kriegel 2015-06-28 02:05:12 UTC
There are a lot of warnings when printing labels
Comment 1 Bernardo Gonzalez Kriegel 2015-06-28 02:09:20 UTC Comment hidden (obsolete)
Comment 2 Chris Nighswonger 2015-06-28 19:19:12 UTC
There will most likely be problems removing the use of internal fonts for those who cannot or do not desire to use TTFs. A better solution would be to incorporate the possibility of both. Ie. Check to see if TTFs are configured and fall back to internal fonts if not.
Comment 3 Bernardo Gonzalez Kriegel 2015-06-28 20:45:24 UTC
(In reply to Chris Nighswonger from comment #2)
> There will most likely be problems removing the use of internal fonts for
> those who cannot or do not desire to use TTFs. A better solution would be to
> incorporate the possibility of both. Ie. Check to see if TTFs are configured
> and fall back to internal fonts if not.

Yes, but that is what C4::Creators::PDF->Font does ATM.

sub Font {
    my $self = shift;
    my $fontName = shift;

    my $ttf = C4::Context->config('ttf');

    if ( $ttf ) {
        my $ttf_path = first { $_->{type} eq $fontName } @{ $ttf->{font} };
        if ( -e $ttf_path->{content} ) {
            return prTTFont($ttf_path->{content});
        } else {
            warn "ERROR in koha-conf.xml -- missing <font type=\"$fontName\">/path/to/font.ttf</font>";
        }
    }
    return prFont($fontName);
}

It returns prTTFont or prFont using C4::Context->config('ttf')

Just tried with ttf 'disabled' I get

pdffonts label_batch_1.pdf 
name                                 type              encoding         emb sub uni object ID
------------------------------------ ----------------- ---------------- --- --- --- ---------
Courier                              Type 1            WinAnsi          no  no  no       5  0
Helvetica                            Type 1            WinAnsi          no  no  no       4  0

With ttf enabled
pdffonts label_batch_1.pdf 
name                                 type              encoding         emb sub uni object ID
------------------------------------ ----------------- ---------------- --- --- --- ---------
Courier                              Type 1            WinAnsi          no  no  no       9  0
BXCJIM+DejaVuSans                    CID TrueType      Identity-H       yes yes no       4  0

Courier is used below barcode, the other is for text.
It's working :) (but without ttf not for complex scripts)
Comment 4 Chris Nighswonger 2015-06-29 13:27:00 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #3)
> (In reply to Chris Nighswonger from comment #2)
> > There will most likely be problems removing the use of internal fonts for
> > those who cannot or do not desire to use TTFs. A better solution would be to
> > incorporate the possibility of both. Ie. Check to see if TTFs are configured
> > and fall back to internal fonts if not.
> 
> Yes, but that is what C4::Creators::PDF->Font does ATM.
> 

I realize that. However, it appears that you remove the calls to that in your patch which is what triggered my concern.

I apologize if I'm missing something since I'm really not focused at the moment, but in any case, as long as things still work for someone who elects not to install TTFs on their system, I'm fine with the fix.
Comment 5 Bernardo Gonzalez Kriegel 2015-06-29 13:51:20 UTC
(In reply to Chris Nighswonger from comment #4)
> 
> I realize that. However, it appears that you remove the calls to that in
> your patch which is what triggered my concern.

Yes, I remove a call in C4/Labels/Label.pm, but add explicitly one on
C4/Creators/PDF.pm replacing Galen's code that do almost the same.
If we call it on Label.pm and again in PDF.pm then we have all those
warnings in the logs.


> I apologize if I'm missing something since I'm really not focused at the
> moment, but in any case, as long as things still work for someone who elects
> not to install TTFs on their system, I'm fine with the fix.

No problem :)
I tried that before your first comment and works fine without TTF fonts.

The problem only affects sites with TTF fonts on config.
Originally we transformed 'TR' to 'Ft1' on Labels.pm (using Font function)
then we searched 'Ft1' inside TTF fonts in StrWidth (using Galen's code) and 
spit a warning because it was not found.

Now I send 'TR' to StrWidth, it calls Font sub and get 'Ft1', that is
passed to prStrWidth (works the same with or without TTF)

That's the reason to remove the call on Label.pm

Regards
Comment 6 Chris Nighswonger 2015-06-29 13:58:17 UTC
That all sounds good. I wish I had time to sign off!
Comment 7 Nick Clemens 2015-07-01 00:10:36 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2015-07-01 00:12:17 UTC
Bonus 1 - Works!
Bonus 2 - I only tried with pasting arabic text in as call number but it displayed nicely in PDF which I think is an improvement
Comment 9 Jonathan Druart 2015-07-09 12:00:32 UTC
(In reply to Chris Nighswonger from comment #6)
> That all sounds good. I wish I had time to sign off!

Chris,
I am not confident with part of code, it would be great to get your signoff on this one :)
Comment 10 Bernardo Gonzalez Kriegel 2015-07-10 18:00:28 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Chris Nighswonger from comment #6)
> > That all sounds good. I wish I had time to sign off!
> 
> Chris,
> I am not confident with part of code, it would be great to get your signoff
> on this one :)

Hi Jonathan,
while waiting to Chris, which part of the code is giving you trouble? 
I can review it if you want
Comment 11 Jonathan Druart 2015-08-05 10:36:16 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #10)
> (In reply to Jonathan Druart from comment #9)
> > (In reply to Chris Nighswonger from comment #6)
> > > That all sounds good. I wish I had time to sign off!
> > 
> > Chris,
> > I am not confident with part of code, it would be great to get your signoff
> > on this one :)
> 
> Hi Jonathan,
> while waiting to Chris, which part of the code is giving you trouble? 
> I can review it if you want

Hum, all C4::Creators? :)

It works as expected but I cannot be sure the patch won't introduce regressions.
Looking at it again and all looks good.

Marked as Passed QA.
Comment 12 Jonathan Druart 2015-08-05 10:37:00 UTC
Created attachment 41375 [details] [review]
Bug 14468: Remove warnings when creating Labels

This patch removes 2 types of warnings when
creating Labels

a) Using '0' as filling char is not to the like of prStrWidth
On C4/Creators/Layout.pm

b) A RM followup from Galen on Bug 8375 to C4/Creators/PDF.pm
makes impossible to find properly a font :(
(http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=f7ef93e758850e991091e7268b8d1b1453082df4)
on C4/Labels/Label.pm

To test:
Pre patch
1) Go to Tools > Labels
2) Create a new layout, name ABC, type 'Barcode/Biblio'
3) Create a new batch, add 2 items
4) Export as PDF using the new layout
5) Look at logs, you will find something like

a) label-create-pdf.pl: Use of uninitialized value $strwidth in numeric lt (<) at /home/bgkriegel/kohaclone/C4/Creators/Layout.pm line 233., referer: http://staffdev.koha-community.org.ar/cgi-bin/koha/labels/label-print.pl
(Only one of this kind)

b) Two related lines similar to this examples
label-create-pdf.pl: Use of uninitialized value in -e at /home/bgkriegel/kohaclone/C4/Creators/PDF.pm line 226., referer: http://staffdev.koha-community.org.ar/cgi-bin/koha/labels/label-print.pl
label-create-pdf.pl: ERROR in koha-conf.xml -- missing <font type="Ft1">/path/to/font.ttf</font> at /home/bgkriegel/kohaclone/C4/Creators/PDF.pm line 229., referer: http://staffdev.koha-community.org.ar/cgi-bin/koha/labels/label-print.pl
(many many lines x number of items on batch)

6) Apply the patch
7) Export the same PDF, no more warnings

8) Bonus 1: change main font to Courier and Helvetica
and check results
9) Bonus 2: check using arabic records

Many hours to find :(
Easy to fix

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Tomás Cohen Arazi 2015-08-07 18:03:06 UTC
Hi, please check why this patch makes tests fail:

 $ prove t/db_dependent/Labels/t_Layout.t

t/db_dependent/Labels/t_Layout.t .. 1/36 # Testing Layout->new() method.
# Testing Layout->get_attr() method.
# Testing Layout->set_attr() method.
# Testing Layout->save() method with a new object.
# Testing Layout->retrieve() method.
# Testing Layout->save() method with an updated object.
# Testing Layout->get_text_wrap_cols() method.

#   Failed test 'Layout->get_text_wrap_cols()'
#   at t/db_dependent/Labels/t_Layout.t line 103.
#          got: '21'
#     expected: '23'
# Testing Layout->delete() method.
Use of uninitialized value $del_results in string ne at t/db_dependent/Labels/t_Layout.t line 108.
# Looks like you failed 1 test of 36.
...
Comment 14 Bernardo Gonzalez Kriegel 2015-10-23 16:44:26 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2015-10-23 16:45:41 UTC
Fixed test, changed to needs signoff.
Comment 16 Marc Véron 2015-10-26 19:07:31 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-10-27 09:06:00 UTC
Created attachment 44045 [details] [review]
Bug 14468: followup to fix tests

This patch fix an expected  test result.
Also changes a comparison for the return value to 'delete'
layout sub, in case of success it returns 'undef' and triggers
the message 'use of uninitialized value $del_results ...'

To test:
1) Whitout this patch

prove t/db_dependent/Labels/t_Layout.t

fails.

Also note the message
"Use of uninitialized value $del_results in string ne at t/db_dependent/Labels/t_Layout.t line 110."
for using the return value of delete layout (undef on success!)

2) Apply the patch
3) Test pass and no more message

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Tomás Cohen Arazi 2015-10-27 13:41:28 UTC
Created attachment 44063 [details] [review]
Bug 14468: (QA followup) remove useless diags

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2015-10-27 13:46:05 UTC
Parche aplicado en master.

¡Gracias Bernardo!