psi-im / iris

XMPP network library
GNU Lesser General Public License v2.1
50 stars 25 forks source link

Qt5-compliant implementation of is_gui_app() #39

Closed aclex closed 8 years ago

aclex commented 8 years ago

Avoid using of obsolete (since Qt5) members of QApplication used to check if we are GUI application. Use dynamic_cast to QGuiApplication instead, which could be a little slower, but seems to be the recommended way.

Turned out to be important for KF5-porting of Kopete, which uses internal copy of libiris.

Ri0n commented 8 years ago

I believe it should be qobject_cast<QApplication *>(QCoreApplication::instance()) Isn't it?

aclex commented 8 years ago

Agreed on qobject_cast, but as to casting to QApplication it seems a little tricky: according to the docs, QApplication implies QWidget-based GUI application, so e.g. for QML-based one it would answer false this way. Unfortunately, I don't know the context for this method, so it's hard to say, what behaviour is desired.

Ri0n commented 8 years ago

Well Ok, let it be QGui* My example was copied from commit deprecating type().

aclex commented 8 years ago

Ok, I seem to have consulted the same commit :) Could you please fix it while merging or should I update the request somehow?

Ri0n commented 8 years ago

Please update the request. You can rewrite history and force push so it still will be just one commit.

aclex commented 8 years ago

Thank you for the suggestion, just updated it this way.

aclex commented 8 years ago

Thank you very much for the merge!

tehnick commented 8 years ago

You have dropped Qt 4.x support due to using of QGuiApplication here.

aclex commented 8 years ago

Oh yes, indeed, that was completely slipped my mind. QApplication, as Ri0n proposed, would be the optimal then. Do we have any chance to rewrite it again?

Ri0n commented 8 years ago

new pool request then. I didn't notice this too =)

Ri0n commented 8 years ago

I'd add few ifdefs

aclex commented 8 years ago

Yeah, just done this way: https://github.com/psi-im/iris/pull/40