Am 16.02.2013 10:46, schrieb Jonathan Gordon:
On 16 February 2013 08:30, Thomas Martitz <ku...@rockbox.org
<mailto:ku...@rockbox.org>> wrote:
Hello guys,
I'm working on a new line print API in apps that's supposed to
replaces most of lcd_puts_* and lcd_putsxy_*.
The lcd_puts* became really messy and it still doesn't support
scrolling properly (not at all for pixel based functions). The
rework I'm working on will hopefully be simplified and fix
scrolling, while allowing more control over the line style and
contents.
My motivation is to properly implement [1] (line separator in
lists). This line separator cannot be properly implemented just in
apps because scrolling draws over it, and scrolling is entirely in
firmware and calls only firmware function. To not further
complicate lcd_puts* a rework is needed.
My idea is a having a single function:
put_line(int x, int y, struct line_desc *desc, const char *fmt, ...).
- x, y are the position of the line (in pixels!).
- struct line_desc defines other properties the line: style (for
line selectors, and the line separators mentioned above),
scrolling, line height and multiline information.
- fmt is a format string that controls the contents of the line.
Similar to printf() tags can be used to put icons, text and
margins (perhaps other stuff too in the future), the variable
paramter list is then used to build the contents
There are some complications, most notably scrolling, multiline
and RTL, which I can hopefully work out soon (i think I will solve
scrolling by setting up a callback so apps code can do the actual
drawing). As if now there is a preview of my work in [2], see
apps/gui/bitmap/list.c for an example of how to use the new api.
So, what do you think? Is going forward worthwhile and welcome? Do
you have comments/remarks/suggestions as to the proposed API function?
Best regards.
[1]: http://gerrit.rockbox.org/r/#/c/384/
[2]: https://github.com/kugel-/rockbox/tree/newline-api
Please don't add any non-text drawing to the lcd_put* API. That should
do nothing but position text. I think you're going about this the
wrong way (or at least doing too much at the firmware level).
Sorry, I wasn't clear enough. My intent is to put put_line() into apps/.
I totally agree that non-text stuff shouldn't be added to the firmware
level, which is why I even make this work (to avoid putting the line
separator to firmware which I *really* don't want). The current patch
has it in apps/.
Being in apps is also a requirement for extending the line selector to
the icons and indent level (right now the line selector is only drawn
below the text portion of a line), which is something I really want and
already implemented.
Eventually the line selector drawing (i.e. line styles) could be removed
from firmware once a sufficient apps API is in place.
I like the idea of registering a callback for scrolling lines, this
would potentially fix existing issues with the list indenting, however
I don't think this is the way to fix this (and your line seperator).
My suggestion is:
1) fix scrolling so it works on a pixel level and not a line level. We
are well past the time when scrolling on a line level (and using the
entire line) is outdated. If scrolling can be told the pixel rectangle
(or sub viewport would be better/simpler) to scroll in then you are
90% of the way to what you want.
This is on my TODO list, but
2) modify list.c to know the difference between the text height and
line height. I'm guessing this is the actual issue you're aving with
line seperater (though your line height patch hsold have done this
anyway?)
list.c already knows the difference, since the line height patch. What
do you mean by "this is the actual issue"? list separator and line
height are barely related.
3) Draw the line separator in the list UI which is the only correct
place to put it. Sure, you could piggy-back off the gradient code and
draw a line manually but please remember that everything in the UI
that is drawn should be customizable (maybe not from the start, but
certainly not modified to make it impossible later). (tangent: the
gradient code is really in the wrong place too, the list should be
drawing the gradient and then drawing the text over the top, not the
lcd_* code).
This is already implemented by the current version.
So yes, the lcd_put* API is long outdated and crud, but changing it
is orthogonal to your motivation.
I don't want to change it. Not now at least. And I particularly dont
want to expand it. This work makes some of lcd_put* obsolete so the API
can be cleaned up in a follow-up change.
Best regards.
