Rockbox

Tasklist

FS#10882 - RTL aware WPS

Attached to Project: Rockbox
Opened by Tomer Shalev (tomers) - Sunday, 27 December 2009, 21:02 GMT
Last edited by Jonathan Gordon (jdgordon) - Thursday, 07 January 2010, 07:57 GMT
Task Type Patches
Category Themes
Status Closed
Assigned To Tomer Shalev (tomers)
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch extends the WPS specification to include new directives that are sensitive to the direction of the language - either Left-to-Right or Right-to-Left.

'aL' - Align Left RTL - Will align left when the language is set to any LTR language, but will align to right on RTL language

'aR' - Opposite of 'aL', It aligns to right normally, or aligns to left on RTL language.

For viewport and album-art directives, there is a new optional 's' flag as a prefix to the X coordinate. It will horizontally mirror the location of the item. E.g.:

Configuring a viewport of size (100, 50) in location (10, 5) is done in the following way:

%V|s10|5|100|50|...

This will result in the item showing as expected in any LTR language, but when RTL language is set, it will result (assuming LCD screen with dimensions of 320x240) in the viewport being set at location (210, 5), with the same width and height.

The patch includes calling to settings_apply_skins() on language load that causes the WPS file to be re-parsed, in order to recalculate the WPS's dimensions.

This patch includes a modified WPS file for only Cowon D2. All other relevant Cabbie v2 files should also be modified.

Please see attached screenshot to see how it looks like.
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Thursday, 07 January 2010, 07:57 GMT
Reason for closing:  Accepted
Comment by Jonathan Gordon (jdgordon) - Monday, 28 December 2009, 02:09 GMT
I'm not a huge fan of the added s option in the viewport definition.... is there a better way to do it?
Comment by Tomer Shalev (tomers) - Monday, 28 December 2009, 05:22 GMT
> I'm not a huge fan of the added s option in the viewport definition.... is there a better way to do it?

Maybe add a language sensitive variants to the following commands (defining a viewport / loading albumart):

%V|[s]x|y|[width]|[height]|[font]|
%Cl|[s]x|y|[l|c|r]maxwidth|[t|c|b]maxheight|

The variants will look like this:

%Vs|x|y|[width]|[height]|[font]|
%Cs|x|y|[l|c|r]maxwidth|[t|c|b]maxheight|

's' is for 'swap'. Might be any other letter, though.
Comment by Thomas Martitz (kugel.) - Tuesday, 29 December 2009, 13:30 GMT
I'm not a fan as well. aL, aR is accaptable but would object adding RTL handling for every possible tag.

I think a rtl conditional should be sufficient, so you could conditionally display viewports. That's more flexible anyway.
Comment by Tomer Shalev (tomers) - Tuesday, 29 December 2009, 18:19 GMT
Attached is a much simpler solution for this problem.
Comment by Tomer Shalev (tomers) - Tuesday, 29 December 2009, 19:01 GMT
- No functional change
- Modified CabbieV2 WPS screens of ALL relevant players
Comment by Tomer Shalev (tomers) - Tuesday, 29 December 2009, 21:56 GMT
There could be an even simpler solution for this! :-)
Comment by Tomer Shalev (tomers) - Tuesday, 29 December 2009, 21:58 GMT
Please ignore last patch which had some irrelevant stuff in it. This one is the appropriate patch.
Comment by Thomas Martitz (kugel.) - Tuesday, 29 December 2009, 23:32 GMT
Neither of those address our objections, right?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 December 2009, 00:44 GMT
wouldnt it make more sense to make viewport always rtl-aware? also, AA should really be loaded in its own viewport at 0,0 viewport relative) so it doesnt need to know where in the viewport it is....
Comment by Tomer Shalev (tomers) - Wednesday, 30 December 2009, 17:45 GMT
> Wouldn't it make more sense to make viewport always rtl-aware?

It wouldn't make more sense to make the viewport always rtl-aware, because some themes will not look well if being mirrored. Also, the text that is usually beneath the progress bar (time elapsed / time remaining) will be swapped, which is undesirable. Swapping it might make sense if the progress-bar will progress from right to left, but then the key mapping should also be swapped - Pressing left instead of right in order to skip to the next track, etc. I don't think that rtl-langs users would want such thing.
There are viewports which we know we can mirror, such as the viewport which holds the track info when an album-art is available. That viewport together with the album-art looks much better when swapped in rtl language.

> AA should really be loaded in its own viewport at 0,0 viewport relative) so it doesnt need to know where in the viewport it is....

The AA is oriented to the left or right according to the format specified in the wiki page (format: %Cl|x|y|[[l|c|r]mwidth]|[[t|c|b]mheight]| ). That alignment should be mirrored in case of direction swapping. The AA location should also be swapped. If it is inside a dedicated viewport, as you suggested, then the viewport would also be swapped, but the AA swapping would not be noticed, since it is in a viewport of the same width. I don't like the idea of not swapping AA, because the WPS designer might not put it in a dedicated viewport.
The AA loading token should be either language direction aware or not, depending on how the WPS designer wants it to be. The way I found to implement this requirement is to introduce a new AA loading token, %CL in addition to current %Cl. If you have any better suggestion, I'm all ears. I can't see how making AA loaded in its own viewport changes anything. Can you please expand on the subject?

Thanks for reviewing!
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 December 2009, 20:20 GMT
ok, good points. In that case how about adding a token which says "the next token needs to respect RTL"? so a viewport decleration would look like %RL%V|...|. doing this means we dont need to change the token definition for every token we want to fix, we only need to have the code check some magic vairable...

it could even use the same token as the conditional (I tihnk)
Comment by Jonathan Gordon (jdgordon) - Friday, 01 January 2010, 20:01 GMT
something like this (untested patch)... add %aa before any %V line and it shuld magically align with the language.
e.g

%aa%V|10|10|30|..........
Comment by Jonathan Gordon (jdgordon) - Monday, 04 January 2010, 23:41 GMT
this one actually works.... I dont really like %aa for this.. but %al is already used... %aL maybe?
Comment by Thomas Martitz (kugel.) - Tuesday, 05 January 2010, 00:08 GMT
We had some discussion about this here: http://www.rockbox.org/irc/log-20100105#00:36:39

In short:
I largely disagree with modifying the location or dimensions of anything that is explicitely set (i.e. viewport and AA numbers), so I disagree with the viewport.c change in the latest patch. And I disagree with changing the location of the albumart.
These things are design decisions and a matter of taste, and nowhere depending on the locale. That you would like to see the AA on the right side is your taste, but it doesn't have anything todo with RTL.

Hence I'm still in favor of a rtl conditional tag and %aL and %aR (to align text depending on RTL because that is a locale thing). That combo is *way* more flexible anyway, it allows to react on rtl in an intelligent manner instead of being stupid and morroring everything.
Comment by Tomer Shalev (tomers) - Tuesday, 05 January 2010, 06:33 GMT
> I largely disagree with modifying the location or dimensions of anything that is explicitely set (i.e. viewport and AA numbers), so I disagree with the viewport.c change in the latest patch. And I disagree with changing the location of the albumart.

I disagree with you. We do not change the dimension of anything, only the location. Please consider the design decisions taken by Trolltech when approaching this problem. The introduced the LayoutDirection property. (see http://doc.trolltech.com/4.6/qt.html#LayoutDirection-enum). Setting layout direction to Qt::RightToLeft inverts the layout horizontally. Please see attached screenshots. I like this approach as it saves the developers from doubling their code in order to allow RTL alignment. All it takes to change the layout is a single command. See http://svn.rockbox.org/viewvc.cgi?view=rev&revision=23264

Applying this approach on the WPS means we do not need to add a lot of extra skin directives (duplicate elements for RTL and LTR) and conditionals to choose the appropriate elements, in order to make the skin layout language direction aware, and we can selectively control which elements on which skins are language direction aware.

> These things are design decisions and a matter of taste, and nowhere depending on the locale.

I don't see how this feature conflicts with any design decision made for the skin system.

> That you would like to see the AA on the right side is your taste, but it doesn't have anything todo with RTL.

It has to do with RTL because this looks differently when the language is RTL. It simply doesn't looks natural... It is indeed a matter of taste, but I think that most users who use RTL language will agree. This is why the layout is swapped in many GUI systems that correctly support RTL languages (Windows, Ubuntu, QT apps, etc.).


> Hence I'm still in favor of a rtl conditional tag and %aL and %aR

Those directives are not needed if we decide to keep the %aa (BTW, I prefer it would be named %ax), since text that is aligned left using %al will be automatically be aligned to the right if the viewport flags are set to RTL.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 05 January 2010, 07:39 GMT
This is how far I got before the laptop battery died on the plane...

%ax does what %aa did above
%aR/L do what they did in the origional patch

%V, %Cl and %pb all work with %ax, but I didnt get enobugh time to fix the AB marks to work when the progress bar is being swapped. All the viewported cabbie's have been fixed to handle this.

it has a few extra chunks which doesnt need to be commited, but im 3/4 asleep and want to get this off my laptop.

Tomers, even if %al works as it should, we should still add %aL because (as in cabbie) sometimes we dont want the viewport to be wholy swapped (I think)
Comment by Tomer Shalev (tomers) - Thursday, 07 January 2010, 05:35 GMT
JdGordon,

I like your changes and it worked very good for me. Thanks!

I think that having the progress bar (and all its related marks, such as A-B and cuesheet marks) should be further considered.
Having the progress-bar fill from left to right implies (intuitively to the user) that pressing Left skips to the next song, instead of pressing Right, otherwise the key mappings are counter-intuitive, TMHO.

In current patch (plus the key mapping change implementation which is not included in the patch), while an RTL language is in use the direction of the progressbar is dictated by which WPS skin is used. This means that changing skin might follow with changed key mappings, if the progress bar direction have changed.
This is both confusing to the user, and is most probably unaccepted by the Rockbox guidelines. Therefore I suggest dropping support for language direction sensitivity of the progress bar and its related marks.
Comment by Jonathan Gordon (jdgordon) - Thursday, 07 January 2010, 05:41 GMT
Well I was working on the assumption that swapping the buttons would be an obvious change to be done with this anyway? dropping that support for the time being and commiting the rest is easy to do if you want.
Comment by Tomer Shalev (tomers) - Thursday, 07 January 2010, 06:48 GMT
I think we should definitely commit the rest ASAP, to be ahead of the the release. Can you please do so?

As for the buttons, the code change might be trivial. button_flip_horizontally() does exactly that. We can add another check in case of CONTEXT_WPS, that checks the progress bar direction of the current skin (what a mess).
I don't think that we can take such decision without further discussion with other developers. I remember that developers were reluctant to changing key mappings while the hold button is set, in the Cowon D2 platform, and this issue is somehow similar to that. So lets commit and maintain the progress bar code changes patch on this task, then discuss this with other developers on IRC (or hopefully they response here).
Comment by Jonathan Gordon (jdgordon) - Thursday, 07 January 2010, 07:35 GMT
done!

I'll leave the progressbar up to you if you like?

Loading...