|
Rockbox mail archiveSubject: Re: jdgordon: r30599 - in trunk/apps: . gui/skin_engineRe: 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 template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |