[haiku-development] Stack & Tile Review
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: "Haiku Development" <haiku-development@xxxxxxxxxxxxx>
- Date: Thu, 29 Oct 2009 12:55:28 +0100
Hi Hong/Christof,
now that Philippe put your Stack & Tile patch into trunk, I finally
took the time to review the code. As you can read on the commit list
(see //www.freelists.org/post/haiku-commits/r33814-haikutrunksrcserversapp,
3), I am not really happy with its current state.
Just some points I stumbled upon:
* Allocations may fail. You *always* have to check for failure when
allocating anything in the app_server, and handle this case gracefully
under all conditions. That's why the app_server uses the WindowList for
lists, as those don't need any additional allocations, and will assure
that the basic functionality will work flawlessly even when there is no
memory left.
* The code is not very much contained - but provides a very specific
feature. While the visual changes (highlighting) seems to be done right,
the behavioral part should be separated more; this would make
maintaining the code much easier, too. Maybe a separate class that is
allocated on demand?
* I miss some overview comments over the functionality - it's hard to
find your way through it.
* Desktop::HighlightTab() uses _RebuildAndRedrawAfterWindowChange() but
currently only changes the color of the tab, so it could be made much
cheaper.
* You introduce a Desktop::GetWindows() which should be called
CurrentWindows(), and replace the private _CurrentWindows() method, not
call it.
* Prefer BObjectList over BList - less casts make cleaner code.
* No need to allocate something separately for each entry if you want
to put an int32 into a list.
* Lots of coding style violations, I already fixed many of them in
trunk, though.
* We usually use "ID" instead of "Id" (didn't change those).
I like the feature in general, and I appreciate all the work you put
into it, but it needs more work until it can be put into the app_server.
Philippe is setting up branch in our repository that you should use as
a base for future work on it, and that should simplify the work flow
(and also allows other developers to fix issues, or enhance the
functionality easier).
Bye,
Axel.
Other related posts:
