D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

meven added a comment. In D14895#683521 , @alanjds wrote: > In D14895#683516 , @meven wrote: > > > Feel free to send a MR, and include more context please. > > > Sorry my ignorance. What "MR"

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

alanjds added a comment. In D14895#683516 , @meven wrote: > Feel free to send a MR, and include more context please. Sorry my ignorance. What "MR" stands for, in this context? REPOSITORY R120 Plasma Workspace REVISION DETAIL

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

alanjds added a comment. I like the idea of making docker imune. May be improved. INLINE COMMENTS > meven wrote in soliddeviceengine.cpp:583-584 > Better approach would be to make this check ignore docker overlay fs. That would fix my issue, sure. Idk if targeting "docker" is better than

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

meven added a comment. Feel free to send a MR, and include more context please. INLINE COMMENTS > alanjds wrote in soliddeviceengine.cpp:583-584 > Can we have this number configurable somehow, even if only before startup. > > It's been annoying to have notifications when bringing lots of

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

alanjds added a comment. As described on https://bugs.kde.org/show_bug.cgi?id=401088, this notification is triggering when it should not: during short bursts of high CPU load, instead of when a real issue is ongoing. Then the //handling// of the high amount of notifications is extending

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

This revision was automatically updated to reflect the committed changes. Closed by commit R120:e1c19ce4daf9: Plasmashell freezes when trying to get free space info from mounted remote… (authored by McPain, committed by ngraham). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added a subscriber: ngraham. McPain added a comment. In D14895#320987 , @ngraham wrote: > +1 for the concept. No opinion about the notification, but I think it's fine. Removing myself as a reviewer since I don't feel qualified to offer a

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

ngraham resigned from this revision. ngraham added a comment. +1 for the concept. No opinion about the notification, but I think it's fine. Removing myself as a reviewer since I don't feel qualified to offer a code review. But sounds like you've got a shipit! You need someone to land it for

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

davidedmundson resigned from this revision. davidedmundson added a comment. This revision is now accepted and ready to land. I wouldn't have kept the notification myself, but its fine. Ship it. REVISION DETAIL https://phabricator.kde.org/D14895 To: McPain, broulik, ngraham Cc:

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

ngraham added a comment. Does this look good now @davidedmundson? REVISION DETAIL https://phabricator.kde.org/D14895 To: McPain, broulik, ngraham, davidedmundson Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts,

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added a comment. Just in case, I don't have commit access REVISION DETAIL https://phabricator.kde.org/D14895 To: McPain, broulik, ngraham, davidedmundson Cc: anthonyfieroni, davidedmundson, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts,

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 40973. McPain marked 12 inline comments as done. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=40923=40973 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES dataengines/soliddevice/CMakeLists.txt

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

broulik added a comment. I'm not a huge fan of having that notification but if you can assure it doesn't spam the user by showing up repeatedly if a mount is blocked indefinitely, it's fine I guess INLINE COMMENTS > soliddeviceengine.cpp:29 > #include > #include > #include Unused >

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 40923. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=39934=40923 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES dataengines/soliddevice/CMakeLists.txt dataengines/soliddevice/soliddeviceengine.cpp

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added inline comments. INLINE COMMENTS > davidedmundson wrote in soliddeviceengine.cpp:555 > I think you can just move the notification to > > connect(job, ::FileSystemFreeSpaceJob::result, ...) { > > if (job->error() == ERR_SERVER_TIMEOUT) { > > > > } > } > > but I haven't

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

anthonyfieroni added inline comments. INLINE COMMENTS > McPain wrote in soliddeviceengine.cpp:560 > Any ideas how to do it correctly? It has a default parameters just call event with 3 parameters not 5. REVISION DETAIL https://phabricator.kde.org/D14895 To: McPain, broulik, ngraham,

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added inline comments. INLINE COMMENTS > davidedmundson wrote in soliddeviceengine.cpp:560 > 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. Any ideas how to do it

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

davidedmundson added inline comments. INLINE COMMENTS > McPain wrote in soliddeviceengine.cpp:555 > There are solutions to notify user about stuck FS without that timer, right? I think you can just move the notification to connect(job, ::FileSystemFreeSpaceJob::result, ...) { if (job->error()

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added inline comments. INLINE COMMENTS > davidedmundson wrote in soliddeviceengine.cpp:555 > 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)

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

davidedmundson added a comment. Much nicer! I'm definitely happy merging something like this. INLINE COMMENTS > soliddeviceengine.cpp:555 > +if (!m_paths.contains(path)) { > +QTimer *timer = new QTimer(this); > +timer->setSingleShot(true); not sure you need this timer

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 39934. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=39933=39934 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES dataengines/soliddevice/CMakeLists.txt dataengines/soliddevice/soliddeviceengine.cpp

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 39933. McPain marked 4 inline comments as done. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=39929=39933 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES dataengines/soliddevice/CMakeLists.txt

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added inline comments. INLINE COMMENTS > anthonyfieroni wrote in soliddeviceengine.cpp:564 > You can remove it, see below. 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" > anthonyfieroni wrote

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

anthonyfieroni added inline comments. INLINE COMMENTS > soliddeviceengine.cpp:588 > +// start timer: after 15 seconds we will get an error > +timer->start(15000); > } job->start(); as well. REVISION DETAIL https://phabricator.kde.org/D14895 To: McPain, broulik,

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

anthonyfieroni added inline comments. INLINE COMMENTS > soliddeviceengine.cpp:564 > + > +m_timers[path] = timer; > + You can remove it, see below. > soliddeviceengine.cpp:574 > +connect(job, ::FileSystemFreeSpaceJob::result, > +[this, path, udi](KIO::Job *job,

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 39929. McPain added a comment. Used KIO::FileSystemFreeSpaceJob CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=39901=39929 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES dataengines/soliddevice/CMakeLists.txt

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

broulik added a comment. It is just a question. I think since you call `setData` when the job finishes anyway, it should be fine but I don't really know the architecture of that stuff either, so it might be fine to just trigger a free space job in `updateSourceEvent` REVISION DETAIL

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added a comment. In D14895#310633 , @broulik wrote: > Might not be an issue as you call `setData` anyway but just something I want you to keep in mind for testing. something like setData(udi, I18N_NOOP("Free Space"),

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added a comment. In D14895#310633 , @broulik wrote: > Might not be an issue as you call `setData` anyway but just something I want you to keep in mind for testing. I REVISION DETAIL https://phabricator.kde.org/D14895 To:

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added a comment. In D14895#310633 , @broulik wrote: > Also beware that `bool SolidDeviceEngine::updateSourceEvent` is currently supposed to return `true` Who will use that `true`? I didn't found any places where that value is

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

broulik added a comment. Yes, please use `KIO::FileSystemFreeSpaceJob` which is a lot simpler than all of this manually added threading code. Also beware that `bool SolidDeviceEngine::updateSourceEvent` is currently supposed to return `true` for when any of the sources changed which

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. But the KIO stuff is already there. > We can always do it later That's never how it works. REVISION DETAIL https://phabricator.kde.org/D14895 To:

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain added a comment. In D14895#310535 , @davidedmundson wrote: > Thanks for looking into this. > > Whilst this looks fine, we have a more standard abstraction layer for async IO via KIO. > There's a FileSystemFreeSpaceJob which

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 39901. McPain marked 3 inline comments as done. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=39897=39901 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES dataengines/soliddevice/CMakeLists.txt

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

anthonyfieroni added inline comments. INLINE COMMENTS > soliddeviceengine.cpp:67 > +KDiskFreeSpaceInfo info = KDiskFreeSpaceInfo::freeSpaceInfo(m_path); > +emit infoObtained(m_path, m_udi, info.isValid(), info.available(), > info.size()); > +} Don't you forget to emit finished? >

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

davidedmundson added a comment. Thanks for looking into this. Whilst this looks fine, we have a more standard abstraction layer for async IO via KIO. There's a FileSystemFreeSpaceJob which should work here. REPOSITORY R120 Plasma Workspace REVISION DETAIL

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain updated this revision to Diff 39897. McPain added a comment. Removed useless debug string REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14895?vs=39896=39897 REVISION DETAIL https://phabricator.kde.org/D14895 AFFECTED FILES

D14895: Plasmashell freezes when trying to get free space info from mounted remote filesystem after losing connection to it

McPain created this revision. McPain added reviewers: broulik, ngraham. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. McPain requested review of this revision. REVISION SUMMARY BUG: 397537 Earlier plasmashell assumed that you'll get free space info immediately