Rockbox

Tasklist

FS#11095 - Fixing the joining algorithm after adding diacritics support

Attached to Project: Rockbox
Opened by Amr Medhat (amr) - Friday, 12 March 2010, 15:59 GMT
Task Type Patches
Category User Interface
Status Unconfirmed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

This patch which is a small change in bidi.c contributes with the diacritics patch (http://www.rockbox.org/tracker/task/10720) to fix the diacritized Arabic text joining problem
This task depends upon

Comment by Amr Medhat (amr) - Friday, 12 March 2010, 16:47 GMT
screen dumps for the a case before adding diacritics support, after adding diacritics support and after the bidi patch
Comment by Amr Medhat (amr) - Sunday, 21 March 2010, 02:22 GMT
A little change in lcd-bitmap-common.c that makes diacritics appear but shifted one character
Comment by Amr Medhat (amr) - Tuesday, 23 March 2010, 17:35 GMT
finally worked by just generalizing drawmode change.
attached the patch and screenshots of the expected results
Comment by Amr Medhat (amr) - Saturday, 27 March 2010, 12:48 GMT
applying amiconn's suggestion
Comment by Tomer Shalev (tomers) - Saturday, 27 March 2010, 16:45 GMT
> + unsigned char bits_tmp[400];
Amr, can you please use a constant instead of using the hard-coded value several times in the code?
Comment by Amr Medhat (amr) - Saturday, 27 March 2010, 22:51 GMT
comments considered
Comment by Tomer Shalev (tomers) - Sunday, 28 March 2010, 17:22 GMT
The attached patch contains the following changes:
- Rename is_diac to diacs, since this variable is used as a counter
- Split long lines
- Simplify loop that writes skipped diacritics

I've tested with Hebrew text that contains diacritics, and it works for me. I would like to commit this soon - Amr, is that ok? I must have your full name for the credits, and to fulfill Rockbox requirements for contributing code (see docs/CONTRIBUTING).

One last thing: I would like you to explain (better have it written as a comment in the code itself) how did you get into the number 32?
Does this number valid for *all targets*, and for *all fonts* ?
I suggest you put a macro instead, with a formula that contains the defines this number is derived from, e.g.
#define GLYPH_BITS_LENGTH (BLAH_BLAH * MAX_FOO / 8)

Thanks,
Tomer

Comment by Amr Medhat (amr) - Monday, 29 March 2010, 06:23 GMT
Thanks Tomer,

I've returned to the non-simple loop of writing skipped diacritics to avoid segmentation fault that happens in special cases when diacs = -1 from a previous decrement causing entering the loop where it shouldn't

Also a comment of the formula has been added : font.height * font.maxwidth / 8 , but I don't know how to make it a macro calculated by the preprocessor while height and maxwidth can be obtained only in runtime, as they are font-dependent...,

I calculated it manually with debugf() by trying out different fonts where height,maxwidth values range from the lowest (8,6) to max (16,16) for GNU_Unifont..., hence I based the hardcoded value on 16 * 16 / 8 = 32 ... I hope I could find some generic value , but there is no stated restriction on font size http://www.rockbox.org/wiki/FontFormat

My full name: Amr Medhat

Thanks
Comment by Amr Medhat (amr) - Friday, 02 April 2010, 12:31 GMT
Changed to runtime buffer allocation (but I get a warning: implicit declaration of buffer_alloc)
(p.s. update of viewer.c since revision 25233 causes problems with diacritized scripts)
Comment by Amr Medhat (amr) - Saturday, 03 April 2010, 03:43 GMT
no warnings now
(but just asking, which is more suitable for our case buffer_alloc() or bufalloc() ?)

Loading...