Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ability for right & middle mouse clicks #47

Merged
merged 5 commits into from
Dec 21, 2021

Conversation

RAMAMario
Copy link
Contributor

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
Copy link
Contributor Author

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 faaxm self-requested a review December 15, 2021 10:20
@faaxm
Copy link
Owner

faaxm commented Dec 15, 2021

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
Copy link
Contributor Author

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

@RAMAMario
Copy link
Contributor Author

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

@faaxm
Copy link
Owner

faaxm commented Dec 17, 2021

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

@faaxm
Copy link
Owner

faaxm commented Dec 20, 2021

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
Copy link
Contributor Author

I added you as collaborateur to be sure ;-)

Copy link
Contributor Author

@RAMAMario RAMAMario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for the help.

@faaxm faaxm merged commit d260b51 into faaxm:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mouse right click
2 participants