Opened 10 years ago

Closed 6 years ago

#78 closed patch (patch_rejected)

Logs don't support RTL text correctly.

Reported by: shlomil Owned by: rlaager
Milestone: Patches Needing Improvement Component: pidgin (gtk)
Version: 2.0 Keywords: Hebrew bidi RTL Arabic "bi-directional text"
Cc: Dawudd

Description

(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.

Attachments (3)

pidgin-bidi-support.patch (17.1 KB) - added by shlomil 10 years ago.
Right to left text support for gtkimhtml and MSN protocol
pidgin-bidi-support2.patch (26.4 KB) - added by shlomil 10 years ago.
2nd version of the Bidi-text support patch for Pidgin
pidgin-log-bidi-support1.diff (10.6 KB) - added by shlomil 10 years ago.
Fix text direction in log files using unicode chars

Download all attachments as: .zip

Change History (34)

Changed 10 years ago by shlomil

Right to left text support for gtkimhtml and MSN protocol

comment:1 Changed 10 years ago by rlaager

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

comment:2 Changed 10 years ago by rlaager

  • Milestone Go public with Pidgin 2.0.0 deleted

comment:3 Changed 10 years ago by lschiere

  • Owner set to rlaager

Changed 10 years ago by shlomil

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

comment:4 Changed 10 years ago by 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.

comment:5 Changed 10 years ago by rlaager

  • Resolution set to fixed
  • Status changed from new to closed

comment:6 follow-up: Changed 10 years ago by rlaager

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:7 in reply to: ↑ 6 Changed 10 years ago by rlaager

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

comment:8 Changed 10 years ago by 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.

comment:9 Changed 10 years ago by seanegan

  • Milestone set to Go public with Pidgin 2.0.0
  • Type changed from patch to defect

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

comment:10 Changed 10 years ago by nosnilmot

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

comment:11 Changed 10 years ago by 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?

comment:12 Changed 10 years ago by rlaager

  • Milestone Go public with Pidgin 2.0.0 deleted
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Type changed from defect to patch

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.

comment:13 Changed 10 years ago by rlaager

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

comment:14 Changed 10 years ago by 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.

comment:15 Changed 10 years ago by rlaager

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Summary changed from Fix Bi-Directional text issues in gtkimhtml and MSN protocol to Logs don't support RTL text correctly.
  • Type changed from patch to defect

Re-opening for the logging issue.

comment:16 Changed 10 years ago by lschiere

  • Milestone set to 2.0.1

comment:17 Changed 10 years ago by lschiere

  • Milestone changed from 2.0.1 to 2.0.2

comment:18 Changed 10 years ago by rlaager

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

comment:19 Changed 10 years ago by 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.

comment:20 Changed 10 years ago by 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 ;)

comment:21 Changed 10 years ago by 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).

comment:22 Changed 10 years ago by 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 ?

comment:23 Changed 10 years ago by 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.

Changed 10 years ago by shlomil

Fix text direction in log files using unicode chars

comment:24 Changed 10 years ago by shlomil

Patch might require some polish but works well: screenshot

comment:25 Changed 10 years ago by db42

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

comment:26 Changed 9 years ago by Sim-on

  • Milestone set to 2.4.2

comment:27 Changed 9 years ago by rlaager

  • Type changed from defect to patch

comment:28 Changed 9 years ago by rlaager

  • Milestone set to Patches Needing Improvement

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).

comment:29 Changed 9 years ago by 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?

comment:30 Changed 9 years ago by 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?

comment:31 Changed 6 years ago by rekkanoryo

  • Resolution set to patch_rejected
  • Status changed from new to closed

Resolving this patch as "patch_rejected" because the issues mentioned here over a year ago have not been addressed. Anyone who wants to pick this patch up and improve it is more than welcome to do so.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!