Created attachment 258872 [details] [review]
Fix-Popup-RTL-LTR-direction

I can to verify - the patch fix the bug 711254,
and also more bugs with the notifications in RTL.

*** Bug 711254 has been marked as a duplicate of this bug. ***

Review of attachment 258872 [details] [review]:

Prepend a "messageTray:" to the commit message, otherwise fine.

Created attachment 258874 [details]
Screenshot - after the patch

We need to align the content of the notification.
The label in the notification itself align to right
also if the content of the label is in English, and
also if the content of the label is in Hebrew.

I think it relate to bug 710241.

For reference, this was done in bug 646921 where the conclusion was that the alignment should actually depend on the actual text.
I don't care one way or the other, but I wouldn't want to change back and forth because we can't make up our mind how non-RTL notifications should behave in RTL locales ...

See https://bugzilla.gnome.org/show_bug.cgi?id=646921#c9 in particular.

Review of attachment 258872 [details] [review]:

Besides Florian's comments (which we should probably check with yoseforb and elad about), you should remove this._titleDirection entirely, along with the set_text_direction call below, as the comment is now wrong.

You will need to fix _bannerBoxallocate to use actor.get_text_direction() rather than this._titleDirection as well...

http://fpaste.org/51842/13836745/

Created attachment 260310 [details] [review]
0001-Fix-Popup-RTL-LTR-direction.patch

Created attachment 271237 [details] [review]
messageTray: Fix the direction of popup notification in RTL

Review of attachment 271237 [details] [review]:

Code looks good now. Still, I do want Elad to take a look at this, as he was very involved in defining the current behavior (e.g. "latin chat notifications look wrong when right-aligned"). Also please provide a proper commit message - "Fix foo" is OK if something is obviously broken, but this is changing behavior from something some people consider correct to something other people consider correct, so there should be some justification for the change.

Review of attachment 271237 [details] [review]:

::: js/ui/messageTray.js
@@ +643,3 @@
         this._titleLabel.clutter_text.set_markup('<b>' + title + '</b>');
 
+        this._table.set_text_direction(Clutter.get_default_text_direction());

This line shouldn't be needed.

Created attachment 271326 [details] [review]
messageTray: Set the direction of notification by the direction of the UI

No, this patch is obviously wrong.

The text in the notification should be aligned in the direction in which it's easier to read, therefor LTR languages should be aligned to the left and displayed as LTR.

Other places in the the system, such as gtk input boxes, or Empathy message viewing behave the same. Notifications should stay the way they are now.

You need to make a distinction between system direction and content direction. As an English speaker, you wouldn't want to see English test displayed as if it was LTR, and English text in notifications is still possible in a system with an RTL locale. Same things go the other way around, I might get an email message with a title in Hebrew, I want it to be aligned to the right and displayed as RTL text so I can properly read it even if my system locale is English.

This bug makes no sense to me, as the behavior we have right now is the correct and logical behavior. Look at Yosef's screenshot, does this really look okay to you? the text is displayed in the wrong direction, and reading it is harder than reading it if it wad displayed in the correct direction.

Better explanation, because Yosef claims this screenshot is unrelated. I really don't understand how a bug like this without a proper description is even an acceptable bug. Anyway,

Text should ALWAYS be aligned to the direction of the text. You really DON'T want to have a case where punctuation marks are in the wrong location. You want text to ALWAYS be as readable as it can be. LTR text aligned to the left, RTL text aligned to the right. The title of the notification and the content of the notification should NOT affect each other. They are individuals, and so is their direction. Each of them should be aligned separately. 

Music notifications is special-case. The title of the song should be aligned as per the content, but the media button themselves should conform with the system direction, they shouldn't flip if you have one song with a Hebrew title and another with an English title. They should also be identical to the direction of the buttons in Music and Rhytmbox: inconsistency is a bad thing.

When a notification has an icon or a picture, the position of the icon or the picture should be decided by the system locale and be consistent, otherwise you might get one notification with the picture in the left, and another with the picture in the right and that should not happen.

These should be the proper guidelines for RTL/LTR handling in notifications, and I'm sure Yosef would agree with these.

Whatever you do, do NOT set the direction of the notification by the system locale's direction, because this will mean people with LTR locales getting messages in any RTL language, or people with RTL locales getting messages in any LTR language will have hard time reading the messages they receive.

This was obsoleted by the new notification design - title/summary are no longer on the same line, so simply following the default text direction should be fine, and a consistent text direction clearly works better with the vertical message list in the calendar.