Logs don't support RTL text correctly.

[shlomil]

(Patch was ported from patch 1668681 in the old BTS)

Fixes and improvements included in this patch:

* Added support for <SPAN style="direction:rtl;"> and <SPAN dir="RTL"> HTML tag for the parser of IMHtml Widget.

* Added text direction detection to outgoing messages and send it with the correct SPAN tag.

* Added support for the "RL=1" flag in Messenger protocol implementation, both in sending and receiving messages.

* Fixed some GTKIMHtml display issues: By injecting RLM and RLE Unicode characters to the conversation display according to the expected alignment and direction.

* Display timestamps and nicknames in the expected order in RTL chats.

* Fixed a problem in gtk_imhtml_get_css_opt() - Function was able to recognized only the first attribute. Now it can read all CSS attributes.

* Fixed a problem with ICQ plug-in: The function purple_unescape_html() was used (oscar.c:4296) to strip HTML tags before sending ch2 messages. Doing that did not work so I changed it to use purple_markup_strip_html() .

Discussion and screenshots are available here.

[shlomil]

Right to left text support for gtkimhtml and MSN protocol

[rlaager]

Duplicated code is bad. Please find an appropriate way to address that. IM me if you need some help.

[shlomil]

2nd version of the Bidi-text support patch for Pidgin

[shlomil]

Changes in the second version of the patch:

• Eliminated code duplication - gtk_imhtml_is_amp_escape(), gtk_imhtml_get_css_opt() functions were removed. libpurple introduces 2 new functions:
  • purple_markup_detect_entity() - was a private function in libpurple now renamed and made public. It does exactly the same thing as gtk_imhtml_is_amp_escape(),
  • purple_markup_get_css_property() - replaces gtk_imhtml_get_css_opt(). Function was buggy and therefore rewritten from scratch.
• changed variable naming style (isrtlmessage => is_rtl_message , etc)
• removed usage of the "0 == ..." idiom.
• Hardcoded line gtkconv.c : isrtlmessage = (purple_strcasestr(message, "<SPAN style=\"direction:rtl;") != NULL); - was changed to parse CSS correctly.
• fixed some code styling issues: removed the braces for single lines, spaces in function arguments.

[rlaager]

This breaks the following protocols:

Yahoo: HTML is shown when receiving. IRC: HTML is shown when receiving.

Additionally, AIM -> ICQ shows LTR alignment. AIM -> AIM and ICQ -> ICQ both work fine.

[rlaager]

The HTML breakage needs to be fixed right away, or I'll have to revert the patch.

[shlomil]

Ok, it's probably only a matter of adding purple_markup_strip_html() at the right place. I'll do it today.

You can revert the patch if you have to.

[seanegan]

I'm making this a defect blocker on 2.0.0 until it's resolved.

[nosnilmot]

I fixed the IRC issue (I think it was actually a bug in sending - not receiving - that I introduced a while ago, oops).

[nosnilmot]

I fixed the Yahoo issue too. Is the AIM->ICQ issue purely cosmetic? does this need to remain a blocker on 2.0.0?

[rlaager]

The AIM->ICQ "issue" is a case where the original patch did not fix the originally problematic behavior. So no, that's not a blocker.

[rlaager]

shlomil: Did you test logging of RTL text? Does it display properly?

[shlomil]

the logs are always displayed in LTR even for RTL messages. I'll look in to logging and see if it can be fixed.

[rlaager]

Re-opening for the logging issue.

[rlaager]

shlomil: Have you had a chance to look at this? I'd really like to get this fixed.

[shlomil]

rlaager: I reviewed the code and I know how to fix it. I was a little busy lately (getting married next month:)) but I promise to fix it as soon as I can.

[seanegan]

shlomil: any updates? If you don't have time to code it, just summarize what needs to be done, and we'll get it finished ;)

[shlomil]

seanegan: I'll start working on this bug in my spare time. No promises yet. (just got back from my honeymoon in Spain today at 5am).

[db42]

can't "purple_log_write" in libpurple/log.c be hooked to add an RTL token or so in correct lines when writing to the log ?

[shlomil]

db42: It actually should_have_been as easy as moving html_is_rtl() and str_embed_direction_chars() from pidgin/gtkconv.c to libpurple/util.h and then using them in log.c, but str_embed_direction_chars() uses Pango and I don't want to add Pango dependency to libpurple... Looking again at the code, I think I might not need that Pango dependency after all, but this change requires additional testing. I'm working on it.

[shlomil]

Fix text direction in log files using unicode chars

[shlomil]

Patch might require some polish but works well: screenshot

[db42]

shlomil, very nice, can you give your input on this patch ? http://developer.pidgin.im/ticket/1977

[rlaager]

shlomil, if you don't have the time to tackle this, could you address the following questions and I can probably get this finished up and committed. I'm sorry it has taken us this long.

There's still a TODO marked in the code that talks about Pango, but it seems like the approach of the latest patch (pidgin-log-bidi-support1.diff) is to do the detection without Pango. If that's the case, can that TODO be removed, or do we still need to do direction detection on "from" there? Either way, the code that was moved from pidgin to libpurple still has #ifdef PANGO14, which should probably be removed.

If the *only way* (short of copying in a ton of Pango code or something) to do this is by adding Pango as a dependency of libpurple, then let's do that but make it optional (so if you don't have it, RTL text will be broken or whatever).

[shlomil]

Ok, So I thought about it for a while and I don't think it's logical to include a Pango dependency just so we can view logs. I realized that the proper way to make it work might be to have the logs include the Unicode direction chars or, if we want an encoding independent solution, use the internal imhtml markup in the logs somehow. This is, IMO, the good,non-hack way to do it, but it might require bigger changes to the code.

What do you think?

[rlaager]

I really don't understand your examples. Could you clarify? If you're thinking about having something in the logs indicate text direction on a per-message basis, where is it going to come from? Is the UI going to have to do text direction detection before the text hits libpurple/log.c?