[Poppler-bugs] [Bug 55977] handling of rtl text inversion is too naive

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=55977

--- Comment #5 from alex <alexbodn.groups at gmail.com> ---
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Hi, thanks for the contribution!
> > 
> > hello, thanks a lot for considering my patch. and more for your work on this
> > great project.
> > > 
> > > Some weak points of the patch (have not really looked at the contents yet,
> > > just at the form):
> > >  * You are missing support for the cmake build system
> > i don't know cmake, but will happily add support for it. how and where?
> 
> Have a look at CMakeLists.txt and the various find modules at cmake/
> 
> > >  * You are exposing the #if HAVE_ICU in TextOutputDev.h (for a variable i
> > > never see you assign or change)
> > the reorderingMode enum has values taken from an icu header. indeed the enum
> > declaration itself should depend on have_icu. will fix.
> 
> But you don't use the enum anywhere, do you? I mean i only see the same
> value being passed everywhere
> 
> > >  * If i have icu and fribidi it will say that both are used, when that's not
> > > true (please switch it to something like the cms check that says "with lcms"
> > > or "with lcms2" depending on the library used)
> > indeed, this might need some thought in configure.ac. like trying fribidi
> > only if icu is missing. this is the behaviour in the TextOutputDev anyway.
> > what would you think?
> 
> Yes, makes sense

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Hi, thanks for the contribution!
> > 
> > hello, thanks a lot for considering my patch. and more for your work on this
> > great project.
> > > 
> > > Some weak points of the patch (have not really looked at the contents yet,
> > > just at the form):
> > >  * You are missing support for the cmake build system
> > i don't know cmake, but will happily add support for it. how and where?
> 
> Have a look at CMakeLists.txt and the various find modules at cmake/
i've looked there and managed to find modules for icu and fribidi.
but cmakelists.txt needs more expertise, so i've only gave it a dry try. please
help here.
> 
> > >  * You are exposing the #if HAVE_ICU in TextOutputDev.h (for a variable i
> > > never see you assign or change)
> > the reorderingMode enum has values taken from an icu header. indeed the enum
> > declaration itself should depend on have_icu. will fix.
> 
> But you don't use the enum anywhere, do you? I mean i only see the same
> value being passed everywhere
oh, it's being used as a new optional arg for some methods. my new patch is
better in this respect too.
> 
> > >  * If i have icu and fribidi it will say that both are used, when that's not
> > > true (please switch it to something like the cms check that says "with lcms"
> > > or "with lcms2" depending on the library used)
> > indeed, this might need some thought in configure.ac. like trying fribidi
> > only if icu is missing. this is the behaviour in the TextOutputDev anyway.
> > what would you think?
> 
> Yes, makes sense
did it too in this new patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20121016/244ef513/attachment.html>