[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 #57 from Adam Reichold <adam.reichold at t-online.de> ---
(In reply to Khaled Hosny from comment #56)
> I’m attaching a patch that fixes just 1), based slightly on the latest
> attached patch, but without all the ICU stuff and with providing a
> reorderText() function similar to the non-ICU dumpReorderedText(). There is
> code duplication here that I don’t feel easy about, but trying to use one
> function for both cases results on lots of if conditions that makes the
> resulting code very unreadable, so I’m not sure which is better.
> 
> I think this patch can go on, while the merits of introducing a huge
> dependency like ICU for such a minor gain is assessed.

I do agree with Khaled's assessment of main issue being search not reordering
at all. I also like the patch as it will also allow us to easily introduce
other reordering algorithms in the style of the original patch by splitting it
out into helper functions.

>From a purely technical point of view, I would like to completely remove any
hand-written reordering code as it only duplicates what libraries like ICU
provide and incompletely at that. But then shoehorning this in by converting
back and forth between Poppler's and ICU's representations in those helper
functions also seems like a stopgap measure and a proper solution would
probably involve aligning or even replacing Poppler's internal text
representation with that of ICU (or whatever other library one chooses to use).
Given the inertia of a code base like Poppler's, this seems rather improbable
to happen any time so.

So my guess is that we should use Khaled's patch and leave ICU out for now so
that the user's immediate issue is resolved.

-- 
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/20151118/57297886/attachment.html>