search

faaxm / spix

UI test automation library for QtQuick/QML Apps
MIT License
188 stars 49 forks source link

Added ability for right & middle mouse clicks #47

Closed RAMAMario closed 2 years ago

RAMAMario commented 2 years ago

Fixes #46

It should not break any existing tests. In AnyRpc I went for a second function name as it seems that the functions are identified by name and a default param should also be difficult with the std::function involved... Should we be consistent and also have the second function name in the plain TestServer?

RAMAMario commented 2 years ago

ah damnit. the unit-tests did not compile for me and I did not notice. Should they not be included in the root cmake?

faaxm commented 2 years ago

ah damnit. the unit-tests did not compile for me and I did not notice. Should they not be included in the root cmake?

They are in the root cmake, but disabled by default unless you provide a -DSPIX_BUILD_TESTS=ON when running cmake. To be honest, I don't remember what the reasoning was to have examples turned on by default, but unit tests turned off, might change that in the future.

I think the problem for the tests and examples currently is that Events.h is a private header and thus they are missing the events constants. I wouldn't want to move the full Events class into the public headers, so maybe create a separate header with just the constants in /lib/include/Spix/Events/, next to KeyCodes.h. Not sure about how to name things... Maybe move the KeyModifiers as struct into KeyCodes.h and have a new MouseButtons.h next to KeyCodes.h?

RAMAMario commented 2 years ago

Ok. I will check it. Not sure when though...

RAMAMario commented 2 years ago

Hm... I don't really get it. Do I have to announce the new header somewhere in the cmake or so?

faaxm commented 2 years ago

Hmmm, it looks like it should have worked... I will try to have a closer look myself over the weekend.

faaxm commented 2 years ago

The issue seems to be that the constants are declared as const static struct members. The tricky thing there is that once they are ODR-used anywhere (i.e. reference/address taken), there has to be an additional declaration at namespace scope. So a static const int MouseButtonCodes::Left; would have to be somewhere outside of the struct. The assignment/initialisation can still happen in the struct.

The reason this popped up now is that when using them as arguments to std::make_unique, they are taken as rvalue reference, which I think translates to const T& for variables, so a reference to the const static is taken, thus odr-use happens. Some compilers (especially when in "Release" mode) worked though, and I suspect that is because in those cases, some optimization happened and the compiler decided to treat the const value as if a literal was passed, then no reference is taken, the odr-use doesn't happen and everyone lives happily ever after. (see also https://en.cppreference.com/w/cpp/language/static close to the end)

I think I would restructure stuff a bit and use enums instead... Do I have permission to commit onto the branch in your fork? If that is ok, I would just push there... Have never tried that on github though, so not sure if that works 😅

RAMAMario commented 2 years ago

I added you as collaborateur to be sure ;-)