From: Jonathan Gordon <jdgordy_at_gmail.com>
Date: Sun, 9 Oct 2011 21:51:17 +1100

On 9 October 2011 21:32, Thomas Martitz <kugel_at_rockbox.org> wrote:
> Am 09.10.2011 12:24, schrieb Jonathan Gordon:
>>
>> Did you completly ignore the part where I said it was a *quick proof*
>> that your patch is wrong? I've already admitted that the skin engine
>> is lacking some minor issues *which need to be fixed anyway*. Of
>> course instead of fixing actual issues you want to do another half
>> assed and wrong patch. :<
>
> The issues I found are not minor. I can add to the list that the item you
> click is not what's being selected (it looks like the code that calculates
> the selection doesn't take the empty space into account), which is a pretty
> major issue.
>
>
> Why are you saying it's "another half assed patch"?
>
> Best regards.
>

The issues you mentioned are things that need to be fixed on skin
lists anyway (or that i didnt bother implementing in the 5 line
snippet).

Take your pick:
"Its a half assed patch because:"
a) Either its a usability feature - in which case completely ignoring
the skin engine makes no sense as its hard to argue that the skin isnt
the thing that the user interacts with 100% of the time. Or is a theme
issue in which case doing it only for the inbuilt list is absolutely
incorrect as it is sort of being phased out in favour of skinnable
lists (or indeed native lists).
b) It's a quick band aid fix for your single use case simply because
noone has got round to fixing the issues which prevent this being done
by the skin engine (which need to be done anyway), in effect adding
nothing but bloat and more hacks into an already messy API (the
lcd/viewport framework).

I wouldnt be arguing so aggressively if I hadnt spend the better part
of the last X years working *in the code which this deals with* if
that doesn't count for anything then I don't see the point of the
thread....

I have no issue with the actual feature, just the implementation (and
even then, the configuration bit is fine).
To do it correctly:
* get kinetic scrolling working in skin lists (actually, move it up
one layer so it works directly on the list widget and not the drawn
list. I don't know how it works or what needs fixing to make it work
(needs to be done anyway so is irrelevant to this topic)
* get a lcd draw mode working so transparent doesnt use the background
image (again, something which has been a wishlist item for ages, and
mostly irrelevant to this topic)
* add the logic to automatically create the sbs (finish my attached patch).
* Add logic to extend custom skins to automatically deal with this
(trivial to do, just resize the %Lb rectangle)

Nothing I've said is hard or contentious, just a different way of
dealing with the problem.

Kindest regards
Received on 2011-10-09