Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

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

Attached to Project: Rockbox
Opened by Amr Medhat (amr) - Friday, 12 March 2010, 16:59 GMT+2
Task Type Patches
Category User Interface
Status Unconfirmed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 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
   bidi.patch (2.3 KiB) 
 firmware/bidi.c |   33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

This task depends upon

Comment by Amr Medhat (amr) - Friday, 12 March 2010, 17:47 GMT+2
screen dumps for the a case before adding diacritics support, after adding diacritics support and after the bidi patch
   before_diacritics1.bmp (75.7 KiB)
   before_diacritics2.bmp (75.7 KiB)
   after_diacritics1.bmp (75.7 KiB)
   after_diacritics2.bmp (75.7 KiB)
   after_diacritics3.bmp (75.7 KiB)
   after_patch1.bmp (75.7 KiB)
   after_patch2.bmp (75.7 KiB)
   after_patch3.bmp (75.7 KiB)
Comment by Amr Medhat (amr) - Sunday, 21 March 2010, 03:22 GMT+2
A little change in lcd-bitmap-common.c that makes diacritics appear but shifted one character
   arab_diac (2.9 KiB)
Comment by Amr Medhat (amr) - Tuesday, 23 March 2010, 18:35 GMT+2
finally worked by just generalizing drawmode change.
attached the patch and screenshots of the expected results
   arab_diac.patch (3.4 KiB) 
 firmware/bidi.c                      |   33 ++++++++++++++++++++++++++-------
 firmware/drivers/lcd-bitmap-common.c |   12 +++++-------
 2 files changed, 31 insertions(+), 14 deletions(-)

   fixed1.bmp (75.7 KiB)
   fixed2.bmp (75.7 KiB)
Comment by Amr Medhat (amr) - Saturday, 27 March 2010, 13:48 GMT+2
applying amiconn's suggestion
   arab_diac.patch (5.1 KiB) 
 firmware/bidi.c                      |   33 ++++++++++++++++++++++++++-------
 firmware/drivers/lcd-bitmap-common.c |   34 ++++++++++------------------------
 2 files changed, 36 insertions(+), 31 deletions(-)

Comment by Tomer Shalev (tomers) - Saturday, 27 March 2010, 17:45 GMT+2
> + 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, 23:51 GMT+2
comments considered
   arab_diac.patch (6.1 KiB) 
 firmware/bidi.c                      |   33 +++++++++++++++++----
 firmware/drivers/lcd-bitmap-common.c |   53 ++++++++++++++---------------------
 2 files changed, 48 insertions(+), 38 deletions(-)

Comment by Tomer Shalev (tomers) - Sunday, 28 March 2010, 19:22 GMT+2
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

   arab_diac_v5.diff (6 KiB) 
 firmware/bidi.c                      |   34 ++++++++++++++++++----
 firmware/drivers/lcd-bitmap-common.c |   53 ++++++++++++++---------------------
 2 files changed, 50 insertions(+), 37 deletions(-)

Comment by Amr Medhat (amr) - Monday, 29 March 2010, 08:23 GMT+2
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
   arab_diac.patch_v6.diff (6.1 KiB) 
 firmware/bidi.c                      |   36 +++++++++++++++++++----
 firmware/drivers/lcd-bitmap-common.c |   53 ++++++++++++++---------------------
 2 files changed, 52 insertions(+), 37 deletions(-)

Comment by Amr Medhat (amr) - Friday, 02 April 2010, 14:31 GMT+2
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)
   arab_diac_v7.patch (5.9 KiB) 
 firmware/bidi.c                      |   36 +++++++++++++++++++----
 firmware/drivers/lcd-bitmap-common.c |   54 ++++++++++++++---------------------
 2 files changed, 53 insertions(+), 37 deletions(-)

Comment by Amr Medhat (amr) - Saturday, 03 April 2010, 05:43 GMT+2
no warnings now
(but just asking, which is more suitable for our case buffer_alloc() or bufalloc() ?)
   arab_diac_v8.patch (6.2 KiB) 
 firmware/bidi.c                      |   36 +++++++++++++++++++---
 firmware/drivers/lcd-bitmap-common.c |   55 +++++++++++++++--------------------
 2 files changed, 54 insertions(+), 37 deletions(-)

Loading...