Code review comment for lp:~zsombi/ubuntu-ui-toolkit/mnemonics

Revision history for this message
Zsombor Egri (zsombi) wrote :

> - m_mouseAttached(false)
> 47 + m_mouseAttached(true),
>
> The default must not change.

Ahh, that's a change which was introduced by the landing fix, where the default got set to false. Good catch!!

>
> // FIXME we should only do this if we have HW keyboard attached
> + if (QuickUtils::instance().keyboardAttached()) {
>
> The comment doesn't make sense - you have an API right there. Better make it a
> FIXME with a reference to lp#.

right... done.

>
> + // FIXME: do this with a proper API
> + connect(&QuickUtils::instance(), &QuickUtils::keyboardAttachedChanged,
> + this, &UCAction::onKeyboardAttached);
>
> Again, add a FIXME with a reference to lp# matching the above.

Same here.

>
> function test_mnemonic_data() {
>
> How about more corner cases that could potentially crash? Such as "Hit&
> Space", "Paste &&&Proceed", "Jump &".

Added them, and I have to check more...

« Back to merge proposal