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

DolphinQt2: Add missing Q_OBJECT macro to all QObject-related classes missing it #6850

Merged
merged 1 commit into from
May 14, 2018

Conversation

lioncash
Copy link
Member

Without this macro, if any signals or slots were attempted to be used, they wouldn't work; neither would various other features of the Qt meta-object system. This can also lead to weird behavior in other circumstances. Qt's documentation specifically states:

Therefore, we strongly recommend that all subclasses of QObject use the Q_OBJECT macro regardless of whether or not they actually use signals, slots, and properties.

on its page for "The Meta-Object System", which can be seen here: https://doc.qt.io/qt-5/metaobjects.html

Let's opt for "always do the right thing", and keep the code extensible for the future and not have random things blow up on us.

… missing it

Without this macro, if any signals or slots were attempted to be used,
they wouldn't work; neither would various other features of the Qt
meta-object system. This can also lead to weird behavior in other
circumstances. Qt's documentation specifically states:

"Therefore, we strongly recommend that all subclasses of QObject use the
Q_OBJECT macro regardless of whether or not they actually use signals,
slots, and properties."

on its page for "The Meta-Object System", which can be seen here:
https://doc.qt.io/qt-5/metaobjects.html

Let's opt for "always do the right thing", and keep the code extensible
for the future and not have random things blow up on us.
@spycrab
Copy link
Contributor

spycrab commented May 14, 2018

Still questionable IMHO since we have to add all moc'd header files to our vcxproj manually which means we're not doing much in terms of future-proofing.

@lioncash
Copy link
Member Author

lioncash commented May 14, 2018

This, I would not consider a failing of moc (rather the fact we don't use CMake for Windows builds), we also have code-review to catch things like that.

From a code perspective, it prevents things from randomly blowing up. In the case of not doing this, and it blows up on someone, they'd be adding the relevant files to the vcxproj file anyways if they want their code to compile. If they don't know how vcxproj files work with Qt, then it just becomes a needless pain in the ass for that person. This gets the bulk out of the way, so that's not even a necessary step. New files will just be audited as they're added.

@spycrab
Copy link
Contributor

spycrab commented May 14, 2018

This, I would not consider a failing of moc

Of course not moc's fault.

we also have code-review to catch things like that.

Then again a missing Q_OBJECT macro would be caught by reviewers as well (Also probably by the person compiling it).

From a code perspective, it prevents things from randomly blowing up.

Seems to be pretty predictable tbh. Things like signals and such won't work.

Not blocking this btw, only pointing out that we should probably make sure the macros that are placed by this PR actually get parsed by moc on every platform we support and they don't just kinda sit there.

@lioncash
Copy link
Member Author

I tested prior to opening the PR; they parse fine

@spycrab
Copy link
Contributor

spycrab commented May 14, 2018

They don't get parsed by moc on every platform.

@lioncash
Copy link
Member Author

You're going to have to elaborate on what you mean. I've tested this on macOS, Windows, and both on Debian and a Fedora installation, and they generate the necessary source files and compile fine.

@spycrab
Copy link
Contributor

spycrab commented May 14, 2018

If we don't add files manually into the Moc section in our .vcxproj, the Q_OBJECT macro will get ignored. That's why I think this change only makes sense in conjunction with either
(a) Adding all files that include a Q_OBJECT macro into this section
(b) Actually using automoc / CMake on Windows

@lioncash
Copy link
Member Author

Is that not what I've done in the change itself? (specifically (a))

Copy link
Contributor

@spycrab spycrab left a comment

Choose a reason for hiding this comment

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

Oh god, now I see it >.<, really sorry for wasting your time: GitHub's search for file feature threw me off (still my fault for being stupid tho). LGTM now!

@spycrab spycrab merged commit 26b1048 into dolphin-emu:master May 14, 2018
@lioncash
Copy link
Member Author

Noooo worries, I've been hit by the search (sometimes, even the diff) being a little wonky as well; it happens.

@lioncash lioncash deleted the moc branch May 14, 2018 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants