Docky

Merge lp://qastaging/~docky-core/docky/appinfo-cleanup into lp://qastaging/docky

appinfo-cleanup
Merge into trunk

Proposed by Rico Tzschichholz on 2010-09-17

Status:	Merged
Merged at revision:	1642
Proposed branch:	lp://qastaging/~docky-core/docky/appinfo-cleanup
Merge into:	lp://qastaging/docky
Diff against target:	184 lines (+54/-54) 2 files modified Docky.Services/Docky.Services/Applications/DesktopItem.cs (+1/-5) Docky.Services/Docky.Services/SystemService.cs (+53/-49)
To merge this branch:	bzr merge lp://qastaging/~docky-core/docky/appinfo-cleanup
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Robert Dyer (community)		Needs Fixing on 2010-09-23
Chris S.	2010-09-17	Approve on 2010-09-23
Review via email: mp+35790@code.qastaging.launchpad.net

Description of the change

Move the AppInfo usage into Docky.Services and clean up it a bit

lp://qastaging/~docky-core/docky/appinfo-cleanup updated on 2010-09-17

1643. By Rico Tzschichholz on 2010-09-17: fix GList leak in SystemService

Revision history for this message

Chris S. (cszikszoy) wrote on 2010-09-23:

Looks good to me. This seems like a code reorganization primarily, I see minimal chance for regressions. Are you sure that this actually fixes a GList leak? How did you test that it doesn't leak anymore?

review: Approve

Revision history for this message

Robert Dyer (psybers) wrote on 2010-09-23:

Move the File.Exists test into the service's Launch method (so you dont have to check if it exists everywhere before calling Launch, which of course is in exactly 1 case *now* but might not be the case later)

Otherwise I approve.

review: Needs Fixing

lp://qastaging/~docky-core/docky/appinfo-cleanup updated on 2010-09-23

1644. By Rico Tzschichholz on 2010-09-23: move application existence check into system services

Revision history for this message

Rico Tzschichholz (ricotz) wrote on 2010-09-23:

> Looks good to me. This seems like a code reorganization primarily, I see
> minimal chance for regressions. Are you sure that this actually fixes a GList
> leak? How did you test that it doesn't leak anymore?

I used your LeakTesting environment ;-)
This works for strings, but of course since GLib.File leaks itself it doesnt fix it in this case.

Revision history for this message

Rico Tzschichholz (ricotz) wrote on 2010-09-23:

> Move the File.Exists test into the service's Launch method (so you dont have
> to check if it exists everywhere before calling Launch, which of course is in
> exactly 1 case *now* but might not be the case later)
>
> Otherwise I approve.

Done