ubuntu-touch-coreapps-reviewers team mailing list archive

[Arto Jalkanen <ajalkane@xxxxxxxxx>]

Some comments:

107	+      shares  = smb.lisShares();

listShares() ?

138	+        m_thread->wait();

This could block forever. Should a reasonable timeout instead be used?

151 +    emit working(true);
152	+    m_sharesList = smb.lisShares();
153	+    emit working(false);

I don't think this works as intended. Emitting signals from the same thread do not get delivered until control is returned from the function. So this would mean emit working(true) and emit working(false) would both get delivered after smb.lisShares() had already done its work.

183	+QStringList SmbPlaces::gePlaces() const

getPlaces() ?

352	+        QStringList paths = path_var.split(QLatin1Char(':'));
353	+        for(int counter = 0; !ret && counter < paths.count(); ++counter)
354	+        {
355	+#if defined(Q_OS_WIN)
356	+            QFileInfo net(paths.at(counter) + QLatin1String(".exe"));

There's split by ':' character, but define makes support for Windows which uses ';' to separate paths. I don't think Windows support is needed, but if it's included should split on Windows using ';'.


-- 
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-browsing-11/+merge/252982
Your team Ubuntu File Manager Developers is requested to review the proposed merge of lp:~carlos-mazieri/ubuntu-filemanager-app/samba-browsing-11 into lp:ubuntu-filemanager-app.