[Scummvm-devel] Fwd: [Scummvm-cvs-logs] SF.net SVN: scummvm:[45775] scummvm/trunk/backends/platform

[Max Horn]

Am 09.11.2009 um 22:35 schrieb Max Horn:

>
>
> Anfang der weitergeleiteten E-Mail:
>
>> Von: Pawel Kolodziejski <aquadran at xtr.net.pl>
>> Datum: 9. November 2009 17:21:14 MEZ
>> An: Max Horn <max at quendi.de>
>> Betreff: Re: [Scummvm-cvs-logs] SF.net SVN: scummvm:[45775] scummvm/
>> trunk/backends/platform
>>
>>
>> On 2009-11-09, at 15:58, Max Horn wrote:
>>
>>>
>>> Am 09.11.2009 um 15:29 schrieb aquadran at users.sourceforge.net:
>>>
>>>> Revision: 45775
>>>>       http://scummvm.svn.sourceforge.net/scummvm/?
>>>> rev=45775&view=rev
>>>> Author:   aquadran
>>>> Date:     2009-11-09 14:29:53 +0000 (Mon, 09 Nov 2009)
>>>>
>>>> Log Message:
>>>> -----------
>>>> added samsung tv backend
>>>>
>>>> Added Paths:
>>>> -----------
>>>> scummvm/trunk/backends/platform/samsungtv/
>>>> scummvm/trunk/backends/platform/samsungtv/events.cpp
>>>> scummvm/trunk/backends/platform/samsungtv/graphics.cpp
>>>> scummvm/trunk/backends/platform/samsungtv/hardwarekeys.cpp
>>>> scummvm/trunk/backends/platform/samsungtv/main.cpp
>>>> scummvm/trunk/backends/platform/samsungtv/module.mk
>>>> scummvm/trunk/backends/platform/samsungtv/sdl.cpp
>>>> scummvm/trunk/backends/platform/samsungtv/sdl.h
>>>
>>> This backend seems to duplicate most of the SDL backend -- very
>>> ugly. Any particular reason it couldn't subclass the SDL backend,
>>> instead of duplicating all that code?
>>
>> It's in progress, so subclass not yet, and there are small changes
>> which does not allow much of methods as subclass. Also I don't want
>> put hacks into main sdl code.

Well, this new backend is still 95% identical to the SDL backend. I  
find this rather aggravating :/. As we make changes to the SDL  
backend, they will start to deviate further, though. I am scared by  
the prospect of having this SDL backend clone in there and starting to  
bitrot, just like ther old sdl-opengl backend. The SDL backend already  
contains tons of #ifdefs etc., so on the one hand it is not nice to  
had more, but on the other hand, it's not really making things worse...

But looking at your changes specifically, it seems to me that it would  
still be far preferable and indeed quite doable to realize this as a  
subclassed backend, by turning only a few key methods into virtual  
ones, resp. factoring out some code that you have to implement  
differently for your backend into new virtual methods. This will lead  
to much better code on the long run.

Such a refactoring would be a nice small step towards refactoring the  
SDL backend to make life better for backends derived from it, such as   
the WinCE & Symbian backends.


Disregarding the issues I have with the actual code, there is one  
other thing: I really really *really* dislike learning about a new  
backend being added via the commit message. This is just as bad as if  
it happens for an engine. This is really not good practice. In the  
future, I expect everybody who adds major new components like backends  
or engines to announce these *beforehand* on -devel, and wait for  
approval. Getting approval from either me or Eugene alone is not  
enough, this must go over -devel. Anyway, it's too late for that now;  
but please at the very least, write an email to -devel now which  
explains your new backend, what it is good for, and what your plans  
are to address the concerns I voiced above.


Bye,
Max