Rockbox Technical Forums

Support and General Use => User Interface and Voice => Topic started by: amr on March 12, 2010, 01:14:42 PM

Title: view Arabic text (cont.) - About the diacritics patch
Post by: amr on March 12, 2010, 01:14:42 PM
Hello,

This is a continuation on the locked post about diacritics http://forums.rockbox.org/index.php?topic=22388.0.
Here I provide my test before and after the fix of the diacritics patch http://www.rockbox.org/tracker/task/10720 and my attempt to contribute in fixing the issue.

First here are screen dumps of the case before adding diacritics support
(http://www.rockbox.org/tracker/task/11095?getfile=21554) (http://www.rockbox.org/tracker/task/11095?getfile=21555)
as you see, the characters are unjoined and each diacritic take the width of a full non-diacritic character

After diacritics support, the case changed where diacritics disappear in both the text viewer and file list view, and appears only (well positioned without taking new character width) for file name when highlighted on selection in the file list view..; but for all, characters remain unjoined
(http://www.rockbox.org/tracker/task/11095?getfile=21557) (http://www.rockbox.org/tracker/task/11095?getfile=21558) (http://www.rockbox.org/tracker/task/11095?getfile=21556)

I tried to fix the problem by a small change in bidi.c http://www.rockbox.org/tracker/task/11095 and here is the result
(http://www.rockbox.org/tracker/task/11095?getfile=21559) (http://www.rockbox.org/tracker/task/11095?getfile=21560) (http://www.rockbox.org/tracker/task/11095?getfile=21561)
This fixed the joining problem, now all characters are well connected, but still diacritics appear only on marking file selection in the list view.., I tried to investigate the cause of that and I guess it has something to do with the change of draw mode for diacritics in lcd-bitmap-common.c, but I still don't have a full understanding of the issue and why diacritics appear only on highlighting.
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: Chronon on March 12, 2010, 11:09:46 PM
Nice work!  I can't offer any direct advice, but you might find some guidance on the IRC channel.
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: tomers on March 14, 2010, 01:52:48 AM
...but still diacritics appear only on marking file selection in the list view.., I tried to investigate the cause of that and I guess it has something to do with the change of draw mode for diacritics in lcd-bitmap-common.c, but I still don't have a full understanding of the issue and why diacritics appear only on highlighting.

Please review the comment in line 168 in firmware/drivers/lcd-bitmap-common.c. It should explain your observation:
http://svn.rockbox.org/viewvc.cgi/trunk/firmware/drivers/lcd-bitmap-common.c?view=annotate

Thanks,
Tomer
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: amr on March 17, 2010, 01:28:00 PM
thanks Tomer,
I've already noticed the comment and tried to comment out the part for changing draw mode but I didn't see any difference.

for now, I tried to do the suggested ORing.., I didn't get the intended result, but also the change (even if it isn't the required) shows up only for the highlighted filename selection, while unselected items and text viewer contents remain intact as if i didn't make any change

a side question, is there some page discussing fonts and rendering other than this: http://www.rockbox.org/wiki/FontFormat which is short and seems not updated?

thanks
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: amr on March 27, 2010, 10:29:54 AM
I've updated the patch http://www.rockbox.org/tracker/task/11095 with amiconn's suggestion in the referred comment and am waiting for review...
Now the result appears like this:
(http://www.rockbox.org/tracker/task/11095?getfile=21626) (http://www.rockbox.org/tracker/task/11095?getfile=21627)

just something sounds not a good practice in my fix, that's the temp buffer size I reserved statically; is it enough to reserve just 16 bytes instead of 400 ?

I couldn't find a #define stating the max generic glyph bitmap size, while it's not recommended to dynamically allocate memory with a variable runtime value of pf->maxwidth * pf->height / 8
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: tomers on March 27, 2010, 01:23:32 PM
I couldn't find a #define stating the max generic glyph bitmap size, while it's not recommended to dynamically allocate memory with a variable runtime value of pf->maxwidth * pf->height / 8

Hi Amr,

I would like to review your code a bit. I hope you consider my remarks :)

You have calculated the value of 400 according to the equation stated above. Please verify if it shouldn't be like the formula used to calculate bitmap_size in font.c:cache_create(), which adds 7 to the font height. If so, consider if it is better for these functions to use some common helper function (or macro) which calculates buffer size given the font dimensions to calculates this, since that calculation is used in many places in font.c, e.g. load_cache_entry.

Just a thought: I wonder of it is better to have the statically allocated buffer in font.c, and use a it from the lcd-bitmap driver. I guess not, since the font files should be independent on the lcd driver implementation.

Few other comments:
* As suggested in FlySpray, please use a #define for the bitmap size, instead of hard-coding the value 400 in several places.
* Use memset to initialize bits_tmp
* Consider renaming bits_tmp to something better
* You can use bits[k] instead of *bits++ to avoid mixing of two different conventions,
    and use the |= operator:

Code: [Select]
    < bits_tmp[k] = bits_tmp[k] | *bits++;
    > bits_tmp[k] |= bits[k];

* I think that LCDFN(mono_bitmap_part) doesn't necessarily uses the entire buffer, therefore you don't need to actually iterate over the whole bits_tmp buffer (400 times). Try to find how to get the size mono_bitmap_part will use, and set bits_tmp accordingly, saving some redundant iterations.
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: amr on March 27, 2010, 07:07:51 PM
Hi Tomer,
Thanks for the review and of course I'm glad to follow the remarks

Please have a look on the update http://www.rockbox.org/tracker/task/11095

N.B.
400 was just a rough value that didn't have any indication, after that I got the equation from kugel (without considering the height + 7 in font.c)
My calculated working value is 16 but I set it in the updated patch to 32 conservatively.
Title: Re: view Arabic text (cont.) - About the diacritics patch
Post by: tomers on March 30, 2010, 04:21:09 PM
Hi Amr, I'll look at your changes in the near future.

Meanwhile, please review the 'RTL with the text editor? possible?' (http://forums.rockbox.org/index.php?topic=24318.0) forum thread. There's some diacritic support needed in the viewer plugin. It would be great if you can help with this too.

Thanks