<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - handling of rtl text inversion is too naive" href="https://bugs.freedesktop.org/show_bug.cgi?id=55977#c5">Comment # 5</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - handling of rtl text inversion is too naive" href="https://bugs.freedesktop.org/show_bug.cgi?id=55977">bug 55977</a> from <a class="email" href="mailto:alexbodn.groups@gmail.com" title="alex <alexbodn.groups@gmail.com>"> alex</a> <pre>(In reply to <a href="show_bug.cgi?id=55977#c4">comment #4</a>) > (In reply to <a href="show_bug.cgi?id=55977#c3">comment #3</a>) > > (In reply to <a href="show_bug.cgi?id=55977#c2">comment #2</a>) > > > 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 <a href="show_bug.cgi?id=55977#c4">comment #4</a>) > (In reply to <a href="show_bug.cgi?id=55977#c3">comment #3</a>) > > (In reply to <a href="show_bug.cgi?id=55977#c2">comment #2</a>) > > > 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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>