Code review comment for lp:~mterry/unity8/greeter-refactor

Revision history for this message
Michael Terry (mterry) wrote :

> This line is way too long.

Fixed, plus another similar line.

> All those greeter.notifyAppFocused calls look redundant and not really correct.

Well, you are right. Back when I originally added "emergency dialer" support, I tested and the focusChanged signal was not emitted in all cases (like if it was already focused) and not everything went through the requestFocus path (some things were just focused without a request, by the shell). So I had redundant bits in each signal handler.

But not 100% redundant. Each behaved slightly different. In this branch, I've consolidated all that down to one function call per signal handler, and I felt pretty good about that. But you're right, the whole thing looks silly from someone that isn't in the weeds of it.

So stepping back, I retested. And nowadays, we seem to always properly requestFocus, not to mention that a focusChanged signal is emitted in all cases now. So all those signal handlers can consolidate down to one, when focusChanged is received.

So I've done that. Thanks for the kick in the pants! :) I've also added a test to cover the specific case of a re-focus on an already focused app from the greeter. We didn't test that precise case before, but since that one depends on special behavior of re-emitting a changed signal even when nothing changed, a test & comment seemed prudent.

« Back to merge proposal