Text wrapping is wrong for RTL languages #189

Closed
artemp opened this Issue Oct 11, 2011 · 21 comments

Comments

Projects
None yet
5 participants
@artemp
Member

artemp commented Oct 11, 2011

This was originally raised on the OSM trac (http://trac.openstreetmap.org/ticket/1515) and an example of the problem can be seen at http://www.openstreetmap.org/?lat=31.42411&lon=34.33985&zoom=16&layers=B000FTF.

When a label such as "مخيّم دير البلح" which is written in a right-to-left language like Arabic is rendered and has to be split across multiple lines the line splitting is done as if it was a left-to-right language.

So in this case it puts "البلح" on the first line and "مخيّم دير" on the second, but the first word of that name is actually "مخيّم" as this is an RTL language, so it is that which should be on the first line, and the rest of the text on the second line.

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[artem] Good catch. Yes, it needs fixing. Jon Burgess pointed me to [http://vimgadgets.sourceforge.net/liblinebreak/]. It solves different problem but we can adopt some better strategies for text splitting in general.
Moving to 0.7.0

Member

artemp commented Oct 11, 2011

[artem] Good catch. Yes, it needs fixing. Jon Burgess pointed me to [http://vimgadgets.sourceforge.net/liblinebreak/]. It solves different problem but we can adopt some better strategies for text splitting in general.
Moving to 0.7.0

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[Ldp] #409 has been marked as a duplicate of this ticket.

Member

artemp commented Oct 11, 2011

[Ldp] #409 has been marked as a duplicate of this ticket.

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[adamk] As I mentioned in #409, I'd love to help and fix this problem. All I need is some initial guidance to get me going - e.g which module/source-file seems to be the most relevant - and I'll continue on my own from there. Thanks!

Member

artemp commented Oct 11, 2011

[adamk] As I mentioned in #409, I'd love to help and fix this problem. All I need is some initial guidance to get me going - e.g which module/source-file seems to be the most relevant - and I'll continue on my own from there. Thanks!

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[springmeyer] Hey adamk,

Take a look at: source:trunk/include/mapnik/font_engine_freetype.hpp where much of the text code is. Also, likely a good idea to take a look at http://www.pango.org/ to get a sense of how they solve this problem.

Member

artemp commented Oct 11, 2011

[springmeyer] Hey adamk,

Take a look at: source:trunk/include/mapnik/font_engine_freetype.hpp where much of the text code is. Also, likely a good idea to take a look at http://www.pango.org/ to get a sense of how they solve this problem.

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[Esperanza36] No news about this bug ?

Member

artemp commented Oct 11, 2011

[Esperanza36] No news about this bug ?

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[dimka] I propose a quick and dirty patch.

The patch is against http://svn.mapnik.org/tags/release-0.7.1

Member

artemp commented Oct 11, 2011

[dimka] I propose a quick and dirty patch.

The patch is against http://svn.mapnik.org/tags/release-0.7.1

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[springmeyer] dimka, great that you are taking a look at this! I think the most critical thing is to have good, ultra simple, testcases. So that we can easily ensure there are no regressions, and troubleshoot the various inter-related RTL/Unicode issues at http://trac.mapnik.org/wiki/InternationalText. By simple I mean just a ~30 line XML file that reads from a file (like a shapefile or geojson) with just one feature with the text in question along with an image displaying how the text should look.

Can you draft up something like that to go along with your patch, and then attach the before and after images?

Member

artemp commented Oct 11, 2011

[springmeyer] dimka, great that you are taking a look at this! I think the most critical thing is to have good, ultra simple, testcases. So that we can easily ensure there are no regressions, and troubleshoot the various inter-related RTL/Unicode issues at http://trac.mapnik.org/wiki/InternationalText. By simple I mean just a ~30 line XML file that reads from a file (like a shapefile or geojson) with just one feature with the text in question along with an image displaying how the text should look.

Can you draft up something like that to go along with your patch, and then attach the before and after images?

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[dimka] Please see attached archive.
It contains a shapefile with one point feature, a single name attribute with a very long name in Hebrew.

  • image-before.png was produced by current mapnik 0.7.1 and is incorrect (bottom to top).
  • image-after.png was produced by the patched version, and it is the correct rendering.

Is that what you had in mind?

Member

artemp commented Oct 11, 2011

[dimka] Please see attached archive.
It contains a shapefile with one point feature, a single name attribute with a very long name in Hebrew.

  • image-before.png was produced by current mapnik 0.7.1 and is incorrect (bottom to top).
  • image-after.png was produced by the patched version, and it is the correct rendering.

Is that what you had in mind?

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[dimka] Any chance the patch gets promoted?

Member

artemp commented Oct 11, 2011

[dimka] Any chance the patch gets promoted?

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[springmeyer] Hey. Test case looks right. I will try to take a look ASAP. I've assigned to 2.0 milestone to remind us to take a closer look in the next two weeks, but most likely this will take more time because of the need to test it's relationship to other patches in conflict.

Member

artemp commented Oct 11, 2011

[springmeyer] Hey. Test case looks right. I will try to take a look ASAP. I've assigned to 2.0 milestone to remind us to take a closer look in the next two weeks, but most likely this will take more time because of the need to test it's relationship to other patches in conflict.

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[springmeyer] #364 breaks this patch according to reporter there. Needs review if the two can be integrated.

Member

artemp commented Oct 11, 2011

[springmeyer] #364 breaks this patch according to reporter there. Needs review if the two can be integrated.

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[springmeyer] #364 got applied, so I'm assuming this is now broken, so it needs either updating or just someone to review it. Dmika?

for now pushing off till next release as mapnik2 is getting cut anytime now :)

Member

artemp commented Oct 11, 2011

[springmeyer] #364 got applied, so I'm assuming this is now broken, so it needs either updating or just someone to review it. Dmika?

for now pushing off till next release as mapnik2 is getting cut anytime now :)

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[dimka] Replying to [comment:16 springmeyer]:

The new patch works against the current trunk, with the patch from [ticket:364] applied. The test case works. I've also checked that the OSM stylesheet renders correctly.

Please consider adding to the upcoming release.

Member

artemp commented Oct 11, 2011

[dimka] Replying to [comment:16 springmeyer]:

The new patch works against the current trunk, with the patch from [ticket:364] applied. The test case works. I've also checked that the OSM stylesheet renders correctly.

Please consider adding to the upcoming release.

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[artem] Replying to [comment:17 dimka]:

Replying to [comment:16 springmeyer]:

The new patch works against the current trunk, with the patch from [ticket:364] applied. The test case works. I've also checked that the OSM stylesheet renders correctly.

Please consider adding to the upcoming release.

Applied in r3365. Thanks!

Member

artemp commented Oct 11, 2011

[artem] Replying to [comment:17 dimka]:

Replying to [comment:16 springmeyer]:

The new patch works against the current trunk, with the patch from [ticket:364] applied. The test case works. I've also checked that the OSM stylesheet renders correctly.

Please consider adding to the upcoming release.

Applied in r3365. Thanks!

@artemp

This comment has been minimized.

Show comment Hide comment
@artemp

artemp Oct 11, 2011

Member

[artem] Replying to [comment:20 artem]:
NOTE: ubidi_getBaseDirection available from ICU 4.6

Member

artemp commented Oct 11, 2011

[artem] Replying to [comment:20 artem]:
NOTE: ubidi_getBaseDirection available from ICU 4.6

@dimkab

This comment has been minimized.

Show comment Hide comment
@dimkab

dimkab Mar 13, 2012

It seems that the effect of the patch was neutralized in this commit:
9d2a608#src/placement_finder.cpp

dimkab commented Mar 13, 2012

It seems that the effect of the patch was neutralized in this commit:
9d2a608#src/placement_finder.cpp

@springmeyer springmeyer reopened this Mar 13, 2012

@springmeyer

This comment has been minimized.

Show comment Hide comment
@springmeyer

springmeyer Mar 13, 2012

Member

re-opening. sorry about that. @herm - can you restore?

Member

springmeyer commented Mar 13, 2012

re-opening. sorry about that. @herm - can you restore?

@springmeyer

This comment has been minimized.

Show comment Hide comment
@springmeyer

springmeyer Mar 13, 2012

Member

@herm - if you need, see @dimkab 's original testcase attached to http://173.255.217.246:8000/mapnik_trac/ticket/189

Member

springmeyer commented Mar 13, 2012

@herm - if you need, see @dimkab 's original testcase attached to http://173.255.217.246:8000/mapnik_trac/ticket/189

@herm herm closed this in 1b85f42 Mar 16, 2012

@herm

This comment has been minimized.

Show comment Hide comment
@herm

herm Jun 25, 2012

Member

The way RTL is currently handled is very ugly. I'm working on a better solution which should avoid this problem even in mixed RTL LTR cases.

Member

herm commented Jun 25, 2012

The way RTL is currently handled is very ugly. I'm working on a better solution which should avoid this problem even in mixed RTL LTR cases.

@sneetsher

This comment has been minimized.

Show comment Hide comment
@sneetsher

sneetsher Sep 13, 2012

Any updates, as HarfBuzz released v1.0

Any updates, as HarfBuzz released v1.0

@herm

This comment has been minimized.

Show comment Hide comment
@herm

herm Sep 13, 2012

Member

Sorry, I forgot to update this bug report as it was already closed. RTL text should work in all circumstance now (in mapnik's harfbuzz branch).

Member

herm commented Sep 13, 2012

Sorry, I forgot to update this bug report as it was already closed. RTL text should work in all circumstance now (in mapnik's harfbuzz branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment