[Merge] lp:~mir-team/qtubuntu/port-to-mirclient into lp:qtubuntu

Robert Carr racarr at canonical.com
Wed Feb 11 20:42:14 UTC 2015


Thanks sorry for all the small errors. I should read the Qt Style Guide.

>> 
>> - if (ubuntuEvent->window->window() == nullptr) {
>> + if (ubuntuEvent->window == nullptr) {

>> Should the second one be
>>  if (ubuntuEvent->window == nullptr || ubuntuEvent->window->window() == nullptr) {
>> To also cover what the previous check was doing?

Fixed.

>> Any reason you decided not to include the event type anymore? I think it's nice to have it in >> the log, otherwise if it ever happens we'll end up wondering which event was and how it could >> happen.

Fixed.

>> reinterpret_cast<const MirEvent*>(event)

Fixed

>> why not make q_modifiers a Qt::KeyboardModifiers instead of an int so you can save the last 
>> static_cast?
>> why not make q_modifiers a Qt::MouseButton instead of an int so you can save the last 
>> static_cast?

Because operator| casts the Qt::KeyboardModifier to an int and then attempting to assign back to a Qt::Modifier results in an illegal conversion between int and enum types. Enums can not be used as flags in C++ without a cast or operator overloading.

>> This TODO is old?

Why yes it is! 

>> Compiled manually and ran some manual tests and could not find anything wrong, looks code sane >> too (but will wait for giving my approval until the small comments i made above are resolved)

Yay. Thanks again.


Thanks :)



-- 
https://code.launchpad.net/~mir-team/qtubuntu/port-to-mirclient/+merge/245164
Your team Ubuntu Phablet Team is subscribed to branch lp:qtubuntu.



More information about the Ubuntu-reviews mailing list