Rockbox mail archiveSubject: Re: jdgordon: r30599 - in trunk/apps: . gui/skin_engine
Re: jdgordon: r30599 - in trunk/apps: . gui/skin_engine
From: Bertrik Sikken <bertrik_at_sikken.nl>
Date: Sun, 25 Sep 2011 23:13:33 +0200
I agree mostly with Thomas, but I can't comment on all of his points,
On 25-9-2011 4:54, Thomas Martitz wrote:
> I want to discuss that commit as I disagree with it in some ways,
> perhaps even see it reverted.
> Clearly it introduced a user configurable skin buffer. That is something
> we discussed many times before with no clear outcome. In fact, in my
> memory there was a general opposition. I feel it silently introduced a
> NoDo, although the wiki page doesn't list it.
> So I'm bitter that this is introduced via a drive-by commit with no
> mention about it at all. This is not the policy I want to see for
> controversial changes.
> It is especially strange that it's introduced now as the vast majority
> of the skin buffer is truly dynamic through buflib (images + backdrops +
> fonts). The skin buffer was a multi-100K thing before buflib; now the
> configuration is made for a fraction of this. We didn't (want to) have
> this configuration when it was still multiple 100K big.
> Furthermore this user setting is implemented in a totally obscure way (a
> magic file containing a magic number, which isn't checked if it's even
> plausible). It's pattern that doesn't exist yet, while there's a clear
> existing implementation of user settings. It's rather hidden without a
> chance to be discovered by the user. I don't think I need to mention
> that the manual doesn't tell anything about this.
This is my main objection.
> Lastly, I complain (also not very strongly) about how it exploits
> buflib. It makes an unmovable buffer, which should simply not exist,
> because they throw away with the entire point of buflib. The exploit
> goes further by forcing it to be the very first allocation. This is a)
> introducing assumption, and b) breaks when something else needs to be
I consider kugel/Thomas to be our resident buflib expert (at least now)
so I'm a bit surprised this appears to have been committed without his
> I understand it's much work to make the skin buffer movable, and it
> might not happen at all. But this is much much less of a problem than
> before. And we never wanted a configurable skin buffer. I think the
> commit deserves _at least_ some discussion.
With kind regards,
Received on 2011-09-25