Merge lp:~jassmith/do/docky-random-polish-and-breakage into lp:do
Proposed by
Jason Smith
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Merged at revision: | not available | ||||||||||||
Proposed branch: | lp:~jassmith/do/docky-random-polish-and-breakage | ||||||||||||
Merge into: | lp:do | ||||||||||||
To merge this branch: | bzr merge lp:~jassmith/do/docky-random-polish-and-breakage | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
David Siegel (community) | Approve | ||
Review via email: mp+3319@code.launchpad.net |
To post a comment you must log in.
DockServices.cs ervice instead of reinventing the wheel. TService> .FirstOrDefault (), then check
Does this class need to be static? Have you thought about using the existing
services class instead of duplicating it here? You can just use IService,
IInitializedS
Use a static constructor to initialize fields.
Please don't use the E1 ?? (E1 = E2) form; use if (E1 == null)...
For items_service, drawing_service, do_interop_service, just new those in the
static constructor and use auto-properties.
For LoadService, you should use OfType<
for null and use the Activator if you need it. Or get fancy with ??
DoInteropService.cs
You may want to use EventHandler and pass (this, EventArgs.Empty) instead of
using Action.
Default. DrawingService. cs
Looks good.
Default. ItemsService. cs Draggable
Can you make your fields more generic -- IDictionary instead of Dictionary,
ICollection instead of List.
s/Dragable/
ItemCanBeMoved (int item) Contains (item);
return DockItems [item] is DockItem && ItemCanBeMoved (DockItems [item]);
ItemCanBeMoved (DockItem item)
return DraggableItems.
It's very weird that you case DockItems [item] to DockItem.
DropItemOnPosi tion, MoveItemToPosition:
Don't repeat yourself. Make the following its own method:
if (DockItems.Contains (item) && 0 <= position && position < DockItems.Count)
MoveItemToPostion (int, int) doesn't do upper bounds checking.
DockPreference s.AutomaticIcon s--
is a bit scary
Enumerable. ToDictionary throws a not implemented exception on mono < 2.0
You should writegeneric Serialize<T> and Deserialize<T> methods instead of
repeating yourself. You can overload them with Funcs if you want to alter the
datastructure before/after serialization, which you seem to be doing.
UpdateWindowItems
first foreach, you have a line ending with '.'
SimplifyPositions:
s/i=0/i = 0
Why do you call ToArray?
Default. PainterService. cs:
Bad file name in header comment.
Use Log<PainterService>
IItemsService.cs
Don't use List, use the thinnest interface you can -- IEnumerable is best.
It's very weird to have a readonly property for a mutable type.
You should not have overloads in your interface. Do calling classes really use
both versions of MoveItemToPosition and RemoveItem? That may be a sign of a
poor design decisison.
A few comments would be nice!
IPainterService.cs
Wrong filename in heaer comment
ApplicationDock Item.cs rectories should be static readonly IEnumerable< string> , not a
DesktopFilesDi
property.
BaseDockItem.cs
MakeIconSurface
if (icon_surface == null)
icon_surface = ...
return icon_surface;
DoDockItem.cs
const "Summon GNOME Do" at the top of the file
DoInteropService
Why do you have IDoInteropService and DoInteropService?
DockArea.cs:
BaseDockItem needs to be public?
Magic numbers 22, 300, 500?
Your AnimationStateC onditions could hav better indentation (I still think the
string-based criteria are a bit wierd, would prefer enum). Now that I read
further and see you writing down "PainterAnimation" over and over again, I
*really* think you should be using an enum.
You frequently place an unneeded ( ) around the predicate in your ternary
e...