Differential D14895 Diff 39897 dataengines/soliddevice/soliddeviceengine.cpp

Changeset View

Standalone View

View Options

dataengines/soliddevice/soliddeviceengine.cpp

Context not available.
24#include <Solid/GenericInterface> 24​#include <Solid/GenericInterface>
25#include <klocalizedstring.h> 25​#include <klocalizedstring.h>
26 26​
27​#include <QApplication>
27#include <QDebug> 28​#include <QDebug>
29​#include <QThread>
broulikUnsubmitted
Done

Unused

broulik: Unused
28#include <KDiskFreeSpaceInfo> 30​#include <KDiskFreeSpaceInfo>
29#include <KFormat> 31​#include <KFormat>
32​#include <KNotification>
30 33​
31#include <Plasma/DataContainer> 34​#include <Plasma/DataContainer>
32 35​
Context not available.
46 } 49​ }
47} 50​}
48 51​
52​Q_DECLARE_METATYPE(KIO::filesize_t)
53​
54​SolidDeviceEngineWorker::SolidDeviceEngineWorker(QString path, QString udi)
55​ : m_path(path)
56​ , m_udi(udi)
57​{
58​}
59​
60​SolidDeviceEngineWorker::~SolidDeviceEngineWorker()
61​{
62​}
63​
64​void SolidDeviceEngineWorker::getFreeSpaceInfo()
65​{
66​ KDiskFreeSpaceInfo info = KDiskFreeSpaceInfo::freeSpaceInfo(m_path);
67​ emit infoObtained(m_path, m_udi, info.isValid(), info.available(), info.size());
anthonyfieroniUnsubmitted
Done

Don't you forget to emit finished?

anthonyfieroni: Don't you forget to emit finished?
68​}
69​
49SolidDeviceEngine::SolidDeviceEngine(QObject* parent, const QVariantList& args) 70​SolidDeviceEngine::SolidDeviceEngine(QObject* parent, const QVariantList& args)
50 : Plasma::DataEngine(parent, args), 71​ : Plasma::DataEngine(parent, args),
51 m_temperature(nullptr), 72​ m_temperature(nullptr),
52 m_notifier(nullptr) 73​ m_notifier(nullptr)
53{ 74​{
54 Q_UNUSED(args) 75​ Q_UNUSED(args)
76​
77​ qRegisterMetaType<KIO::filesize_t>("KIO::filesize_t");
78​
55 m_signalmanager = new DeviceSignalMapManager(this); 79​ m_signalmanager = new DeviceSignalMapManager(this);
56 80​
57 listenForNewDevices(); 81​ listenForNewDevices();
anthonyfieroniUnsubmitted
Done

You can remove it, see below.

anthonyfieroni: You can remove it, see below.
McPainAuthorUnsubmitted
Done

What if it creates another job working on the same path?

The purpose is "check if nobody is working on path", not "save the timer pointer"

McPain: What if it creates another job working on the same path? The purpose is "check if nobody is…
davidedmundsonUnsubmitted
Done

We don't know which window will be active when this timer fires. Which means the behavior to dismiss is somewhat random.

davidedmundson: We don't know which window will be active when this timer fires. Which means the behavior to…
McPainAuthorUnsubmitted
Done

I tested it - works for me.

McPain: I tested it - works for me.
davidedmundsonUnsubmitted
Done

Sure, it /might/ work, but you're still fetching a random window N seconds after a user event.

I don't see how we can know that will result in correct behaviour.

davidedmundson: Sure, it /might/ work, but you're still fetching a random window N seconds after a user event.
McPainAuthorUnsubmitted
Done

Any ideas how to do it correctly?

McPain: Any ideas how to do it correctly?
anthonyfieroniUnsubmitted
Done

It has a default parameters just call event with 3 parameters not 5.

anthonyfieroni: It has a default parameters just call event with 3 parameters not 5.
davidedmundsonUnsubmitted
Done

not sure you need this timer

  1. You should still get the job finishing with error ERR_SERVER_TIMEOUT. (untested, but the protocol manager in the client does have some stuff doing this)
  1. if you don't get a progress bar for a drive that's frozen anyway, do you really care?
davidedmundson: not sure you need this timer 1) You should still get the job finishing with error…
McPainAuthorUnsubmitted
Done

There are solutions to notify user about stuck FS without that timer, right?

McPain: There are solutions to notify user about stuck FS without that timer, right?
davidedmundsonUnsubmitted
Done

I think you can just move the notification to

connect(job, &KIO::FileSystemFreeSpaceJob::result, ...) {

if (job->error() == ERR_SERVER_TIMEOUT) {

....

}
}

but I haven't tested that.

davidedmundson: I think you can just move the notification to connect(job, &KIO::FileSystemFreeSpaceJob…
McPainAuthorUnsubmitted
Done

I did. Job will never finish in that case

McPain: I did. Job will never finish in that case
Context not available.
547 return false; 571​ return false;
broulikUnsubmitted
Done

Give connect a context:

connect(job, &KIO::FileSystemFreeSpaceJob::result, this, ...

otherwise it would blow up if we are deleted before the job finishes

broulik: Give `connect` a context: ``` connect(job, &KIO::FileSystemFreeSpaceJob::result, this, ... ```…
548 } 572​ }
549 573​
550 KDiskFreeSpaceInfo info = KDiskFreeSpaceInfo::freeSpaceInfo(storageaccess->filePath()); 574​ QString path = storageaccess->filePath();
anthonyfieroniUnsubmitted
Done

Capture timer too

anthonyfieroni: Capture timer too
551 if (info.isValid()) { 575​ if (!m_timers.contains(path)) {
552 setData(udi, I18N_NOOP("Free Space"), QVariant(info.available())); 576​ QThread *thread = new QThread;
anthonyfieroniUnsubmitted
Done

You can omit declaration of map, just

timer->stop();
anthonyfieroni: You can omit declaration of map, just ``` timer->stop(); ```
553 setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(info.available())); 577​ SolidDeviceEngineWorker *worker = new SolidDeviceEngineWorker(path, udi);
554 setData(udi, I18N_NOOP("Size"), QVariant(info.size())); 578​
555 return true; 579​ QTimer *timer = new QTimer(this);
580​ timer->setSingleShot(true);
581​ connect(timer, &QTimer::timeout, [path]() {
582​ KNotification::event(KNotification::Error, i18n("Filesystem is not responding", path),
583​ i18n("Filesystem mounted at '%1' is not responding", path),
584​ QApplication::activeWindow(), KNotification::CloseWhenWidgetActivated);
alanjdsUnsubmitted
Not Done

Can we have this number configurable somehow, even if only before startup.

It's been annoying to have notifications when bringing lots of Docker instances at once.

The overhead of _receiving and showing_ the notifications is being longer than the one about bringing the instances up.

alanjds: Can we have this number configurable somehow, even if only before startup. It's been annoying…
mevenUnsubmitted
Not Done

Better approach would be to make this check ignore docker overlay fs.

meven: Better approach would be to make this check ignore docker overlay fs.
alanjdsUnsubmitted
Not Done

That would fix my issue, sure. Idk if targeting "docker" is better than having it configurable, as other similar solutions will have the same problem.

I liked the idea of making docker imune here. May be even better by having a passlist of what should be imune, allowing the user to address issues for similar solutions

alanjds: That would fix my issue, sure. Idk if targeting "docker" is better than having it configurable…
585​ });
586​
587​ m_timers[path] = timer;
588​
anthonyfieroniUnsubmitted
Done
job->start();

as well.

anthonyfieroni: ``` job->start(); ``` as well.
McPainAuthorUnsubmitted
Done

Job starts without it

McPain: Job starts without it
589​ worker->moveToThread(thread);
590​ // start worker after starting thread, and stop thread when finished
591​ connect(thread, SIGNAL(started()), worker, SLOT(getFreeSpaceInfo()));
592​ connect(worker, SIGNAL(finished()), thread, SLOT(quit()));
593​
594​ // delete later thread and worker
595​ connect(worker, SIGNAL(finished()), worker, SLOT(deleteLater()));
596​ connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater()));
597​
598​ // collect and process info
599​ connect(worker, SIGNAL(infoObtained(QString, QString, bool, KIO::filesize_t, KIO::filesize_t)),
600​ this, SLOT(slotInfoObtained(QString, QString, bool, KIO::filesize_t, KIO::filesize_t)),
anthonyfieroniUnsubmitted
Done

Use new connect syntax .

anthonyfieroni: Use new connect syntax .
601​ Qt::QueuedConnection);
602​
603​ // start thread and timer: after 15 seconds we will get an error
604​ thread->start();
605​ timer->start(15000);
556 } 606​ }
557 607​
558 return false; 608​ return false;
559} 609​}
560 610​
611​void SolidDeviceEngine::slotInfoObtained(QString path, QString udi, bool isValid, KIO::filesize_t available, KIO::filesize_t size)
612​{
613​ if (m_timers[path]->isActive())
614​ m_timers[path]->stop();
615​
616​ if (isValid) {
617​ setData(udi, I18N_NOOP("Free Space"), QVariant(available));
618​ setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(available));
619​ setData(udi, I18N_NOOP("Size"), QVariant(size));
620​ }
621​
622​ m_timers[path]->deleteLater();
anthonyfieroniUnsubmitted
Done

It does not need, you connect finished to deleteLater.

anthonyfieroni: It does not need, you connect finished to deleteLater.
623​ m_timers.remove(path);
624​}
625​
561bool SolidDeviceEngine::updateHardDiskTemperature(const QString &udi) 626​bool SolidDeviceEngine::updateHardDiskTemperature(const QString &udi)
562{ 627​{
563 Solid::Device device = m_devicemap.value(udi); 628​ Solid::Device device = m_devicemap.value(udi);
Context not available.