Code review comment for lp:~mterry/unity8/locking-hash

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

> In general I'm in favour of bracketing single-line if/else()s, but I won't hold it against you.

I added a few more brackets.

> Is there any chance of stuff getting out of sync (like response coming to a different future than expected)? Should we pass around an id of the message and expect it back from the prompt?

Shouldn't be. That's up the to the UI side of things to make sure to give back a response for each prompt in order. Order is important with PAM prompts, so we don't expect a UI component to present them out of order.

> Is there any chance of futureWatcher.result() never returning, or does pam_end() in start() ensure it will get reset every time you go start()?

No, futureWatcher.result() should always return, because it is only called after we get the "finished()" signal from it. And the thread should always finish due to pam_end, yes.

> I tried to run it with ./run.sh, got the password prompt, but couldn't authenticated, that expected?

Fixed. I'm so used to testing on the device, that I didn't notice we still hardcode the phablet user in one place.

> Also see inline.

Replies inline. Except all of a sudden LP wouldn't let me edit or make new comments. So here's a little bit extra:

Regarding whether to call it a mock, I will say that it *is* a mock for liblightdm-qt5. It's just a functional one.

And as for qRegisterMetaType, it only seems to work with that method. Note this section of the documenation for Q_DECLARE_METATYPE:

"Note that if you intend to use the type in queued signal and slot connections or in QObject's property system, you also have to call qRegisterMetaType() since the names are resolved at runtime."

« Back to merge proposal