Bug 12068 - label-create-pdf.pl Add support for RTL languages
Summary: label-create-pdf.pl Add support for RTL languages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bernardo Gonzalez Kriegel
QA Contact: Testopia
URL:
Keywords:
Depends on: 8375
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-10 17:40 UTC by Bernardo Gonzalez Kriegel
Modified: 2015-12-03 22:11 UTC (History)
7 users (show)

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


Attachments
Bug 12068 - label-create-pdf.pl Add support for RTL language (4.12 KB, patch)
2014-04-10 17:47 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12068 - label-create-pdf.pl Add support for RTL language (4.32 KB, patch)
2014-04-21 00:32 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12068 - label-create-pdf.pl Add support for RTL language (4.61 KB, patch)
2014-04-23 21:28 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12068 - label-create-pdf.pl Add support for RTL language (4.63 KB, patch)
2014-08-13 21:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 12068 - label-create-pdf.pl Add support for RTL language (4.71 KB, patch)
2014-08-14 15:31 UTC, Karam Qubsi
Details | Diff | Splinter Review
pdf exported file with Arabic items (19.10 KB, application/pdf)
2014-08-14 15:41 UTC, Karam Qubsi
Details
Bug 12068 - label-create-pdf.pl Add support for RTL language (4.85 KB, patch)
2014-10-11 16:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12068: Follow up - make Text::Bidi mandatory (864 bytes, patch)
2014-10-23 05:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12068: Follow up - make Text::Bidi mandatory (921 bytes, patch)
2014-10-23 05:28 UTC, Chris Cormack
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 2014-04-10 17:40:28 UTC
On top of Bug 8375

Right-to-left languages are not properly displayed on PDF,
chars are presented in logical order from left to right
Comment 1 Bernardo Gonzalez Kriegel 2014-04-10 17:47:34 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2014-04-21 00:32:52 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2014-04-23 21:28:42 UTC Comment hidden (obsolete)
Comment 4 Bernardo Gonzalez Kriegel 2014-04-23 22:19:08 UTC
When I run koha-qa, got something strange

 FAIL   C4/Labels/Label.pm
   OK     pod
   OK     forbidden patterns
   FAIL   valid
                Name "swig_runtime_data::type_pointer3" used only once: possible 

 FAIL   labels/label-create-pdf.pl
   OK     pod
   OK     forbidden patterns
   FAIL   valid
                Name "swig_runtime_data::type_pointer3" used only once: possible typo

Perhaps only my machine, can't understand :(
Comment 5 Bernardo Gonzalez Kriegel 2014-04-24 00:39:09 UTC
Seems to be something related with Text::Bidi library,
see http://www.cpantesters.org/cpan/report/cd6c2fe8-da0a-11e0-bbec-9aeec8b7ec30
Comment 6 Bernardo Gonzalez Kriegel 2014-08-09 13:07:35 UTC
If you run 

perl -cwe "use Text::Bidi;"

then the output is

Name "swig_runtime_data::type_pointer3" used only once: possible typo at /usr/lib/perl/5.14/DynaLoader.pm line 207.
-e syntax OK

That's the reason koha-qa complains.
Can't find another lib to replace Text::Bidi, and without it we can't print RTL labels correctly

Patch still valid, save for small conflict on PerlDependencies.pm
Comment 7 Bernardo Gonzalez Kriegel 2014-08-13 21:17:32 UTC Comment hidden (obsolete)
Comment 8 Bernardo Gonzalez Kriegel 2014-08-13 21:37:33 UTC
Another comment about the 'version' of Text::Bidi

On Ubuntu 12.04 it's version is 0.03-5build2, on Debian sqeeze and weezy is also 0.03-5, but on Ubuntu 14.04 it's now 2.08, and for Debian Jessie it's 2.09

With 2.08 test does not fail!!
Comment 9 Karam Qubsi 2014-08-14 15:31:20 UTC Comment hidden (obsolete)
Comment 10 Karam Qubsi 2014-08-14 15:41:18 UTC
Created attachment 30793 [details]
pdf exported file with Arabic items

as you can see everything is perfect here :)
Comment 11 Mark Tompsett 2014-08-28 21:32:52 UTC
> the fonts in koha-conf.xml are modified as the following 
>     <font type="H" 
> >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSans.ttf</font>
>     <font type="HO"
> >/usr/share/fonts/truetype/ttf-dejavu/DejaVuSans.ttf</font>

Why isn't it:
    <font type="HO" /usr/share/fonts/truetype/ttf-dejavu/DejaVuSans-Oblique.ttf</font>

instead?
Comment 12 Bernardo Gonzalez Kriegel 2014-08-30 13:18:43 UTC
(In reply to M. Tompsett from comment #11)
> 
> Why isn't it:
>     <font type="HO"
> /usr/share/fonts/truetype/ttf-dejavu/DejaVuSans-Oblique.ttf</font>
> 
> instead?

Problem is DejaVuSans-Oblique lack of Arabic glyphs :(
Only DejaVuSans have a decent support for them.

This is only a workaround to print Arabic labels. 

Correct solution is add an option to label layout to use o not Oblique/Italic variant for title.
Comment 13 Katrin Fischer 2014-10-11 16:02:18 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2014-10-21 19:17:48 UTC
Patch pushed to master.

Thanks Bernardo!
Comment 15 Chris Cormack 2014-10-23 05:08:26 UTC
t/00-load.t ......................... 2/? 
#   Failed test 'use C4::Labels;'
#   at t/00-load.t line 33.
#     Tried to use 'C4::Labels'.
#     Error:  Can't locate Text/Bidi.pm in @INC (you may need to install the Text::Bidi module) (@INC contains: /etc/perl /usr/local/lib/perl/5.18.2 /usr/local/share/perl/5.18.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.18 /usr/share/perl/5.18 /usr/local/lib/site_perl .) at C4/Labels/Label.pm line 11.
# BEGIN failed--compilation aborted at C4/Labels/Label.pm line 11.


Tests now fail, 2 options, either we make Text::Bidi mandatory (it's optional at the moment) or we switch C4/Labels to do a conditional load instead of a use.

What would you prefer? I can do a patch for either.
Comment 16 Katrin Fischer 2014-10-23 05:11:13 UTC
Ah, I had done a patch to make it mandatory but forgot to attach it here :( Not sure which solution is better.
Comment 17 Chris Cormack 2014-10-23 05:13:04 UTC
I feel like mandatory is probably the right thing, its a packaged module. If you add the patch ill sign it off
Comment 18 Katrin Fischer 2014-10-23 05:23:50 UTC Comment hidden (obsolete)
Comment 19 Chris Cormack 2014-10-23 05:28:03 UTC
Created attachment 32615 [details] [review]
Bug 12068: Follow up - make Text::Bidi mandatory

Without the module some of the tests will fail. As this is
fixing a serious bug for RTL language users, we agreed
to make it mandatory.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 20 Katrin Fischer 2014-10-24 05:34:27 UTC
Switching to "PQA" as the patch was intended to be QA follow-up and only changes one character.
Comment 21 Tomás Cohen Arazi 2014-10-28 14:14:05 UTC
Followup pushed to master.

Thanks Katrina!
Comment 22 Katrin Fischer 2014-10-28 14:17:19 UTC
Thx Bernardo! :)
Comment 23 Marcel de Rooy 2014-10-30 21:25:43 UTC
Can't locate ExtUtils/PkgConfig.pm in @INC (@INC contains: /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5 .) at Makefile.PL line 9.

It seems that fribidi is missing.

fribidi should be installed prior to the installation of this module.
Comment 24 Katrin Fischer 2014-10-30 21:27:40 UTC
Hi Marcel, did you install Text::Bidi? It's a new dependency here.
Comment 25 Marcel de Rooy 2014-10-31 07:30:05 UTC
(In reply to Katrin Fischer from comment #24)
> Hi Marcel, did you install Text::Bidi? It's a new dependency here.

Yes, that is actually my problem.
I am not sure if this should have been marked as required btw..
Comment 26 Katrin Fischer 2014-11-01 09:11:15 UTC
Hi Marcel, can you explain?
Comment 27 Marcel de Rooy 2014-11-03 11:11:00 UTC
(In reply to Katrin Fischer from comment #26)
> Hi Marcel, can you explain?

Installing Text::Bidi gives headaches:

0) I installed a fribidi with yum already (not on debian!)
1) After last error, installed ExtUtils::PkgConfig
2) Package fribidi was not found in the pkg-config search path. Made a fribidi.pc file
3) After several warnings and editing several lines in this fribidi.pc, I came one step further: 'YAML' not installed, will not store persistent state.
4) After installing YAML, got this fatal error: fribidi.h: No such file or directory. The installed fribidi appears to be a problem.

So I am still stuck with this. Since I am not using this feature at all, I would not like to waste much more time on it..

Not sure if other people on other distros will have similar problems.
Comment 28 Katrin Fischer 2014-11-03 11:14:27 UTC
Hi Marcel, 

hope I didn't miss it, but which distro are you using?

I think all I did was something like 'sudo apt-get install libtext-bidi-perl' - as it seemed to be ok on Ubuntu and Debian, I didn't see a problem here when we discussed the patch.
Comment 29 Katrin Fischer 2014-11-03 12:17:13 UTC
(12:15:07) marcelr: cait: fedora
Comment 30 David Cook 2015-04-07 04:02:01 UTC
(In reply to M. de Rooy from comment #27)
>
> Not sure if other people on other distros will have similar problems.

I've just run into this problem today while using OpenSuse 13.2, and I'm pretty sure I ran into this problem in the past as well.

The problem is that you're missing the headers for the fribidi library. If you do a "ls /usr/include/fribidi/fribidi.h", I suspect that you'll get nothing. Look for a package like "fribidi-devel" and install that, as that will include the headers that Text::Bidi needs when it's compiling code.

You can see some of that in the guts of the Text::Bidi Makefile: http://cpansearch.perl.org/src/KAMENSKY/Text-Bidi-2.09/Makefile.PL

I just installed "fribidi" and "fribidi-devel", and now I've managed to install Text::Bidi on OpenSuse 13.2.

The building and testing took a long time, so be patient with it, but it should install once you have those fribidi C headers installed :).