[haiku-commits] Re: r42501 - in haiku/trunk: build/jam src/add-ons/decorators src/servers/app src/servers/app/StackAndTile src/servers/app/decorator
- From: "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Wed, 27 Jul 2011 09:54:44 +0200 (MEST)
clemens.zeidler@xxxxxxxxxxxxxx wrote:
> haiku/trunk/src/servers/app/StackAndTile/
We usually use lower case directory (and static library) names only.
> Move S&T back into the app server.
Nice work, Clemens! I don't have the time to look into your changes in detail,
unfortunately, so I only mention a few coding style issues I noticed while
skimming over this commit message.
> +StaticLibrary StackAndTile.a :
Missing lib prefix? I know it's not really required, but for the sake of
consistency I'd usually add it.
> +#include "SATDecorator.h"
For what is the decorator used exactly still?
Shouldn't it just be a window behaviour?
> +static const rgb_color kTabColor = {255, 203, 0, 255};
> +static const rgb_color kHighlightTabColor = tint_color(kTabColor,
In general, it would be nice if those colors were all user configurable (via
Appearance).
> +WindowArea::MoveWindowToPosition(SATWindow* window, int32 index)
[...]
> + if (!fWindowList.AddItem(window, index))
> + return false;
> + fWindowList.RemoveItemAt(oldIndex);
Moving those the other way around is less likely to fail, but you should really
consider either using your recently added MoveItem(), using a doubly linked
list, as that kind of user action should really not fail because of low memory.
> +bool
> +WindowArea::_AddWindow(SATWindow* window, SATWindow* after)
> +{
> + if (after) {
> + int32 indexAfter = fWindowList.IndexOf(after);
> + if (!fWindowList.AddItem(window, indexAfter + 1))
> + return false;
> + }
> + else if (!fWindowList.AddItem(window))
'else' goes to the previous line.
> +Corner::Corner()
> + :
> + status(kNotDockable),
> + windowArea(NULL)
Why no 'f' prefix here?
> + switch (corner) {
> + case Corner::kLeftTop:
> + return const_cast<Corner*>(&fRightBottom);
Maybe an array would serve better here?
> +int32
> +Tab::FindCrossingIndex(Tab* tab)
> +{
> + if (fOrientation == kVertical) {
> + for (int32 i = 0; i < fCrossingList.CountItems(); i++)
> + if (fCrossingList.ItemAt(i)->HorizontalTab() == tab)
> + return i;
> + } else {
> + for (int32 i = 0; i < fCrossingList.CountItems(); i++)
> + if (fCrossingList.ItemAt(i)->VerticalTab() == tab)
> + return i;
Those are multi-line statements; the for should get {} around its contents.
> +int32
> +Tab::FindCrossingIndex(float pos)
> +{
> + if (fOrientation == kVertical) {
> + for (int32 i = 0; i < fCrossingList.CountItems(); i++)
> + if
> (fCrossingList.ItemAt(i)->HorizontalTab()->Position() == pos)
> + return i;
> + } else {
> + for (int32 i = 0; i < fCrossingList.CountItems(); i++)
> + if (fCrossingList.ItemAt(i)->VerticalTab()->Position()
> == pos)
> + return i;
> + }
Same here.
Bye,
Axel.
Other related posts:
