Code review comment for lp:~azzar1/unity/custom-bg-color-icon

> 186 -// -*- Mode: C++; indent-tabs-mode: nil; tab-width: 2 -*-
> 187 +// -*- Mode: C++; indent-tamos-mode: nil; tab-width: 2 -*-
>
> Ummm.... ??
>
> 422 +
> 423 +
> 424 +
> 425 +
> 426 +
> 427 +
>
>
> Extra blank lines don't need to be there.
>

Oos.

> 439 + def test_custom_background_color(self):
> 440 + """Tests X-Unity-IconBackgroundColor."""
>
> Please change this docstring. I suggest the following:
> """THe X-Unity-IconBackgroundColor property must change the launcher icon's
> background color."""
>
> 441 + path = os.getenv("PYTHONPATH")
> 442 + if path == None:
> 443 + self.skipTest("Cannot run this test if PYTHONPATH has not been
> set.")
>
> Don't do this. For a start, PYTHONPATH can be multiple directories, or it can
> be empty. Your test shouldn't rely on a specific configuration in order to
> run.
> 444 +
> 445 + desktop_file = os.path.join(path, "unity/tests/unity-test.desktop")
> 446 + desktop_file = os.path.realpath(desktop_file)
>
> Again, this won't work if PYTHONPATH is empty or is set to multiple
> directories. If you *really* need to get the path of this file you can do:
>
> os.path.dirname(__file__)
>
> ... which will give you something like '/home/thomi/code/canonical/unity/revie
> ws/branchname/tests/autopilot/unity/tests/'.
>
> 447 +
> 448 + old_value = self.call_gsettings_cmd('get',
> 'com.canonical.Unity.Launcher', 'favorites')
> 449 + value = eval(old_value)
>
> Can you please explain what's going on here? Generally speaking, calling
> 'eval' is a bad idea, and is especially bad when passing it a string that
> comes from elsewhere. What happens when I set that config option to 'import
> subprocess;subprocess.call("rm -rf".split())' ?
>
> 450 + value.append(desktop_file)
> 451 + self.addCleanup(self.call_gsettings_cmd, 'set',
> 'com.canonical.Unity.Launcher', 'favorites', old_value)
> 452 + self.call_gsettings_cmd('set', 'com.canonical.Unity.Launcher',
> 'favorites', str(value))
>
> Again, I'm not sure what this is supposed to do. I'm sure that we can find a
> better way to do it though. The launcher emulator already has a method that
> allows you to add a custom .desktop file to the launcher - can you use that?

No because we need a local .desktop file with a X-Unity-IconBackroundColor key
and we cannot edit a .desktop file in /usr/share/applications.

> If not, let me know what you need and I'll try and come up with a solution for
> you.
>

If you can find another way to test it (with AP) let me know :)

>
> 453 +
> 454 + icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
> 455 + self.assertThat(icon, NotEquals(None))
> 456 + self.assertThat(icon.bg_color_red, Eventually(Equals(0xaa)))
> 457 + self.assertThat(icon.bg_color_green, Eventually(Equals(0xbb)))
> 458 + self.assertThat(icon.bg_color_blue, Eventually(Equals(0xcc)))
> 459 + self.assertThat(icon.bg_color_alpha, Eventually(Equals(0xff)))
> 460 +
>
> This part is fine however :)