Differential D14895 Diff 40973 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>
28#include <KDiskFreeSpaceInfo> ​
29#include <KFormat> 29​#include <KFormat>
broulikUnsubmitted
Done

Unused

broulik: Unused
30​#include <KIO/FileSystemFreeSpaceJob>
31​#include <KNotification>
30 32​
31#include <Plasma/DataContainer> 33​#include <Plasma/DataContainer>
32 34​
anthonyfieroniUnsubmitted
Done

Don't you forget to emit finished?

anthonyfieroni: Don't you forget to emit finished?
Context not available.
547 return false; 550​ return false;
548 } 551​ }
549 552​
550 KDiskFreeSpaceInfo info = KDiskFreeSpaceInfo::freeSpaceInfo(storageaccess->filePath()); 553​ QString path = storageaccess->filePath();
551 if (info.isValid()) { 554​ if (!m_paths.contains(path)) {
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
552 setData(udi, I18N_NOOP("Free Space"), QVariant(info.available())); 555​ QTimer *timer = new QTimer(this);
553 setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(info.available())); 556​ timer->setSingleShot(true);
554 setData(udi, I18N_NOOP("Size"), QVariant(info.size())); 557​ connect(timer, &QTimer::timeout, [path]() {
555 return true; 558​ KNotification::event(KNotification::Error, i18n("Filesystem is not responding", path),
559​ i18n("Filesystem mounted at '%1' is not responding", path));
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.
560​ });
561​
562​ m_paths.insert(path);
563​
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…
564​ // create job
565​ KIO::FileSystemFreeSpaceJob *job = KIO::fileSystemFreeSpace(QUrl::fromLocalFile(path));
566​
567​ // delete later timer
568​ connect(job, &KIO::FileSystemFreeSpaceJob::result, timer, &QTimer::deleteLater);
569​
570​ // collect and process info
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, ... ```…
571​ connect(job, &KIO::FileSystemFreeSpaceJob::result, this,
572​ [this, timer, path, udi](KIO::Job *job, KIO::filesize_t size, KIO::filesize_t available) {
573​ timer->stop();
anthonyfieroniUnsubmitted
Done

Capture timer too

anthonyfieroni: Capture timer too
574​
575​ if (!job->error()) {
anthonyfieroniUnsubmitted
Done

You can omit declaration of map, just

timer->stop();
anthonyfieroni: You can omit declaration of map, just ``` timer->stop(); ```
576​ setData(udi, I18N_NOOP("Free Space"), QVariant(available));
577​ setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(available));
578​ setData(udi, I18N_NOOP("Size"), QVariant(size));
579​ }
580​
581​ m_paths.remove(path);
582​ });
583​
584​ // start timer: after 15 seconds we will get an error
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​ timer->start(15000);
anthonyfieroniUnsubmitted
Done
job->start();

as well.

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

Job starts without it

McPain: Job starts without it
556 } 586​ }
557 587​
558 return false; 588​ return false;
Context not available.
anthonyfieroniUnsubmitted
Done

It does not need, you connect finished to deleteLater.

anthonyfieroni: It does not need, you connect finished to deleteLater.
anthonyfieroniUnsubmitted
Done

Use new connect syntax .

anthonyfieroni: Use new connect syntax .