Differential D14895 Diff 41511 dataengines/soliddevice/soliddeviceengine.cpp

Changeset View

Standalone View

View Options

dataengines/soliddevice/soliddeviceengine.cpp

Show All 18 Lines
19#include "soliddeviceengine.h" 19​#include "soliddeviceengine.h"
20#include "soliddeviceservice.h" 20​#include "soliddeviceservice.h"
21 21​
22#include <QMetaEnum> 22​#include <QMetaEnum>
23#include <QDateTime> 23​#include <QDateTime>
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​
33//TODO: implement in libsolid2 35​//TODO: implement in libsolid2
34namespace 36​namespace
35{ 37​{
36 template <class DevIface> DevIface *getAncestorAs(const Solid::Device &device) 38​ template <class DevIface> DevIface *getAncestorAs(const Solid::Device &device)
37 { 39​ {
Show All 19 Lines55{
57 listenForNewDevices(); 59​ listenForNewDevices();
58 setMinimumPollingInterval(1000); 60​ setMinimumPollingInterval(1000);
59 connect(this, &Plasma::DataEngine::sourceRemoved, 61​ connect(this, &Plasma::DataEngine::sourceRemoved,
60 this, &SolidDeviceEngine::sourceWasRemoved); 62​ this, &SolidDeviceEngine::sourceWasRemoved);
61} 63​}
62 64​
63SolidDeviceEngine::~SolidDeviceEngine() 65​SolidDeviceEngine::~SolidDeviceEngine()
64{ 66​{
65} 67​}
anthonyfieroniUnsubmitted
Done

Don't you forget to emit finished?

anthonyfieroni: Don't you forget to emit finished?
66 68​
67Plasma::Service* SolidDeviceEngine::serviceForSource(const QString& source) 69​Plasma::Service* SolidDeviceEngine::serviceForSource(const QString& source)
68{ 70​{
69 return new SolidDeviceService (this, source); 71​ return new SolidDeviceService (this, source);
70} 72​}
71 73​
72void SolidDeviceEngine::listenForNewDevices() 74​void SolidDeviceEngine::listenForNewDevices()
73{ 75​{
▲ Show 20 Lines • Show All 468 Lines • ▼ Show 20 Line(s)
542{ 544​{
543 Solid::Device device = m_devicemap.value(udi); 545​ Solid::Device device = m_devicemap.value(udi);
544 546​
545 Solid::StorageAccess *storageaccess = device.as<Solid::StorageAccess>(); 547​ Solid::StorageAccess *storageaccess = device.as<Solid::StorageAccess>();
546 if (!storageaccess || !storageaccess->isAccessible()) { 548​ if (!storageaccess || !storageaccess->isAccessible()) {
547 return false; 549​ return false;
548 } 550​ }
549 551​
550 KDiskFreeSpaceInfo info = KDiskFreeSpaceInfo::freeSpaceInfo(storageaccess->filePath()); 552​ QString path = storageaccess->filePath();
551 if (info.isValid()) { 553​ if (!m_paths.contains(path)) {
552 setData(udi, I18N_NOOP("Free Space"), QVariant(info.available())); 554​ QTimer *timer = new QTimer(this);
553 setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(info.available())); 555​ timer->setSingleShot(true);
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
554 setData(udi, I18N_NOOP("Size"), QVariant(info.size())); 556​ connect(timer, &QTimer::timeout, [path]() {
555 return true; 557​ KNotification::event(KNotification::Error, i18n("Filesystem is not responding", path),
558​ i18n("Filesystem mounted at '%1' is not responding", path));
559​ });
560​
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.
561​ m_paths.insert(path);
562​
563​ // create job
564​ KIO::FileSystemFreeSpaceJob *job = KIO::fileSystemFreeSpace(QUrl::fromLocalFile(path));
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…
565​
566​ // delete later timer
567​ connect(job, &KIO::FileSystemFreeSpaceJob::result, timer, &QTimer::deleteLater);
568​
569​ // collect and process info
570​ connect(job, &KIO::FileSystemFreeSpaceJob::result, this,
571​ [this, timer, path, udi](KIO::Job *job, KIO::filesize_t size, KIO::filesize_t available) {
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, ... ```…
572​ timer->stop();
573​
574​ if (!job->error()) {
anthonyfieroniUnsubmitted
Done

Capture timer too

anthonyfieroni: Capture timer too
575​ setData(udi, I18N_NOOP("Free Space"), QVariant(available));
576​ setData(udi, I18N_NOOP("Free Space Text"), KFormat().formatByteSize(available));
anthonyfieroniUnsubmitted
Done

You can omit declaration of map, just

timer->stop();
anthonyfieroni: You can omit declaration of map, just ``` timer->stop(); ```
577​ setData(udi, I18N_NOOP("Size"), QVariant(size));
578​ }
579​
580​ m_paths.remove(path);
581​ });
582​
583​ // start timer: after 15 seconds we will get an error
584​ timer->start(15000);
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…
556 } 585​ }
557 586​
558 return false; 587​ return false;
559} 588​}
anthonyfieroniUnsubmitted
Done
job->start();

as well.

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

Job starts without it

McPain: Job starts without it
560 589​
561bool SolidDeviceEngine::updateHardDiskTemperature(const QString &udi) 590​bool SolidDeviceEngine::updateHardDiskTemperature(const QString &udi)
562{ 591​{
563 Solid::Device device = m_devicemap.value(udi); 592​ Solid::Device device = m_devicemap.value(udi);
564 Solid::Block *block = device.as<Solid::Block>(); 593​ Solid::Block *block = device.as<Solid::Block>();
565 if (!block) { 594​ if (!block) {
566 return false; 595​ return false;
567 } 596​ }
568 597​
569 if (!m_temperature) { 598​ if (!m_temperature) {
570 m_temperature = new HddTemp(this); 599​ m_temperature = new HddTemp(this);
571 } 600​ }
anthonyfieroniUnsubmitted
Done

Use new connect syntax .

anthonyfieroni: Use new connect syntax .
572 601​
573 if (m_temperature->sources().contains(block->device())) { 602​ if (m_temperature->sources().contains(block->device())) {
574 setData(udi, I18N_NOOP("Temperature"), m_temperature->data(block->device(), HddTemp::Temperature)); 603​ setData(udi, I18N_NOOP("Temperature"), m_temperature->data(block->device(), HddTemp::Temperature));
575 setData(udi, I18N_NOOP("Temperature Unit"), m_temperature->data(block->device(), HddTemp::Unit)); 604​ setData(udi, I18N_NOOP("Temperature Unit"), m_temperature->data(block->device(), HddTemp::Unit));
576 return true; 605​ return true;
577 } 606​ }
578 607​
579 return false; 608​ return false;
580} 609​}
581 610​
582bool SolidDeviceEngine::updateEmblems(const QString &udi) 611​bool SolidDeviceEngine::updateEmblems(const QString &udi)
583{ 612​{
584 Solid::Device device = m_devicemap.value(udi); 613​ Solid::Device device = m_devicemap.value(udi);
585 614​
586 setData(udi, I18N_NOOP("Emblems"), device.emblems() ); 615​ setData(udi, I18N_NOOP("Emblems"), device.emblems() );
587 return true; 616​ return true;
588} 617​}
589 618​
590bool SolidDeviceEngine::forceUpdateAccessibility(const QString &udi) 619​bool SolidDeviceEngine::forceUpdateAccessibility(const QString &udi)
591{ 620​{
592 Solid::Device device = m_devicemap.value(udi); 621​ Solid::Device device = m_devicemap.value(udi);
593 if (!device.isValid()) { 622​ if (!device.isValid()) {
anthonyfieroniUnsubmitted
Done

It does not need, you connect finished to deleteLater.

anthonyfieroni: It does not need, you connect finished to deleteLater.
594 return false; 623​ return false;
595 } 624​ }
596 625​
597 updateEmblems(udi); 626​ updateEmblems(udi);
598 Solid::StorageAccess *storageaccess = device.as<Solid::StorageAccess>(); 627​ Solid::StorageAccess *storageaccess = device.as<Solid::StorageAccess>();
599 if (storageaccess) { 628​ if (storageaccess) {
600 setData(udi, I18N_NOOP("Accessible"), storageaccess->isAccessible()); 629​ setData(udi, I18N_NOOP("Accessible"), storageaccess->isAccessible());
601 } 630​ }
▲ Show 20 Lines • Show All 81 Lines • Show Last 20 Lines