Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: 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

Hi all,

I agree mostly with Thomas, but I can't comment on all of his points,
see below.

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
> unmovable.

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
consent.

> 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,
Bertrik
Received on 2011-09-25


Page was last modified "Jan 10 2012" The Rockbox Crew
aaa