Code review comment for lp://qastaging/~bilalakhtar/unity/sc-integration-phase2

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

64 + sigc::signal<void, std::string const&, AbstractLauncherIcon::Ptr, std::string const&, std::string const&,
65 + gint32, gint32, gint32> launcher_addrequest_special;

You can just use int at this level, so we are more C++ conformant.

139 + ((SoftwareCenterLauncherIcon*)result.GetPointer())->Animate(launcher_, icon_x, icon_y, icon_size);

Don't use C-style casts.

184 + finished = true;
185 + finished_just_now = true;

Set them using the constructor initializer list.

201 +void SoftwareCenterLauncherIcon::AddSelfToLauncher()
202 +{
203 + _launcher->icon_animation_complete.emit(self_abstract);
204 +}

306 + void AddSelfToLauncher();

322 + AbstractLauncherIcon::Ptr self_abstract;

I don't really think you need those, please don't do that. You already handling an AbstractLauncherIcon.

244 + target_x = (int)current_bamf_icon->GetCenter(launcher->monitor).x;

Again, use static_cast

256 + g_timeout_add(30, &SoftwareCenterLauncherIcon::OnDragWindowAnimComplete, this);

Maybe it can be a little more lazy.
Anyway I'd focus on fixing the problem at the source, instead of using this workaround.

308 + static gboolean OnDragWindowAnimComplete(gpointer data);

Put in private fields.

310 + void Animate(nux::ObjectPtr<Launcher> launcher, gint32 icon_x, gint32 icon_y, gint32 icon_size);

You can also just use Animate(nux::ObjectPtr<Launcher> const& launcher ... And move to pure int's.

312 + void ActivateLauncherIcon(ActionArg arg);

Put it in protected field.

323 + bool finished;
324 + bool finished_just_now;

Add the _ as prefix for private variables. (Actually it would be betteer use a suffixed _, as defined by our coding standards, but in that case you need to update all the members).

For testing, I think you can also emulate something without autopilot by making your LauncherSpecial request to be caused by a fake dbus server you write on tests.

review: Needs Fixing

« Back to merge proposal