Code review comment for lp:~unity-team/unity/unity.hide-cleanup

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Thanks Jason, that's an amazing work!

I'll fix the remaining issues in a further branch. I'm sure that setting last_action_activate to false in the set_hidden() function has some side effect as I did it that way before

280 + _hide_machine->SetQuirk (LauncherHideMachine::LAUNCHER_HIDDEN, hidden);
281 + _hide_machine->SetQuirk (LauncherHideMachine::LAST_ACTION_ACTIVATE, false);

On that part:
This is wrong, I'm sure that we get false positive here, because on some workspace transition, with a slow cpu maybe, the computation is redone between a two ws switch and the we get false positive like: apps in fullscreen on ws1 -> apps in fullscreen on ws2 if the refresh is done at the wrong time.

I'll keep it removed for now, but keeping that in mind…

235 - if (strcmp (event, "begin_viewport_switch") == 0)
1236 - {
1237 - launcher->EnableHiddenStateCheck (false);
1238 - }
1239 - if (strcmp (event, "end_viewport_switch") == 0)
1240 - {
1241 - // compute again the list of all window on the new viewport
1242 - // to decide if we should or not hide the launcher
1243 - launcher->EnableHiddenStateCheck (true);
1244 - launcher->CheckWindowOverLauncher ();
1245 - }
1246 -
1247 -
1248 + launcher->CheckWindowOverLauncher ();

We still have some duplicated variables between the QUIRK and the launcher, I'll ensure to remove them properly.

Also, some false positive, like:
- activate the keynav, mouse doesn't move (but was over launcher), end keynav without doing any move: launcher stays visible
- exiting keynav should exit immediatly.

and as we discussed, fixing the fade effect on bfb as well. Seems you pushed th 0x0 in two places, will get it back to one/ (and push the slide effect by default as well)

(ok, this is more or less ends up as my TODO :))

Apart from those, very nice refactoring! Starting working from there and waiting for the nux branch to be approved and merged! :)

review: Approve

« Back to merge proposal