Fetch text from a site as QString

JonB

@realroot said in Fetch text from a site as QString:

In the function there is no more code:

It is not a question of whether this function has more code. It is a question of whether MyClass::replyFinished() completes all processing of the reply/downloading the file. Which I imagine it does.

Should I make a manager instance as private member of MyClass?

Yes. And do not allocate it more than once! You could alternatively allocate manager within the class instead of with new, i.e. QNetworkAccessManager manager; as a member variable would work. And don't forget you need to call reply->deleteLater() in MyClass::replyFinished(QNetworkReply *reply). As per https://doc.qt.io/qt-6/qnetworkaccessmanager.html#details

Note: After the request has finished, it is the responsibility of the user to delete the QNetworkReply object at an appropriate time. Do not directly delete it inside the slot connected to finished(). You can use the deleteLater() function.

SGaist

In addition to what @JonB wrote, the fact that you pass a parent to your manager object only ensures that it will get destroyed when the parent gets destroyed.

What you currently have is a variant of memory leak since you create new instances of QNetworkAccessManager every time you call that function and they will only get destroyed when your MyClass instance will as well.

realroot

I see thanks.
If I declare it as QNetworkAccessManager manager; I have errors so I made it like this:

private:
    QNetworkAccessManager* m_manager = new QNetworkAccessManager(this);

void MyClass::downloadFile() {
   connect(m_manager, &QNetworkAccessManager::finished,
       this, &MyClass::replyFinished);

   manager->get(QNetworkRequest(QUrl(
       "https://...file")));
}

void MyClass::replyFinished(QNetworkReply *reply) {
   if (reply->error() == QNetworkReply::NoError) {
       QByteArray data = reply->readAll();
       QFile file(<file>);
       if (file.open(QIODevice::WriteOnly)) {
           QTextStream out(&file);
           out << data;
           file.close();
           doSomething(<file>);
       }
   }
   reply->deleteLater();
}

SGaist

@realroot said in Fetch text from a site as QString:

I see thanks.
If I declare it as QNetworkAccessManager manager; I have errors so I made it like this:
private:
    QNetworkAccessManager* m_manager = new QNetworkAccessManager(this);

void MyClass::downloadFile() {
   connect(m_manager, &QNetworkAccessManager::finished,
       this, &MyClass::replyFinished);

Move that connect to the constructor of your class. Otherwise each time you call downloadFile you will create a new connection which means that the slot will be called an additional time.

realroot

Then it should be so I believe:

class MyClass : public QAbstractListModel {
    Q_OBJECT
public:
    MyClass(QObject *parent = nullptr) : QAbstractListModel(parent) {
        connect(m_manager, &QNetworkAccessManager::finished,
                this, &MyClass::downloadFinished);
    }
private:
    QNetworkAccessManager* m_manager = new QNetworkAccessManager(this);

void MyClass::downloadFile() {
    m_manager->get(QNetworkRequest(QUrl(
       "https://...file")));
}

It's working at least.

realroot

If something is wrong let me know, thanks.

JonB

@realroot
It looks reasonable. Although your

QNetworkAccessManager* m_manager = new QNetworkAccessManager(this);

will work personally I would do the new in the MyClass constructor, to the line above where you have moved the connect() like @SGaist said. But maybe that's just me. In any case I believe your code is now acceptable.

realroot

@JonB Like this?

public:
            MyClass(QObject *parent = nullptr) : QAbstractListModel(parent) {
                m_manager = new QNetworkAccessManager(this);
                connect(m_manager, &QNetworkAccessManager::finished,
                        this, &MyClass::downloadFinished);
            }

Pl45m4

@realroot

Yeah, better, but why is everything in your header?
As you add more to your class, it will become pretty crowded and chaotic.

SGaist

C++ now allows to initialize class variable at declaration spot. It's easy and nice for base types however, it can make things harder to read for complex type.

Next, unless you have complex logic associated, you should use your class initializer list and after that the constructor. This will help the compiler optimize some things.

realroot

@Pl45m4 Functions ( downloadFile and replyFinished) are not in the header. I did not specify that but I did add MyClass scope e.g. void MyClass::downloadFile().

@SGaist So:

MyClass(QObject *parent = nullptr) : QAbstractListModel(parent), 
        m_manager(new QNetworkAccessManager(this)) {
        connect(m_manager, &QNetworkAccessManager::finished,
                this, &MyClass::downloadFinished);
}

Pl45m4

@realroot said in Fetch text from a site as QString:

@Pl45m4 Functions ( downloadFile and replyFinished) are not in the header. I did not specify that but I did add MyClass scope e.g. void MyClass::downloadFile().

Don't know what this means, but might be okay.