Bug 8375 - Common diacritics not shown correctly when exporting batch label to PDF
Summary: Common diacritics not shown correctly when exporting batch label to PDF
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: PC Linux
: P5 - low normal (vote)
Assignee: wajasu
QA Contact:
URL:
Keywords:
: 10512 11423 12286 (view as bug list)
Depends on: 9854 9856
Blocks: 12068
  Show dependency treegraph
 
Reported: 2012-07-05 23:34 UTC by wajasu
Modified: 2018-11-21 11:27 UTC (History)
24 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch enables some diacritics to show correctly when exporting labels to PDF (917 bytes, patch)
2012-07-06 01:47 UTC, wajasu
Details | Diff | Splinter Review
Bug 8375 - Patch for Common diacritics not shown correctly when exporting batch label to PDF (941 bytes, patch)
2012-07-06 01:55 UTC, Mason James
Details | Diff | Splinter Review
Bug 8375 - Patch for Common diacritics not shown correctly when exporting batch label to PDF (988 bytes, patch)
2012-07-06 01:56 UTC, Mason James
Details | Diff | Splinter Review
pic (25.37 KB, image/png)
2012-07-06 02:09 UTC, Mason James
Details
use truetype fonts in PDF::Reuse (2.48 KB, patch)
2012-07-19 22:04 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
use truetype fonts in PDF::Reuse (2.57 KB, patch)
2012-10-19 01:19 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 8375 - use truetype fonts in PDF::Reuse (4.21 KB, patch)
2013-03-20 16:40 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 8375 - use truetype fonts in PDF::Reuse (4.26 KB, patch)
2013-09-25 18:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8375 - Followup - Fix font code (1.11 KB, patch)
2014-04-09 00:56 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 8375 - Followup 2 - Fix label justification/aligning (3.90 KB, patch)
2014-04-09 00:57 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
pic2 (29.82 KB, image/png)
2014-04-09 01:35 UTC, Bernardo Gonzalez Kriegel
Details
Bug 8375 - Followup - Fix font code and alignment (4.09 KB, patch)
2014-04-09 17:42 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bad right-to-left text (15.55 KB, image/png)
2014-04-10 14:23 UTC, Bernardo Gonzalez Kriegel
Details
Good right-to-left text (13.01 KB, image/png)
2014-04-10 14:24 UTC, Bernardo Gonzalez Kriegel
Details
Bug 8375 - Followup - Use unicode bidi for rtl languages (4.06 KB, patch)
2014-04-10 14:25 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 8375 - Followup - Fix font code and alignment (3.32 KB, patch)
2014-04-20 23:48 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Arabic records to test with (13.56 KB, text/plain)
2014-04-21 17:57 UTC, Chris Nighswonger
Details
Bug 10859: (follow-up) clarify logic on multiple loans on same bib (2.13 KB, patch)
2014-04-23 13:52 UTC, Chris Nighswonger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8375 - Followup - Fix font code and alignment (3.90 KB, patch)
2014-04-23 13:55 UTC, Chris Nighswonger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8375 - use truetype fonts in PDF::Reuse (4.32 KB, patch)
2014-04-23 13:56 UTC, Chris Nighswonger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8375 - Followup - Fix font code and alignment (3.90 KB, patch)
2014-04-23 13:56 UTC, Chris Nighswonger
Details | Diff | Splinter Review
pdf broken with barcode 50&51 (19.06 KB, application/pdf)
2014-05-02 12:18 UTC, Jonathan Druart
Details
Display variations (31.38 KB, image/png)
2014-05-02 14:16 UTC, Bernardo Gonzalez Kriegel
Details
Marc file containing record with UTF-8 (436 bytes, text/plain)
2015-03-26 14:53 UTC, Barton Chittenden
Details
UTF-8 Quotes (22.64 KB, image/png)
2015-03-26 16:46 UTC, Bernardo Gonzalez Kriegel
Details

Note You need to log in before you can comment on or make changes to this bug.
Description wajasu 2012-07-05 23:34:01 UTC
When exporting a PDF for labels, many common diacritics display as 2 unrelated characters.

-----------------
Steps to reproduce bug:
a) setup a koha test environment
b) get a record with a diacritic in your system
 catalogue search -> Z3950 search -> Author: jurgen -> select
 the record matching the one i selectd above (jahre) ->
 save record -> add item -> wait for zebra to index (5min) ->
 catalog serach: jahre
c) generate a label batch
 more -> tools -> label creator -> batch -> new -> add item ->
  title: jahre   -> select and add checked 
   (select library if not already set)
d) export as PDF
  select record -> export batch -> export as PDF
e) open and view PDF  (zoom as necessary)
   (you can select text with PDF viewer and cut to cllipboard if needed)

Actual Results: 
Example PDF output that I cut and pasted:

1150Jahre Marköbel, 850 Jahre Baiersröderhof
3886542378
Books
943/.416

Expected Results:
The output should have the diacritic for ö correctly displayed.
1150 Jahre Marköbel, 850 Jahre Baiersröderhof 


Build Date & Platform: master
    2012-07-05 on Debian Squeeze  Linux 2.6.32

Additional Information: Refer to my detailed efforts in bug 2246.
There were too many different target solutions, so I broke the specific
behavior out in this new bug.
Comment 1 Chris Nighswonger 2012-07-06 00:27:04 UTC
I'm marking these label/diacritical related bugs as WONTFIX due to the well rehearsed issues of the pdf standard and diacriticals. If someone wants to put time into this, feel free to reopen and take ownership of these bugs.
Comment 2 wajasu 2012-07-06 00:34:29 UTC
I'm adding a fix for this.
Comment 3 wajasu 2012-07-06 01:47:13 UTC Comment hidden (obsolete)
Comment 4 Mason James 2012-07-06 01:55:38 UTC Comment hidden (obsolete)
Comment 5 Mason James 2012-07-06 01:56:57 UTC Comment hidden (obsolete)
Comment 6 Mason James 2012-07-06 01:58:56 UTC
(In reply to comment #5)
> Created attachment 10653 [details] [review]
> Bug 8375 - Patch for Common diacritics not shown correctly when exporting
> batch label to PDF
> 
> Signed-off-by: Mason James <mtj@kohaaloha.com>

works for me, nice spotting! :)
Comment 7 Mason James 2012-07-06 02:09:41 UTC
Created attachment 10654 [details]
pic

... and a pic too
Comment 8 wajasu 2012-07-06 13:47:50 UTC
I did some further testing.  I added a second label, but with some unicode Hindi text, and that was enough to prevent this fix from working if one other label has unicode text in the batch.  It prevented the utf8::decode from flipping the utf8 flag.

This fix helps some of the time, and is a best effort, and will help for many latin1 cases.

But this might be applicable for many one off spine label or small batch runs.

Block-C1_Controls_and_Latin-1_Supplement is probably what is being enable here.
Block-Latin_Extended-A - may not be.
Comment 9 Paul Poulain 2012-07-13 15:06:25 UTC
QA comment: tiny patch, perlcritic OK after the patch

passed QA
Comment 10 Paul Poulain 2012-07-13 15:08:37 UTC
wajasu = I think you're not on the developer list, in docs/history.txt

Could you dive me your complete name ? (I can put "wajasu" in history.txt if it's your name, just confirm ;-) )
Comment 11 Chris Cormack 2012-07-17 05:35:45 UTC
Pushed to 3.8.x, no string changes, will be in 3.8.3
Comment 12 Dobrica Pavlinusic 2012-07-19 13:35:24 UTC
I'm getting

Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 825.

errors when I try to use label printing with utf-8 characters (with or without this patch).

However, when changing utf8::decode to utf8::encode in this patch, everything works. Let me try to explain why:

PDF::Reuse does binmode on it's file-handle so we shouldn't pass any utf-8 characters ("wide characters") to it. However, this patch proposes using utf8::decode which (according to perldoc utf8) does:

       ·   $success = utf8::decode($string)

           Attempts to convert in-place the octet sequence in UTF-X to the corresponding character sequence.  The UTF-8 flag is turned on
           only if the source string contains multiple-byte UTF-X characters.  If $string is invalid as UTF-X, returns false; otherwise
           returns true.

So decode is used when we are converting bytes into utf-8 strings, not the other way around.

However, utf8::encode does exactly what we need:

       ·   utf8::encode($string)

           Converts in-place the character sequence to the corresponding octet sequence in UTF-X.  The UTF8 flag is turned off, so that
           after this operation, the string is a byte string.  Returns nothing.

Which is exactly why I'm proposing to change this patch to use encode instead of decode.

I guess that this patch has passed sign-off and QA because it's a special case:

We are not correctly converting latin-1 characters into utf-8 when using Z39.50 search, so we get utf-8 marked string which contains latin1 umlauts inside it. In that case, utf8::decode correctly strips utf-8 flag and everything works, but this change also breaks real string which have utf-8 characters in them.
Comment 13 Chris Cormack 2012-07-19 18:48:18 UTC
(In reply to comment #12)
> I'm getting
> 
> Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 825.
> 
> errors when I try to use label printing with utf-8 characters (with or
> without this patch).
> 
So this patch makes things no worse, and fixes them in one case?

Is it possible to do a follow up which can do decode,or encode depending on certain conditions. Or better yet fix the encoding in other places and then a patch here to switch to encode.

If this patch is not breaking anything more, and is fixing things in one case, it certainly isn't a regression, and so I see no need to revert it.

It does improve things in one situation, until a more proper fix is done.
Comment 14 Dobrica Pavlinusic 2012-07-19 22:04:18 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2012-07-19 22:24:44 UTC
Hmm hardcoded paths ....

I think it's better to kill this code and focus on a css based approach. 

But if this can be made to work with the free (as in freedom) fonts
http://packages.debian.org/squeeze/ttf-liberation or ttf-dejavu-core (which I see you are using) I don't see why it couldn't go in until a better solution is committed.
Comment 16 wajasu 2012-09-13 04:49:53 UTC
Here is an article that helps one see how perl has things internally encoded.
Just for future reference.  
http://blog.twoshortplanks.com/2009/12/02/under-the-hood/
But I'm sure you experts know about this sort of stuff.

I think its worth investigating the truetype option.  Although I agree that some web font css label solution or such may be the worth pursuing.
Comment 17 Paul Poulain 2012-09-17 10:40:42 UTC
(In reply to comment #16)
> Although I agree that
> some web font css label solution or such may be the worth pursuing.

I investigated a lot Perl libraries for managing PDF. I feel we will have a better results investigating the CSS way.
Comment 18 Kyle M Hall 2012-09-21 12:56:42 UTC
I also agree HTML/CSS is far better in the long run than PDF.
Comment 19 Mirko Tietgen 2012-10-19 01:19:26 UTC Comment hidden (obsolete)
Comment 20 Mirko Tietgen 2012-10-19 01:25:07 UTC
Confirming that Dobrica's patch works for the example given by wajasu. Using TrueType fonts makes sense to me, the hardcoded path is ugly though.

Since PDF (and the problems it comes with) is what we have now that should have precedence over what would be nice to have in the future.

What about putting the font path in a syspref instead of hardcoding?
Comment 21 Mason James 2012-11-01 08:10:13 UTC
(In reply to comment #20)
> Confirming that Dobrica's patch works for the example given by wajasu. Using
> TrueType fonts makes sense to me, the hardcoded path is ugly though.
> 
> Since PDF (and the problems it comes with) is what we have now that should
> have precedence over what would be nice to have in the future.
> 


> What about putting the font path in a syspref instead of hardcoding?

+1 agreed, thats a great idea

fyi: the 'ttf-dejavu*' packages need to be added to the debian/control file too
Comment 22 MJ Ray (software.coop) 2013-02-05 11:20:16 UTC
Could we apply this latest patch and open new bugs for the hardcoded paths and packaging changes?
Comment 23 Mason James 2013-03-20 00:56:08 UTC
(In reply to comment #22)
> Could we apply this latest patch and open new bugs for the hardcoded paths
> and packaging changes?

yep, sounds like a good idea. i've created 2 new bugs for the above issues

 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9854
 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9856
Comment 24 Mason James 2013-03-20 01:01:56 UTC
(In reply to comment #23)
> (In reply to comment #22)
> > Could we apply this latest patch and open new bugs for the hardcoded paths
> > and packaging changes?
> 
> yep, sounds like a good idea. i've created 2 new bugs for the above issues
> 
>  http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9854
>  http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9856

2 signed-off patches now waiting for QA...
Comment 25 Dobrica Pavlinusic 2013-03-20 09:47:34 UTC
First patch was already applied to master back in 2012, so I obsoleted it.
Comment 26 Dobrica Pavlinusic 2013-03-20 16:40:10 UTC Comment hidden (obsolete)
Comment 27 Dobrica Pavlinusic 2013-03-20 16:43:03 UTC
I opted to use ttf font mapping in koha-conf.xml since it enables uses to use
non-free msttcorefonts package to get more precise mappings to built-in pdf
fonts (at expense of theirs freedom).

This patch also supports correctly italic, bold and bold-italic. If there are no objections to this approach, I would love to address other two bugs which are related to this patch during this week.
Comment 28 Dobrica Pavlinusic 2013-03-20 16:44:39 UTC
Having said all that, patron cards suffer from same problems as labels do, so
I will open separate bug to fix that if this mapping is good idea.
Comment 29 Chris Cormack 2013-04-07 03:32:28 UTC
Im happy to sign off on this as soon as bug 9856 and bug 9854 have patches to sign off on as well.
Comment 30 Vitor Fernandes 2013-04-19 13:54:39 UTC
This patch works fine for me!
Comment 31 Chris Nighswonger 2013-05-29 21:49:37 UTC
(In reply to comment #29)
> Im happy to sign off on this as soon as bug 9856 and bug 9854 have patches
> to sign off on as well.

It looks like we are still lacking patches to move ttf pathing to a
syspref. RE bugs 9854 and 9856...

Embedding fonts is the only way to fix the problems according to a
software engineer at Adobe who I corresponded with a couple of years
back concerning this very issue. So this sounds like a good thing for
now.

Once patches for the above mentioned bugs are submitted, I'd be glad to sign off on this as well.
Comment 32 Galen Charlton 2013-05-31 20:08:24 UTC
I've switched the dependencies around since folks are requesting patches for 9854 and 9856 before signing off on this one, although I wonder if all three bugs should just be merged.
Comment 33 Chris Nighswonger 2013-06-27 11:26:39 UTC
*** Bug 10512 has been marked as a duplicate of this bug. ***
Comment 34 Paola Rossi 2013-09-25 14:57:54 UTC
Under Debian 7.1 wheezy, the master 3.13.00.021 displays the expected results: in output, the diacritic for ö is correct.

After selecting:
applied template:      Avery 5160 | 1 x 2-5/8
applied layout:        Lable Test
stating lable position:1

and downloading as pdf, as required, the lable of LOC 88126978 biblio is:

Dekonstruktionen, oder, Vom Widersprechen in Bildern :
Schöttler-Boll, Doris,
3792709910
book
333444555666

[Schöttler-Boll, Doris is 100 MARC21 tag].

So I don't sign off the patch.
Comment 35 Kyle M Hall 2013-09-25 18:01:01 UTC Comment hidden (obsolete)
Comment 36 Chris Nighswonger 2013-09-25 18:26:23 UTC
This will, of course, require that the user have the appropriate TTF fonts installed on their system.

We visited this idea some years ago (I had code to do this very thing then), but encountered some resistance to "requiring" the user to install fonts which may not be available in the default install of thier OS. Maybe times have changed, and we no longer have issues with that aspect of the fix.

Here is some historical context into which to fit this problem:

http://comments.gmane.org/gmane.education.libraries.koha.devel/3413/

Also see Bug 2246

FTR: I'm all for embedded ttf fonts as it is the only realistic way to fix the problem.
Comment 37 Paul Poulain 2013-10-21 00:48:29 UTC
(In reply to Paola Rossi from comment #34)
> Under Debian 7.1 wheezy, the master 3.13.00.021 displays the expected
> results: in output, the diacritic for ö is correct.
<snip>
> So I don't sign off the patch.


Paola, I don't understand : you say "the diacritic is correct" then "I don't sign off the patch". Did you wanted to say "I sign off the patch" ?
Comment 38 Paola Rossi 2013-10-21 09:19:31 UTC
(In reply to Paul Poulain from comment #37)
> (In reply to Paola Rossi from comment #34)
> > Under Debian 7.1 wheezy, the master 3.13.00.021 displays the expected
> > results: in output, the diacritic for ö is correct.
> <snip>
> > So I don't sign off the patch.
> 
> 
> Paola, I don't understand : you say "the diacritic is correct" then "I don't
> sign off the patch". Did you wanted to say "I sign off the patch" ?

Kind Paul, I wanted to say:
"I don't sign off the patch"
because the master was already OK without applying the patch: the master didn't show diacritics badly.
I write the details of my test, because the bug could appear in another case, or in another install.
In my opinion, with the test required I couldn't know if the patch fixed a bug or not, neither if it really improved master in some way.
Comment 39 Paul Poulain 2013-10-25 16:46:51 UTC
Kyle, could you confirm that/if the problem exist BEFORE the path please ?
Comment 40 Chris Nighswonger 2013-12-19 23:18:12 UTC
*** Bug 11423 has been marked as a duplicate of this bug. ***
Comment 41 MJ Ray (software.coop) 2014-01-09 16:53:25 UTC
(In reply to Paola Rossi from comment #38)
> Kind Paul, I wanted to say:
> "I don't sign off the patch"
> because the master was already OK without applying the patch: the master
> didn't show diacritics badly.

That diacritic was o-umlaut which is character F6 in the latin-1 character set, wasn't it?  Therefore, I think it wouldn't trigger both aspects of this bug.

You're correct that the test case isn't great in that regard.

I'm still getting "Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 819" when attempting to print a record containing a stroked-l (B3 from latin-2, whatever that is in utf8).  I will investigate further soon but would really appreciate any thoughts from the wise heads here.
Comment 42 Marcel de Rooy 2014-02-18 16:22:30 UTC
Hmm. We have an open question from MJ, going back to January 9.
The patch depends on something that is in status NEW and on another patch that is breaking records for being in PQA (seeing why now..) No strict dependency..
The 9856 dependency does actually not contain any code.

Since the previous comment looks like a "Failed QA", I would like to ask Dobrica or Kyle to respond now.
Changing status in the meantime.
Comment 43 Dobrica Pavlinusic 2014-03-11 10:51:59 UTC
bug 9856 is obsoleted by moving ttf paths into kona-conf.xml which is better place since it involves system paths so it should be under control of system administrators as opposed to end users.
Comment 44 Dobrica Pavlinusic 2014-03-11 15:40:19 UTC
I'm not getting "Wide character in compress" error from PDF::Reuse, but I do get a

Use of uninitialized value within @PDF::Reuse::objekt in sprintf at /usr/share/perl5/PDF/Reuse.pm line 1092

and a bounch of

Use of uninitialized value in addition (+) at /usr/share/perl5/PDF/Reuse.pm line 1286

Bottom one referes to table which exists only for built-in pdf fonts, and upper one would require me to know more about internal pdf structure than I do now.

I don't have solution for those, but from my tests this patch still apply cleanly on current master and works as expected, so I would love to get it in.

WRT, "Wide character in compress" we could probably fix it by digging through PDF::Reuse code and marking something utf-8 (or declaring output to use :raw handler) but in plack we are not really emitting to STDOUT but to scalar.

Do you think that this is show-stopper to get this into master?
Comment 45 Katrin Fischer 2014-03-14 10:09:11 UTC
I was testing this today with Dobrica's help and we are not quite there yet... but I hope some of my notes will be helpful in fixing the remaining issues:

- ttf-dejavu is installed
- Added the following mapping to my koha-conf.xml file:

 <!-- true type font mapping accoding to type from $font_types in C4/Creators/Lib.pm -->
 <ttf>
    <font type="TR" >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSerif.ttf</font>
    <font type="TB" >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSerif-Bold.ttf</font>
    <font type="TI" >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSerif-Italic.ttf</font>
    <font type="TBI">/usr/share/fonts/truetype/ttf-dejavu/DejaVuSerif-BoldItalic.ttf</font>
    <font type="C"  >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSansMono.ttf</font>
    <font type="CB" >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSansMono-Bold.ttf</font>
    <font type="CO" >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSansMono-Oblique.ttf</font>
    <font type="CBO">/usr/share/fonts/truetype/ttf-dejavu/DejaVuSansMono-BoldOblique.ttf</font>
    <font type="H"  >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSans.ttf</font>
    <font type="HB" >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSans-Bold.ttf</font>
    <font type="HBO">/usr/share/fonts/truetype/ttf-dejavu/DejaVuSans-BoldOblique.ttf</font>
 </ttf>

- Created a batch with 2 items, one German with umlauts and the other Arabic.
- Tried exporting the batch into PDF:

-- Times New Roman: PDF opens, but the Arabic characters show up as little squares, the German record is correctly printed

-- Helvetica: PDF cannot be opened, if you open it in a text editor, you can see the usual error:
<pre>Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 819
Comment 46 Chris Nighswonger 2014-03-14 13:38:23 UTC
(In reply to Katrin Fischer from comment #45)

> - ttf-dejavu is installed

> 
> -- Times New Roman: PDF opens, but the Arabic characters show up as little
> squares...

It appears that DejaVu does not support Arabic ATM:

"Following scripts aren't available in all the styles:

    Hebrew
    Arabic
    N'ko
    Lao
    Canadian Aboriginal Syllabics
    Ogham
    Tifinagh
    Lisu" [1]

I believe you will need to install a font which supports whatever language you would like to generate pdfs in.


[1] http://dejavu-fonts.org/wiki/index.php?title=Main_Page
Comment 47 Bernardo Gonzalez Kriegel 2014-04-09 00:55:35 UTC
Today I re-invented this fix trying to answer a mail :(

Comments:

1. Of DejaVu fonts, only Sans version has a good collection of chars,
so we can advice to use only Helvetica if you want Arabic for example.
It's not a problem of the proposed solution, but a shortcoming of the
available font. I think it's the best we can do for now.

2. Helvetica (DejaVuSans) PDF fails because the wrong code is passed to the file selection on Creators/PDF.pm, I will add a followup

3. Have you tried changing justification on layout? The result is wrong.
I will add another followup.
Comment 48 Bernardo Gonzalez Kriegel 2014-04-09 00:56:38 UTC Comment hidden (obsolete)
Comment 49 Bernardo Gonzalez Kriegel 2014-04-09 00:57:28 UTC Comment hidden (obsolete)
Comment 50 Bernardo Gonzalez Kriegel 2014-04-09 01:35:35 UTC
Created attachment 26907 [details]
pic2

another pic
Comment 51 MJ Ray (software.coop) 2014-04-09 17:06:44 UTC
Is there any reason not to fix the alignment?  In other words, could we combine the two patches?
Comment 52 Bernardo Gonzalez Kriegel 2014-04-09 17:34:34 UTC
(In reply to MJ Ray (software.coop) from comment #51)
> Is there any reason not to fix the alignment?  In other words, could we
> combine the two patches?

Yes, of course.
Comment 53 Bernardo Gonzalez Kriegel 2014-04-09 17:42:07 UTC Comment hidden (obsolete)
Comment 54 Bernardo Gonzalez Kriegel 2014-04-10 14:23:48 UTC
Created attachment 26976 [details]
Bad right-to-left text
Comment 55 Bernardo Gonzalez Kriegel 2014-04-10 14:24:18 UTC
Created attachment 26977 [details]
Good right-to-left text
Comment 56 Bernardo Gonzalez Kriegel 2014-04-10 14:25:28 UTC Comment hidden (obsolete)
Comment 57 Chris Nighswonger 2014-04-10 14:41:52 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #56)
> 
> This patch will try to fix those cases adding a new
> perl dependency, Text::Bidi, and using the automagic
> feature if it's log2vis() function to rearrange chars
> based on detected text 'direction'

This should probably have its own bug since it adds a dep. and is really unrelated to diacrticals.
Comment 58 Bernardo Gonzalez Kriegel 2014-04-10 17:49:19 UTC
(In reply to Chris Nighswonger from comment #57)
> This should probably have its own bug since it adds a dep. and is really
> unrelated to diacrticals.

Ok, Bug 12068
Comment 59 Chris Nighswonger 2014-04-17 12:19:09 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #53)
> Created attachment 26928 [details] [review] [review]
> Bug 8375 - Followup - Fix font code and alignment
> 
> This follow up fixes font code taking it from layout,
> and alignment.

I see no obvious problem with the using prText for alignment (thought I have a nagging feeling there were some issues with it), however, this fix is applied in the wrong location in the code. The text justification mechanism is housed at the module level in C4::Labels::Label[1] and fixes should probably be applied there or else things will turn to spaghetti pretty fast.

Also, its not apparent how this fix interacts with the existing justification code, if at all, which makes it further desirable to rewrite/replace it. Although this patch may fix the immediately apparent problem, there may be unexpected hidden side effects because of this.

[1]http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Labels/Label.pm;h=cb6a631d9ffbbeca1f96254352575503ec93f861;hb=ffd90a1564328add63ce376a15b097707d5d66be#l443
Comment 60 Bernardo Gonzalez Kriegel 2014-04-20 23:48:29 UTC Comment hidden (obsolete)
Comment 61 Bernardo Gonzalez Kriegel 2014-04-20 23:59:45 UTC
(In reply to Chris Nighswonger from comment #59)
> 
> Also, its not apparent how this fix interacts with the existing
> justification code, if at all, which makes it further desirable to
> rewrite/replace it. Although this patch may fix the immediately apparent
> problem, there may be unexpected hidden side effects because of this.
> 

If found a solution that use current code :)
Comment 62 Chris Nighswonger 2014-04-21 12:04:31 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #61)
> (In reply to Chris Nighswonger from comment #59)
> > 
> > Also, its not apparent how this fix interacts with the existing
> > justification code, if at all, which makes it further desirable to
> > rewrite/replace it. Although this patch may fix the immediately apparent
> > problem, there may be unexpected hidden side effects because of this.
> > 
> 
> If found a solution that use current code :)

I think you may be misunderstanding me.

Using your proposed fix will result in justification code in multiple places. This is bad a practice otherwise known as spaghetti code[1]. It also makes the "fix" inaccessible via the C4::Labels module by placing it at the script level. This is bad practice as no other scripts (existing or future) will have access to this "fix."

Unless this is corrected, I will (as the module maintainer) be forced to hold this bug in "failed-QA" status.

Just to clarify: I have no issue with the proposed fix in principle. We just need to put it into the correct place in the code.

[1]http://www.computerhope.com/jargon/s/spaghett.htm
Comment 63 Bernardo Gonzalez Kriegel 2014-04-21 12:17:48 UTC
(In reply to Chris Nighswonger from comment #62)
> 
> I think you may be misunderstanding me.
> 
> Using your proposed fix will result in justification code in multiple
> places. This is bad a practice otherwise known as spaghetti code[1]. It also
> makes the "fix" inaccessible via the C4::Labels module by placing it at the
> script level. This is bad practice as no other scripts (existing or future)
> will have access to this "fix."
> 
> Unless this is corrected, I will (as the module maintainer) be forced to
> hold this bug in "failed-QA" status.
> 
> Just to clarify: I have no issue with the proposed fix in principle. We just
> need to put it into the correct place in the code.
>

Chris,
I changed completely the solution. Please, take a look (second patch)
The fix IS in Label.pm, that was the meaning of 'using current code.'

Your were right, I was bypassing current justification code, and that was wrong.

I found the real problem (non-existent font code and bad parameter to StrWidth) and fixed that.
Comment 64 Chris Nighswonger 2014-04-21 13:00:03 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #63)
> (In reply to Chris Nighswonger from comment #62)
> > 
> > I think you may be misunderstanding me.
> I changed completely the solution. Please, take a look (second patch)
> The fix IS in Label.pm, that was the meaning of 'using current code.'

Ouch! My bad. I apologize for my misunderstanding.

> 
> I found the real problem (non-existent font code and bad parameter to
> StrWidth) and fixed that.

I'll take a look at it now and sign off.
Comment 65 Bernardo Gonzalez Kriegel 2014-04-21 14:12:47 UTC
(In reply to Chris Nighswonger from comment #64)
> 
> Ouch! My bad. I apologize for my misunderstanding.

No problem :)

> 
> I'll take a look at it now and sign off.

Thinking about the patch, perhaps $fontCode/fontObject (or font_code/font_object) are better than $fontName, I can change that (or you, if you like)
StrWidth was receiving 'TI' or 'HO' for example, not Ft1/Ft2 for embedded fonts, nor 'Times-Italic' or 'Helvetica-Oblique', in that way it use Helvetica, which is approximately good for latin scripts[1]

[1] http://search.cpan.org/dist/PDF-Reuse/Reuse.pm#prStrWidth_-_calculate_the_string_width
Comment 66 Chris Nighswonger 2014-04-21 17:57:47 UTC
Created attachment 27363 [details]
Arabic records to test with
Comment 67 Chris Nighswonger 2014-04-23 13:52:40 UTC Comment hidden (obsolete)
Comment 68 Chris Nighswonger 2014-04-23 13:55:17 UTC Comment hidden (obsolete)
Comment 69 Chris Nighswonger 2014-04-23 13:56:16 UTC
Created attachment 27483 [details] [review]
[SIGNED-OFF] Bug 8375 - use truetype fonts in PDF::Reuse

Since built-in PDF fonts suport just latin-1 encoding, we have
to switch to truetype fonts to correctly encode all utf-8 characters
(which we should be getting from database anyway).

This approach also nicely sidesteps our encoding cludges, but
requires paths to truetype fonts which are included in koha-conf.xml
under new <ttf> section. Without this directive in kona-conf.xml
code will still use latin-1 built-in pdf fonts.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Comment 70 Chris Nighswonger 2014-04-23 13:56:29 UTC
Created attachment 27484 [details] [review]
[SIGNED-OFF] Bug 8375 - Followup - Fix font code and alignment

This patch fixes two problems:
a) Bad PDF when using Helvetica font.
Current label code assigns 'italic' or 'oblique' variants
to title. Helvetica-Oblique was not defined, but is present

b) Bad alignment using center/right justification
Problem was bad font parameter passed to StrWidth
routine

To test:
1. Try making a batch using Helvetica, downloaded PDF do not open.
2. Try a batch of mixed scripts with layout alignment center or right, only latin scripts align almost correctly.
3. Apply the patch and update your koha-conf.xml to add Oblique variant
4. Try again 1, now PDF opens
5. Try 2, now alignment is correct

New problem (for another bug): DejaVuSans has a good
support for arabic, but not Oblique variant. As selection
of italic/oblique is hardcoded, now Arabic titles are
not displayed. I'll try to add a checkbox to select
or not this feature.

Added a FIXME for the hardcoded forced oblique -chris_n

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Comment 71 Bernardo Gonzalez Kriegel 2014-04-23 14:52:33 UTC
Can be changed to 'Signed-off'?
Comment 72 Chris Nighswonger 2014-04-23 15:48:44 UTC
This works as described for me.
Comment 73 Jonathan Druart 2014-05-02 10:45:34 UTC
I never used the label creator tools, but it does not seem to work for me (tested with my UNIMARC DB and with my MARC21 using the "Arabic records" in attachment).

I got badly encoded characters for barcodes 50 and 51 (see attachment).

Moreover, I don't really understand why the usefulness of the font list in the koha-conf file. The font list already exist in a pl, why not just put the directory path (/usr/share/fonts/truetype/ttf-dejavu)? If a font is added in the koha-conf, it won't be added to the list in the Koha interface, which is quite odd.
Comment 74 Chris Nighswonger 2014-05-02 11:19:10 UTC
(In reply to Jonathan Druart from comment #73)
> I never used the label creator tools, but it does not seem to work for me
> (tested with my UNIMARC DB and with my MARC21 using the "Arabic records" in
> attachment).
> 
> I got badly encoded characters for barcodes 50 and 51 (see attachment).

I seem to be missing the attachment. Please give more description of your test procedure as well.

> 
> Moreover, I don't really understand why the usefulness of the font list in
> the koha-conf file. The font list already exist in a pl, why not just put
> the directory path (/usr/share/fonts/truetype/ttf-dejavu)? If a font is
> added in the koha-conf, it won't be added to the list in the Koha interface,
> which is quite odd.

We only support the hardcoded fonts ATM. It would be good to fix the problem of hard coding but in another bug.
Comment 75 Jonathan Druart 2014-05-02 12:17:33 UTC
(In reply to Chris Nighswonger from comment #74)
> I seem to be missing the attachment. Please give more description of your
> test procedure as well.

Yep, sorry, it's coming.
I imported the record, go on "Currently Available Layouts", edit the first layout, type:"Barcode/biblio".
Manage batches > new batch > add items barcode 50, 51 > Export batch > Export > Download as pdf.
And I got it, see attachment (I won't forget this time!).
(I updated the koha-conf.xml, of course).
Comment 76 Jonathan Druart 2014-05-02 12:18:07 UTC
Created attachment 27917 [details]
pdf broken with barcode 50&51
Comment 77 Chris Nighswonger 2014-05-02 14:05:07 UTC
(In reply to Jonathan Druart from comment #76)
> Created attachment 27917 [details]
> pdf broken with barcode 50&51

With the attached records you will need to use the following fields:

245a, author, itemtype, itemcallnumber
Comment 78 Chris Nighswonger 2014-05-02 14:07:26 UTC
(In reply to Jonathan Druart from comment #76)
> Created attachment 27917 [details]
> pdf broken with barcode 50&51

Also check to be sure your ttf config tags fall inside of the <config> tags in koha-conf.xml
Comment 79 Bernardo Gonzalez Kriegel 2014-05-02 14:15:24 UTC
(In reply to Jonathan Druart from comment #75)
> (In reply to Chris Nighswonger from comment #74)
> > I seem to be missing the attachment. Please give more description of your
> > test procedure as well.
> 
> Yep, sorry, it's coming.
> I imported the record, go on "Currently Available Layouts", edit the first
> layout, type:"Barcode/biblio".
> Manage batches > new batch > add items barcode 50, 51 > Export batch >
> Export > Download as pdf.
> And I got it, see attachment (I won't forget this time!).
> (I updated the koha-conf.xml, of course).

Seems that you are using Times-Roman in Layout, please confirm.
If that is the case, what you view is the expected result, because there are no glyphs for that chars in DejaVuSerif. Using Helvetica will give you something better, mapping HO to DejaVuSans better, and with Bug 12063 correct display

(See new attached image)
Comment 80 Bernardo Gonzalez Kriegel 2014-05-02 14:16:10 UTC
Created attachment 27918 [details]
Display variations
Comment 81 Bernardo Gonzalez Kriegel 2014-05-02 14:17:45 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #79)
> something better, mapping HO to DejaVuSans better, and with Bug 12063

Sorry, Bug 12068
Comment 82 Jonathan Druart 2014-05-02 14:18:08 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #79)
> Seems that you are using Times-Roman in Layout, please confirm.
haaa! Ok thanks. I used Times-Roman.
Comment 83 Jonathan Druart 2014-05-02 14:40:14 UTC
hum, weird, I got
ERROR in koha-conf.xml -- missing <font type="HBOO">/path/to/font.ttf</font> at /home/koha/src/C4/Creators/PDF.pm line 123ERROR in koha-conf.xml -- missing <font type="HBOO">/path/to/font.ttf</font> at /home/koha/src/C4/Creators/PDF.pm line 123

when I select "Helvetica-Bold-Oblique".
The value is correct in DB:
  font: HBO
Comment 84 Bernardo Gonzalez Kriegel 2014-05-02 14:48:18 UTC
(In reply to Jonathan Druart from comment #83)
> hum, weird, I got
> ERROR in koha-conf.xml -- missing <font type="HBOO">/path/to/font.ttf</font>
> at /home/koha/src/C4/Creators/PDF.pm line 123ERROR in koha-conf.xml --
> missing <font type="HBOO">/path/to/font.ttf</font> at
> /home/koha/src/C4/Creators/PDF.pm line 123
> 
> when I select "Helvetica-Bold-Oblique".
> The value is correct in DB:
>   font: HBO

Problem is that current code appends an 'O' to selected font for title
C4/Labels/Label.pm:400:        
($field->{'code'} eq 'title') ? (($font =~ /T/) ? ($font = 'TI') : ($font = ($font . 'O'))) : ($font = $font);

If you select HBO as main font, then it would search for HBOO for title
Comment 85 Galen Charlton 2014-05-05 21:57:14 UTC
In this case, I agree with Chris.  Since we have two independent signoffs, I've gone ahead and pushed this.  Thanks, Dobrica!
Comment 86 Jonathan Druart 2014-05-06 07:46:59 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #84)
> Problem is that current code appends an 'O' to selected font for title
> C4/Labels/Label.pm:400:        
> ($field->{'code'} eq 'title') ? (($font =~ /T/) ? ($font = 'TI') : ($font =
> ($font . 'O'))) : ($font = $font);
> 
> If you select HBO as main font, then it would search for HBOO for title

Bernardo, what meant this comment? Is there a bug somewhere? I get the error on master now (with the patches).
Comment 87 Bernardo Gonzalez Kriegel 2014-05-06 11:32:52 UTC
(In reply to Jonathan Druart from comment #86)
> (In reply to Bernardo Gonzalez Kriegel from comment #84)
> > Problem is that current code appends an 'O' to selected font for title
> > C4/Labels/Label.pm:400:        
> > ($field->{'code'} eq 'title') ? (($font =~ /T/) ? ($font = 'TI') : ($font =
> > ($font . 'O'))) : ($font = $font);
> > 
> > If you select HBO as main font, then it would search for HBOO for title
> 
> Bernardo, what meant this comment? Is there a bug somewhere? I get the error
> on master now (with the patches).

Yes. It'a bug, but a pre-existent one, and it makes you try to use nonexistent fonts.
I can add a patch, but for now do not select CO, CBO, HO or HBO for main font

Can be fixed changing line 400 of C4/Labels/Label.pm with

if ( $field->{'code'} eq 'title' ) {
    if ( $font =~ /^(T|TB)$/             ) { $font .= 'I'; }
    if ( $font !~ /^T/ and $font !~ /O$/ ) { $font .= 'O'; }
}

or something better
Comment 88 Chris Nighswonger 2014-05-06 12:56:36 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #87)
> (In reply to Jonathan Druart from comment #86)
> > (In reply to Bernardo Gonzalez Kriegel from comment #84)
> > > Problem is that current code appends an 'O' to selected font for title
> > > C4/Labels/Label.pm:400:        
> > > ($field->{'code'} eq 'title') ? (($font =~ /T/) ? ($font = 'TI') : ($font =
> > > ($font . 'O'))) : ($font = $font);
> > > 
> > > If you select HBO as main font, then it would search for HBOO for title
> > 
> > Bernardo, what meant this comment? Is there a bug somewhere? I get the error
> > on master now (with the patches).
> 
> Yes. It'a bug, but a pre-existent one, and it makes you try to use
> nonexistent fonts.
> I can add a patch, but for now do not select CO, CBO, HO or HBO for main font
> 
> Can be fixed changing line 400 of C4/Labels/Label.pm with
> 
> if ( $field->{'code'} eq 'title' ) {
>     if ( $font =~ /^(T|TB)$/             ) { $font .= 'I'; }
>     if ( $font !~ /^T/ and $font !~ /O$/ ) { $font .= 'O'; }
> }
> 
> or something better

This code really needs to be revisited. It would be great if Jonathan coould open a bug for it. I'll try to get to it in the next week or so.
Comment 89 Jonathan Druart 2014-05-06 13:30:55 UTC
(In reply to Chris Nighswonger from comment #88)
> This code really needs to be revisited. It would be great if Jonathan coould
> open a bug for it. I'll try to get to it in the next week or so.

Bug 12194 created.
Comment 90 Chris Nighswonger 2014-05-19 13:29:03 UTC
*** Bug 12286 has been marked as a duplicate of this bug. ***
Comment 91 Barton Chittenden 2015-03-26 14:48:43 UTC
Under 3.19.00.016, printing the title field containing RIGHT SINGLE QUOTATION MARK (UTF-8 0xe28099) causes the following text to be written to the PDF file.

<h1>Software error:</h1>
<pre>Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 820.
</pre>
<p>
For help, please send mail to the webmaster (<a href="mailto:staff@bywatersolutions.com">staff@bywatersolutions.com</a>), giving this error message 
and the time and date of the error.

</p>
Comment 92 Barton Chittenden 2015-03-26 14:53:18 UTC
Created attachment 37279 [details]
Marc file containing record with UTF-8
Comment 93 Barton Chittenden 2015-03-26 15:03:49 UTC
Steps to re-create bug:

1) Import 'Marc file containing record with UTF-8', create item record.
   [the record has a title containing 0xe28099 ]
2) Go to Tools->Labels, click 'New Batch' button.
3) Add barcode of item created in step 1.
4) Click 'Manage Batches'
5) Select the check-box for the new batch
6) Click 'Export'
7) Select a layout which contains the 'Title' field
8) Click 'Export'
9) Click 'Download as PDF'

The exported PDF will read as follows:

<h1>Software error:</h1>
<pre>Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 820.
</pre>
<p>
For help, please send mail to the webmaster (<a href="mailto:staff@bywatersolutions.com">staff@bywatersolutions.com</a>), giving this error message 
and the time and date of the error.

</p>
Comment 94 Chris Nighswonger 2015-03-26 15:43:17 UTC
(In reply to Barton Chittenden from comment #91)
> Under 3.19.00.016, printing the title field containing RIGHT SINGLE
> QUOTATION MARK (UTF-8 0xe28099) causes the following text to be written to
> the PDF file.
> 
> <h1>Software error:</h1>
> <pre>Wide character in compress at /usr/share/perl5/PDF/Reuse.pm line 820.
> </pre>
> <p>
> For help, please send mail to the webmaster (<a
> href="mailto:staff@bywatersolutions.com">staff@bywatersolutions.com</a>),
> giving this error message 
> and the time and date of the error.
> 
> </p>

This is unrelated to this bug, Please open a new bug for it.
Comment 95 Bernardo Gonzalez Kriegel 2015-03-26 16:46:24 UTC
Created attachment 37285 [details]
UTF-8 Quotes

Hi, on current master it works for me (see the screenshot)
Comment 96 ahmed123 2018-11-21 11:27:25 UTC
Comment on attachment 26977 [details]
Good right-to-left text

how do RTL In Label