Adium

Opened 12 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 Ariel Chinn 12 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 12 years ago by Ariel Chinn

Attachment: main.css added

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

comment:1 Changed 12 years ago by Ariel Chinn

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 12 years ago by Ariel Chinn

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 12 years ago by Jordan

Milestone: Patches welcome

comment:4 Changed 12 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

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

comment:5 Changed 12 years ago by Evan Schoenberg

Milestone: Patches welcomeAdium X 1.3
Patch Status: Initially IncludedAccepted

comment:6 Changed 12 years ago by Robert

Summary: The (standard shipped) Stockholm message view brakes with RTL textStockholm breaks with RTL text

comment:7 in reply to:  4 Changed 12 years ago by Eric Richie

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 12 years ago by Evan Schoenberg

Resolution: fixed
Status: closedreopened

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

comment:9 Changed 12 years ago by Evan Schoenberg

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

comment:10 Changed 12 years ago by Evan Schoenberg

Milestone: Adium X 1.3Patches welcome

comment:11 Changed 12 years ago by Robert

Patch Status: AcceptedNeeds Changes by Author

comment:12 Changed 11 years ago by Robert

Milestone: Patches welcomeAdium bugs
priority: normallowest
Severity: normaltrivial

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 Evan Schoenberg

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

Attachment: messageDirection.png added

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 Changed 9 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

(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

Attachment: nodice.png added

comment:20 Changed 9 years ago by Robert

Resolution: fixed
Status: closednew

comment:21 Changed 9 years ago by Evan Schoenberg

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 Evan Schoenberg

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: fixed
Status: newclosed

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

Milestone: Adium bugsAdium 1.4.2
Patch Status: Needs Changes by AuthorRejected

comment:26 Changed 9 years ago by mathuaerknedam

Resolution: fixed
Status: closednew

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

comment:27 Changed 9 years ago by mathuaerknedam

Resolution: fixed
Status: newclosed

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