search

larpon / QtFirebase

An effort to bring Google's Firebase C++ API to Qt + QML
MIT License
283 stars 83 forks source link

QString::sprintf was removed in Qt 5.14.x? #144

Closed Markus87 closed 3 years ago

Markus87 commented 4 years ago

https://github.com/Larpon/QtFirebase/blob/36141cf33c4f8bd2d1aea326df9c53eb4a4287bd/src/qtfirebasemessaging.cpp#L20

I am not completely sure about it, but it seems QString::sprintf was removed with Qt 5.14.0 . It is marked as deprecated https://doc.qt.io/qt-5/qstring-obsolete.html#sprintf but it does not say anything about that it was removed. In my build with Qt 5.14.1 it´s missing.

I believe it can be replaced with https://doc.qt.io/qt-5/qstring.html#asprintf , it builds but I am not sure yet if it works.

larpon commented 4 years ago

Thanks for reporting it! I haven't noticed it was deprecated. It's even discouraged to use asprintf. I'll see if I can get some time to use a TextStream based version. All it does is using string versions of pointer addresses to give the event callbacks a way to identify where a firebase event is coming from - as these need to be unique.

shujaatak commented 4 years ago

So is it mean that QtFirebase doesn't support Qt5.14.x?

larpon commented 4 years ago

Right now, yes. I'm tied up with other projects. But there's also breaking changes in the Android Manifest file for 5.14.x ...

shujaatak commented 4 years ago

I need to use the latest NDK version (r21) and it is supported by only Qt5.14.2, so I am a bit lost how I would use QtFirebase as you say it doesn't support Qt5.14.x.

larpon commented 4 years ago

It works with 5.14.x but you'll need to make the changes yourself for now (hence 5.14.x is not supported officially). If I push the changes to master now it will break for other users using LTS versions of Qt etc. 🙂 besides that I'm busy with other projects so until I have time - you'll have to replace all sprintf calls, and make the necessary changes in Android Manifest.

Pull Requests are very welcome! Especially for the sprintf fix - because that will hopefully not break anything 🙂

shujaatak commented 4 years ago

I am so sorry to bother you again. Can you please mention what changes in Android Manifest would be required?

By the way I have got no errors while building QtFirebaseExample with Qt5.14.2 and Firebase Version 6.14.0 but the running app doesn't show anything it just show white blank window.

If the changes are small enough then committing those changes to a separate branch would be so much appreciable.

Once I get started, I would definitely post Pull Requests but right now I am just stuck in the first step.

larpon commented 4 years ago

I can't confirm it works but other users have left some hints. See: https://github.com/Larpon/QtFirebaseExample/issues/26

Something about libs.xml

davidzwa commented 4 years ago

@Larpon I would love to help. Where to start on the Qt 5.14.2 fix?

As you mentioned we probably have to use flags to determine Qt version and use a define in code to differentiate between the sprintf calls. Is that something you'd be finding acceptable?

larpon commented 4 years ago

@davidzwa Hi and welcome! 👋 The first thing would be to replace all sprintf calls with an alternative. I'm basically only using them to generate unique id's of the different instances to be able to filter out where events should go. Here is an example - I just need a pointer address -> QString conversion, nothing else, nothing fancy. Maybe a PointerAddressToQString(...) utility function or so.

From there on I think we can provide docs or alternative AndroidManifests versions to help people out.

But those sprintf calls will have to be replaced as a start. It should be easy, I'm just super busy ATM.

Any help would be much appreciated!

davidzwa commented 4 years ago

I got your example working fine with Qt 5.14.2 (I even think without the sprintf replacements, so we can definitely leave that for a later PR). Only changes are indeed in the region of the example's platform changes for Android and nothing constructive in the QtFirebase wrapper itself. (Didnt/couldnt test iOS.)

I did notice that I got an TLS error when testing FCM on Android. We import an Android openssl qt helper in our project. Might be handy for QtFirebaseExample as well?

davidzwa commented 4 years ago

See my PR on https://github.com/Larpon/QtFirebaseExample/pull/29