Why is RTL class user dependent instead of locale?

[ibnesayeed (Sawood Alam)]

Recently, I added support for Urdu language, which, like Arabic, Hebrew, and Persian, is an RTL (right-to-left) language. To make the software aware of it I added ur locale in rtl_locales method of the RTL model class.

https://github.com/discourse/discourse/blob/49fc54aac603fee08a6fb1e8b9610a0537ca9ec2/app/models/rtl.rb#L21-L23

However, my site’s direction did not change in right-to-left. So, I started investigating what’s wrong? I found that there is a helper method rtl? in the ApplicationHelper class.

https://github.com/discourse/discourse/blob/49fc54aac603fee08a6fb1e8b9610a0537ca9ec2/app/helpers/application_helper.rb#L137-L139

There seems some duplication of logic. So, I looked into the RTL class further and found it confusing because it’s initialization is dependent on a User instance.

https://github.com/discourse/discourse/blob/49fc54aac603fee08a6fb1e8b9610a0537ca9ec2/app/models/rtl.rb#L1-L7

This implementation would make perfect sense if RTL is a property independent of the locale in a way that my Arabic may be RTL, but your Arabic may not. However, this is not the case. RTL (or LTR) is the property of the locale, independent of the user. If the I18n.locale is assigned a property called direction which may hold values rtl or ltr, with ltr being the default (let’s forget about the ttb direction for now because that is almost always represented as ltr on the web). Alternatively, a method rtl? is added to the I18n.locale that returns false by default, unless the locale is in the array of RTL locales.

This way, entire RTL class can go away and wherever a locale object is accessible, its direction is also known. We don’t need to know separately if a user’s locale has RTL enabled, we just find the effective locale and apply RTL if applicable.

I know that Discourse does not encourage code refactoring for the sake of refactoring, but being a newbie to the code base I would like to hear the rationale behind why it is the way it is?

[simon]

I think the RTL class is used in UserNotifications for adding the rtl class to emails.

[ibnesayeed (Sawood Alam)]

@simon

I think the RTL class is used in UserNotifications for adding the rtl class to emails.

Does this change anything? After all user’s locale or site’s default locale is determined in order to know if it belongs to RTL or not. If the direction was property of the locale, then we wont need this RTL class at all, be it in the email, notification, site’s layout, CSS classes and whatnot.

[simon]

@ibnesayeed

Does this change anything?

It just means that the class can’t be eliminated without changing a few other things as well. But yes, what you’re saying seems correct to me.

[ibnesayeed (Sawood Alam)]

@simon

It just means that the class can’t be eliminated without changing a few other things as well.

Such as, lines 389 and 401

discourse/discourse/blob/ad5d16af8ed0257fd14ed54d060946dbda6f8db1/app/mailers/user_notifications.rb#L388-L390

become (if direction property is defined on locale)

classes: user.effective_locale.direction

or (if rtl? method is defined on locale)

classes: user.effective_locale.rtl? ? 'rtl' : ''

[simon]

Yes. I don’t know if this is a change they will want to make, but it’s good you are interested in working on this. I’ve done some work on it in the past. I got involved with it to help set up a forum, but I don’t speak a RTL language. It’s a great way to learn the Discourse code. There are still a few issues in the javascript as well: RTL interface has wrong position for some of popup boxes.

[ibnesayeed (Sawood Alam)]

Also, I noted that in the existing TRL class there is some logic duplicated.

https://github.com/discourse/discourse/blob/49fc54aac603fee08a6fb1e8b9610a0537ca9ec2/app/models/rtl.rb#L9-L19

This chunk can be replaced by the following code.

def enabled?
  user.effective_locale.in?(rtl_locales)
end

Because current_user_rtl? and site_locale_rtl? are local methods and not used elsewhere. While the common portion of the two methods (finding the effective locale) is already defined in the User class.

https://github.com/discourse/discourse/blob/49fc54aac603fee08a6fb1e8b9610a0537ca9ec2/app/models/user.rb#L186-L192

[ibnesayeed (Sawood Alam)]

@simon

I don’t know if this is a change they will want to make, but it’s good you are interested in working on this.

I feel that they might actually be willing to get rid of a class that does not deserve to be a class, because the class itself is not that deep rooted. If we keep the surface methods such as rtl? helper method behave the very same way, just change the base implementation without affecting the API then it should be OK. Obviously, we might need some more test cases related to locale direction beyond what is already there.

However, being new to the code base I won’t jump to a conclusion too quickly without hearing from some other people who have better understanding of the code inside out.

[Mittineague]

I won’t go so far to say I know the code inside out, but I’ll chip in my

I looked to see if any tests might break if current_user_rtl or site_locale_rtl were removed. I found none and in fact the only place they exist in the Core code is in the models/rtl.rb file.
So removing them might break some plugins if they are using them, but it shouldn’t break Discourse.

I don’t think “i18n” would need to be used in the proposed enabled but I’ve seen it used in other files.

https://github.com/discourse/discourse/blob/master/app/services/post_alerter.rb#L336-L344

https://github.com/discourse/discourse/blob/master/app/services/badge_granter.rb#L46-L57

https://github.com/discourse/discourse/blob/master/app/controllers/application_controller.rb#L214-L225

[ibnesayeed (Sawood Alam)]

It looks like locale is simply a string, not an object. Hence we can’t attach properties to it. However, there are other ways to specify the direction. One is to add a direction or rtl? attribute in the config/locales/names.yml file. Another possibility is to define rtl? method in a place from where it can be called from any part of the application.

https://github.com/discourse/discourse/blob/master/config/locales/names.yml

[Mittineague]

For some reason I like the idea of having it in the yml file.

I have no basis for the feeling other than it just seems like the logical best place for it.

I tried hacking the rtl.rb file to

  def enabled?
#    site_locale_rtl? || current_user_rtl?
    user.effective_locale.in?(rtl_locales)
  end

#  def current_user_rtl?
#    SiteSetting.allow_user_locale && user.try(:locale).in?(rtl_locales)
#  end

#  def site_locale_rtl?
#    !SiteSetting.allow_user_locale && SiteSetting.default_locale.in?(rtl_locales)
#  end

I don’t know Farsi so I could very well not be seeing a glaring issue.
But the hack did not crash Discourse and I saw no error messages anywhere.
I did notice that the title was not translated for some pages, but that was true for both hacked and not-hacked.
And I did not try the set locale from accept language header setting nor test emails or many pages at all.

I did test as a site setting desktop and mobile view, and allow user preference desktop and mobile view.

site default setting desktop

site default setting mobile

user preference desktop

user preference mobile