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.

(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)

(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.

(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

That all sounds good. I wish I had time to sign off!

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

(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 :)

(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

(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.

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>

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.
...

Fixed test, changed to needs signoff.

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>

Created attachment 44063 [details] [review]
Bug 14468: (QA followup) remove useless diags

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Parche aplicado en master.

¡Gracias Bernardo!