[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: Support for PSF2 console fonts
From: Harald Becker <ralda () gmx ! de>
Date: 2010-02-21 15:09:02
Message-ID: 4B814C8E.5020503 () gmx ! de
[Download RAW message or body]
Hallo Denys!
> + * Format of the Unicode information:
> Whatever this comment tries to explain, it fails.
>
I know. Understanding this took me more time than actually adding the
required code. I just copied this comment over from the original setfont
(kbd package). More as a help for me to do things in the right order. A
better explanation describing the font formats is also provided in the
kbd package as html documentation. May be it would be better to delete
that comment replacing it with a reference to the original documentation.
> Are you talking about binary or textual format here?
> "little endian" implies binary, but "UTF-8 coded value" implies text - ?!
>
Yes, that's totally confusing. First at all, this comment tries to
describe both font formats, PSF1 and PSF2. In principal they contain
the same information, but in a different encoding. PSF1 fonts use UCS2
(that is little endian 16 bit values), where as PSF2 fonts encode all
values as UTF-8 (but keep in mind, not the complete file is in this
format, only the unicode table of the font, which follows the binary
font data). Just ignoring the "<seq>*" part may help a bit, because it
was neither used in the fonts of the current gentoo distribution nor is
it implemented in busybox. That means it wasn't implemented before my
changes either, so I skipped this part not adding any extra code. I just
added an error message, if one loads a font which contains such
sequences (wasn't included in the former version of loadfont, which
would probably die without any notice or totally misleading error messages).
In short: The table holds for each glyph position in the font a list of
UCS2 codes (on psf1 fonts) or UTF-8 codes (on psf2 fonts), which should
be displayed as this glyph. The end of each list is a termination
character of 0xFFFF (psf1) or 0xFF (psf2). The termination character is
followed by the list of codes for the next glyph, and so on until all
glyphs of the font (usually 256) got assigned there list of unicode numbers.
For a more thorough description see the linux kbd package:
doc/font-formats/font-formats-1.html
> I am confused. Please rephrase the comment for someone who
> deals with setfont for the first time and is not an Einstein.
> Maybe also give a few examples to make it more clear.
>
Let me think about, if I can write a better comment than the original
one from the kbd package. But it will probably get lengthy to explain it
in detail.
> +comment "Common options für loadfont and setfont"
> + depends on LOADFONT || SETFONT
>
> German word slipped in.
>
Uhps, sorry! No problem to fix this.
> +#if !ENABLE_FEATURE_LOADFONT_PSF1
> +#if !ENABLE_FEATURE_LOADFONT_PSF2
> +#if !ENABLE_FEATURE_LOADFONT_RAW
> +#error You need to enable at least one console font type
> +#endif /* ENABLE_FEATURE_LOADFONT_RAW */
> +#endif /* ENABLE_FEATURE_LOADFONT_PSF2 */
> +#endif /* ENABLE_FEATURE_LOADFONT_PSF1 */
>
> This breaks randomconfig.
>
Compiling loadfont/setfont with at least one of the fonts enabled, would
make the applet useless. It won't harm, but does nothing at all then. So
I decided to produce a compiler message. Maybe it's better to use #warn
to produce just a warning?
> +#if ENABLE_FEATURE_LOADFONT_PSF1 || ENABLE_FEATURE_LOADFONT_PSF2
> +static void do_loadtable(int fd, unsigned char *inbuf, int tailsz, int fontsize, \
> int psf2)
> You have both macro and a variable named "psf2". It is confusing.
>
The macros strip of the table code if neither PSF1 nor PSF2 fonts are
enabled because raw fonts doesn't contain an unicode table. The variable
needs to selects the type of the table when both font types are
included, psf1 and psf2 (think of UCS2 <-> UTF-8). I tried to change the
former psf1 only loadfont as less as possible, so i needed to pass on
the information which type of font gets loaded in any way.
The function load table is just used once. So it would be possible to
move the function body completely to the location where it is called.
Maybe, that would be better and allow some more optimizations. What do
you think?
> - while (tailsz >= 2) {
> + while (tailsz > 0) {
> + if (!psf2) { /* PSF1 */
> +
> +#if ENABLE_FEATURE_LOADFONT_PSF1
> unicode = (((uint16_t) inbuf[1]) << 8) + inbuf[0];
> tailsz -= 2;
> inbuf += 2;
>
> Broken indentation.
>
Uhhhh. That was a file copy in the wrong direction. I fixed it already
and wondered afterwards how it got lost again. Now I know. Sorry, but no
problem to fix.
> + } else if (unicode >= 0xC0) {
> + if (unicode >= 0xFC) unicode &= 0x01, \
> maxct = 5; + else if(unicode >= 0xF8) \
> unicode &= 0x03, maxct = 4; + else if(unicode \
> >= 0xF0) unicode &= 0x07, maxct = 3; + else \
> > if(unicode >= 0xE0) unicode &= 0x0F, maxct = 2;
> + else unicode &= 0x1F, \
> maxct = 1; + do {
> + if (tailsz <= 0 || *inbuf < 0x80 || \
> *inbuf > 0xBF) + \
> bb_error_msg_and_die("illegal UTF-8 character"); + \
> --tailsz; + unicode = (unicode << 6) \
> + (*inbuf++ & 0x3F); + } while( --maxct > 0 \
> ); + } else if (unicode >= 0x80 ) {
>
> Inconsistent style. I almost read that "else" as "....else do {} while();"
>
Ok I tried to adapt to the original coding style as close as possible,
but it differs from my own style. The if ladder is copied over from an
other program of mine and I got a little bit confused during editing and
reformatting. I can move those assignments to their own lines. That
would going a bit lengthy for me, but would make it clear.
> I propose this patch:
>
After a first look it doesn't seem to make any differences in code, so
it's ok. I do check that out, compare it with my development version and
test if it works as expected.
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic