GNOME Bugzilla – Bug 507564
Mangled contact view for RTL languages.
Last modified: 2013-09-13 00:58:07 UTC
The contact view for RTL languages such as Arabic is mangled. First, I'll attach a screenshot that shows this. Then I'll attach a fix, and a screenshot that shows the final view.
Created attachment 102242 [details] Bad view. RTL languages should swap the directions and swap everything.
Created attachment 102243 [details] [review] Proposed fix. Applied in dir addressbook/gui/widgets This does not touch the original formatting, it just fixes stuff when the language is RTL. This final view is ideal.
Created attachment 102244 [details] Fixed view Screenshot of fixed view. I'm doing this in detail so developers who are not familiar with RTL know what I am addressing.
a) add ChangeLog entry, please b) in this line: gtk_html_stream_printf (html_stream, " </td></tr>", icon); is not the icon parameter needed, please fix it. c) a minor coding style issue: place there a space between function name and an open bracket. Do not have things like this: if(gtk_widget_get_default_direction()... but if (gtk_widget_get_default_direction ()... It's on couple of places. Also, we usually (in new code) have: if (expr) { } else { } d) there is a "width=\"100\"" near HEADER_COLOR in the original code and you do not have it at the RTL code. Does it matter? (It's in couple of places there.) I also noticed you kept "%s:" in RTL code, should not it be ":%s" instead? (note: I do not know RTL at all, I'm just guessing) e) at chunk "@@ -369,17 +389,34 @@", I would prefer not to duplicate the lines with <BR> special for RTL and LTR, do pre and post ifs, please. f) question, is it possible to not generate table in table? (last few chunks in a patch). If there is no other element good for this, then I'm fine with it, but placing table in table was a bad thing for GtkHtml (of course not with only one and one, and it's not the problem anymore, but we can try to avoid in our code, at least). I didn't test the patch yet, I'll do with updated one.
Milan: Before changing the patch: c) if (expr) { } else { } Even for single statements when "{}" are not needed at all? d) ":" is direction neutral, so it will be placed as appropriate based on text (see my last screenshot). There is only "MSN:" that is displayed incorrectly because the latin chars make it LTR, but that's a translator bug, remedied by inserting direction characters or actually translating MSN (I'll do this, I'm Arabic coordinator). f) I found that only <td> obeys align. So, to send the contact to the right I had to encapsulate the whole thing in a table, set it to 100% and align the one child <td> to the right. Otherwise, the table will be placed on the left of the screen. <body> did not obey align. If there is an element I'm not aware of that obeys it then I'd love to know :-)
ad c), of course not, if there is only one expression in the "block", then it's ok to not write brackets. (My intent was in the space, and showing the other style too - two in one.) Just be sure you do a space between "if" and "(expr)". ad d) if it's done automatically, then even better :) ad f) I thought about span, but not sure if it works as expected. If not, feel free to keep it as is. g) chunk "@@ -621,14 +682,16 @@", do not define variables in the middle of the block. :)
Created attachment 102497 [details] [review] Fix contact view. Fixed patch, addresses all queries above. I kept the table inside table thing.
h) It works for standard contacts, but if you create a contact list, File->New->Contact List, then it doesn't work. I think you filled for this separate bug. Am I right? i) Coding style: in all yours "if (...)" you placed the opening bracket of the block on new line, but it's not the right way :) Please fix it. (it's more than once in the patch) j) you didn't add there the "width=\"100\"" to header column in RTL, are you sure with this? (it was the part of d) in my initial review) k) you have there a typo "</tr></tr>", should be "</td></tr>", I guess. I hope you do not take it bad... that I'm too pedantic... I just want have it nice :)
Created attachment 102573 [details] [review] Revised patch. h) Contact list is a separate issue, I'll file a bug and fix. i) Fixed. j) That escaped me at first, it was added in the patch. k) True, fixed. I like the rigor, keep it up :-)
ad h) I thought about bug #391408, where you attached a patch which waits for improvement. It touches same part, so please do it there, not new bug. ad j) you didn't add it into the first chunk, somehow :) and in chunk "@@ -390,11 +412,22 @@" too. In chunk "@@ -529,10 +563,16 @@" and the next one you added width=100% there, it wasn't the intend, was it? Because it belongs to the next column, and without the '%' in LTR code. l) a really little thing, at line 25 of last patch is an empty line which is not necessary there, so remove it. m) the HEADER_COLOR column should have "%s:", not the "mapquest" column in RTL in second chunk and in chunk "@@ -379,7 +398,10 @@". n) in chunk "@@ -379,7 +398,10 @@" and the right above, the LTR order is "image_col, header_col, data_col", the RTL part has "data_col, header_col, image_col" in HTML format, which is right, but the '_("map")' is used to header_col's header_part, which is not right (it should be used to mapquest part). Same in above maprequest in preceding chunk. Do you know what I mean with this? o) Also in chunk "@@ -379,7 +398,10 @@" do not add there a white space at the beginning of the HTML code (...gstr, " </td>... should be ...gstr, "</td>...), or does it have any special function there (btw, it's a tab, not a space). p) just a question, you've there a comment about a "filler", I guess it's because of " " in icon_col, but I think the is not needed there, as it isn't elsewhere and it works fine. Or does it? Otherwise looks good, really. One hint I used to check the HTML code: copy the resulting HTML code on one line, exchange the left and right columns (there are usually 3 columns, so it's fine), and then just look if it's same or not in RTL part as in LTR part (except of possible align=\"right\", of course.
(from IRC) forgot the o) please, my fault, I'm so sorry for that, *I* added it there when I was checking the HTML code.
Created attachment 102588 [details] [review] Fixed patch Thanks Milan for the quick review. h) Well, apart from both being in addressbook, they have little connection. This uses GtkHTML, and the minicards use gnomecanvas. I don't really know much about gnomecanvas so I'm postponing that to some other time (also because of some build errors). I really prefer if these two issues stay separate. j) Fixed. l) Fixed. m) Fixed. n) That's right. I thought they were in different td's hence the reversal. Fixed. o) <retracted> p) I removed these.
q) another empty line at line 94 of the patch, should be moved just before line 101 of the patch (g_free (value);) r) the first chunk, <FONT> cannot have a width=\"100\" attribute, move it a few characters earlier, just before "nowrap" word, as it is in LTR. Same applies to chunk "@@ -529,10 +561,16 @@" s) second chunk, in RTL, the column for header info is not aligned to the right. (twice) t) in chunk "@@ -379,7 +398,10 @@" you moved the "align=\"right\"" too much to the right, you misplaced it to icon column, not the header column. We are close now, we are close :)
Created attachment 102619 [details] [review] Fixed patch ^ 5 Embarassing, sometimes I wonder where my head is. Fixed above points.
This is the patch we were looking for. Please commit to trunk.
Committed to trunk. Committed revision 34832. I hope you are fine with this. I'm going to touch same source file so I decided to commit it before my changes. Thanks for understanding.