From: Jonathan Gordon <jdgordy_at_gmail.com>
Date: Mon, 18 Feb 2013 10:50:25 +1100

On 18 February 2013 07:40, Thomas Martitz <kugel_at_rockbox.org> wrote:

> Am 16.02.2013 10:46, schrieb Jonathan Gordon:
>
>
>>
>>
>> On 16 February 2013 08:30, Thomas Martitz <kugel_at_rockbox.org <mailto:
>> kugel_at_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/<http://gerrit.rockbox.org/r/#/c/384/>
>> [2]: https://github.com/kugel-/**rockbox/tree/newline-api<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 separator).
>>
>>
>> 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.
>

OK, so its clear I (and wodz) misunderstood, do you mind please explaining
exactly what you're doing again to clear it up? I don't understand what the
point of having a (almost?) wrapper around lcd_puts* in apps/ if really the
only place it is called is gui/bitmap/list.c in one place? Everywhere else
which is is called (no major screens) wouldn't make use of whatever is
being added?

If all you want is a line separator then why is it not a simple 1 line
lcd_hline() call in the draw_list() loop?

Thinking about it now, it would be pretty cool to register a callback (per
viewport) for the scroll engine to call which would do the line drawing
instead of the current code. (It would have to be per viewport because it
is very likley to have a skin scrolling line and the list scrolling line
both scrolling at the same time, though a generic one would be needed for
default which should "just work").

Jonathan
Received on 2013-02-18