[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