Adium

Opened 11 years ago

Closed 9 years ago

#10403 closed defect (fixed)

Stockholm breaks with RTL text

Reported by: yelly Owned by: mathuaerknedam
Milestone: Adium 1.4.2 Component: Message View
Version: Severity: minor
Keywords: Cc:
Patch Status: Rejected

Description

The Stockholm message view displays the time on hover by messages, but the time is always aligned to the right, braking RTL text. I've attached an updated version of main.css, as the developer has no contact info.

Attachments (4)

main.css (6.1 KB) - added by yelly 11 years ago.
An updated version of main.css from the resources folder that fixes RTL text
messageDirection.png (192.0 KB) - added by mathuaerknedam 9 years ago.
nodice.png (333.7 KB) - added by mathuaerknedam 9 years ago.
Stockholm.AdiumMessageStyle.zip (68.8 KB) - added by mathuaerknedam 9 years ago.
messageDirection-modified Stockholm

Download all attachments as: .zip

Change History (31)

Changed 11 years ago by yelly

An updated version of main.css from the resources folder that fixes RTL text

comment:1 Changed 11 years ago by yelly

I forgot that I needed to edit more than just the main.css file, and I've just found a problem with my implementation. I'll upload a complete version soon.

comment:2 Changed 11 years ago by yelly

Well, I've tried to solve the problems that arose, but one led to the next and I am defeated. If anyone wants to pick up from where I left, contacts me on the forums.

comment:3 Changed 11 years ago by jas8522

  • Milestone set to Patches welcome

comment:4 follow-up: Changed 11 years ago by evands

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

(In [24363]) Fixed RTL text in Stockholm. Patch from yelly, thanks :) Fixes #10403

comment:5 Changed 11 years ago by evands

  • Milestone changed from Patches welcome to Adium X 1.3
  • Patch Status changed from Initially Included to Accepted

comment:6 Changed 11 years ago by Robby

  • Summary changed from The (standard shipped) Stockholm message view brakes with RTL text to Stockholm breaks with RTL text

comment:7 in reply to: ↑ 4 Changed 11 years ago by edr1084

Replying to evands:

(In [24363]) Fixed RTL text in Stockholm. Patch from yelly, thanks :) Fixes #10403

Did you make any changes to the patch before applying? I'm seeing some major breakage now as yelly had said.

comment:8 Changed 11 years ago by evands

  • Resolution fixed deleted
  • Status changed from closed to reopened

Oh. Heh. Sorry, I missed that. It looked like just RTL changes in the diff.

comment:9 Changed 11 years ago by evands

(In [24379]) Reverted [24363]. Bad patch review; sorry. Refs #10403

comment:10 Changed 11 years ago by evands

  • Milestone changed from Adium X 1.3 to Patches welcome

comment:11 Changed 11 years ago by Robby

  • Patch Status changed from Accepted to Needs Changes by Author

comment:12 Changed 11 years ago by Robby

  • Milestone changed from Patches welcome to Adium bugs
  • priority changed from normal to lowest
  • Severity changed from normal to trivial

comment:13 Changed 9 years ago by mathuaerknedam

Could we get message direction (either rtl or ltr) added to %messageClasses%? This would allow (ltr-centric) style authors to better account for rtl messages.

comment:14 Changed 9 years ago by evands

Is the %messageDirection% replacement not sufficient? It can easily be added to messageClasses, as well, if that's needed.

comment:15 Changed 9 years ago by mathuaerknedam

Oops! %messageDirection% should be sufficient; I forgot that it existed.

comment:16 Changed 9 years ago by mathuaerknedam

  • Owner set to mathuaerknedam

Changed 9 years ago by mathuaerknedam

comment:17 Changed 9 years ago by mathuaerknedam

As can be seen from the pic I attached, %messageDirection% doesn't seem to be working as [I] expected. It returns "LTR" even though <div dir="rtl"> is wrapped around the actual message.

comment:18 follow-up: Changed 9 years ago by Evan Schoenberg

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

(In dd1d939811c7) When checking to see if the HTML includes an RTL div, do a case-insensitive search. This check had been broken by changes in AIHTMLDecoder; it's significantly less fragile if case insensitive. Fixes #10403 I believe, though I have not tested it.

comment:19 in reply to: ↑ 18 Changed 9 years ago by mathuaerknedam

Replying to Evan Schoenberg:

(In dd1d939811c7) When checking to see if the HTML includes an RTL div, do a case-insensitive search. This check had been broken by changes in AIHTMLDecoder; it's significantly less fragile if case insensitive. Fixes #10403 I believe, though I have not tested it.

Unfortunately, this does not seem to have fixed the issue. Screenshot attached.

Changed 9 years ago by mathuaerknedam

comment:20 Changed 9 years ago by Robby

  • Resolution fixed deleted
  • Status changed from closed to new

comment:21 Changed 9 years ago by evands

Could you please post how you're testing this so I can look locally?

Changed 9 years ago by mathuaerknedam

messageDirection-modified Stockholm

comment:22 Changed 9 years ago by mathuaerknedam

I've attached a zip of the version of Stockholm that's had %messageDirection%. I added it at each level from the root to the message in content.html, as well as the root level of status.html. When loaded, the message style will be listed in Adium's prefs as "Stockholm ☿".

To test I made this style active, connected to freenode and opened a window to #test. I pasted in the hebrew text that's part of the Trac Docs (אני יכול לאכול זכוכית וזה לא מזיק לי), and then from the chat window contextual menu chose "Inspect element" to see the resulting HTML.

comment:23 Changed 9 years ago by evands

Thanks very much. Bug was easy to find & fix with that approach and the modified Stockholm.

comment:24 Changed 9 years ago by Evan Schoenberg

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

(In e06b74c6f7fd) The RTL check must look at the encoded message, not the template, to determine if the encoded message contains RTL text. Fixes #10403

comment:25 Changed 9 years ago by Robby

  • Milestone changed from Adium bugs to Adium 1.4.2
  • Patch Status changed from Needs Changes by Author to Rejected

comment:26 Changed 9 years ago by mathuaerknedam

  • Resolution fixed deleted
  • Status changed from closed to new

Reopening because Evan's commit only means that now I can fix Stockholm. :)

comment:27 Changed 9 years ago by mathuaerknedam

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

(In d3e9da090928) Stockholm: Add support for displaying message timestamps on the left when the message text is RTL. Fixes #10403.

Note: See TracTickets for help on using tickets.