Re: LibKF6Breeze Icons and KIconLoader

[Fabian Vogt]

Hi,

Am Freitag, 24. Mai 2024, 00:13:11 MESZ schrieb Bernhard Rosenkränzer:
> On Thursday, May 23, 2024 22:20 CEST, Jonathan Riddell  
> wrote:
> 
> > Has anyone had issues with Breeze loading using Frameworks 6.2 or
> > master building the libKF6BreezeIcons.so.6 library in breeze-icons.
> 
> Yes, there was an oddity here too. When building the package with LTO enabled 
> as we usually do, it fails with
> 
> ld.lld: error: src/qrc_breeze-icons.o: Invalid bitcode signature
> 
> while linking libKF6BreezeIcons.so.6.2.0
> 
> But I didn't get around to debugging it properly yet, and disabling LTO fixed 
> at least building it.
> This smells more like an rcc or toolchain bug though (and 
> qrc_breeze-iconstmp.cpp.o looks ok), so it may not be related to the problem 
> you're seeing.

That sounds like it uses qt_add_big_resources which is completely broken by
design and must not be used. It messes with object file data directly in a way
which just breaks everything: https://bugreports.qt.io/browse/QTBUG-73834

Cheers,
Fabian

> ttyl
> bero
> 
> 

Re: LibKF6Breeze Icons and KIconLoader

[Fabian Vogt]

Hi,

Am Freitag, 24. Mai 2024, 00:13:11 MESZ schrieb Bernhard Rosenkränzer:
> On Thursday, May 23, 2024 22:20 CEST, Jonathan Riddell  
> wrote:
> 
> > Has anyone had issues with Breeze loading using Frameworks 6.2 or
> > master building the libKF6BreezeIcons.so.6 library in breeze-icons.
> 
> Yes, there was an oddity here too. When building the package with LTO enabled 
> as we usually do, it fails with
> 
> ld.lld: error: src/qrc_breeze-icons.o: Invalid bitcode signature
> 
> while linking libKF6BreezeIcons.so.6.2.0
> 
> But I didn't get around to debugging it properly yet, and disabling LTO fixed 
> at least building it.
> This smells more like an rcc or toolchain bug though (and 
> qrc_breeze-iconstmp.cpp.o looks ok), so it may not be related to the problem 
> you're seeing.

That sounds like it uses qt_add_big_resources which is completely broken by
design and must not be used. It messes with object file data directly in a way
which just breaks everything: https://bugreports.qt.io/browse/QTBUG-73834

Cheers,
Fabian

> ttyl
> bero
> 
> 

Re: KIO workers and the Qt5->Qt6 transition

[Fabian Vogt]

Hi,

Am Dienstag, 12. Dezember 2023, 15:57:49 CET schrieb Nicolas Fella:
> Hi,
> 
> this is a variant of the plugin problem outlined in
> https://mail.kde.org/pipermail/kde-devel/2023-December/002234.html
> 
> TL;DR Qt6 can't load Qt5-based plugins and vice versa
> 
> This also affects KIO workers.
> 
> We have a number of sources of KIO worker plugins:
> 
> - KIO itself provides file, http, help, ftp, remote, and trash. Since
> KIO is available from KF5 and KF6 we get builds of those for both Qt
> versions. No problem here.
> 
> - kio-extras provides additional workers: sftp, mtp, smb, ... kio-extras
> is available for KF5 and KF6, so no problem here.
> 
> - various kio-* modules, like kio-gdrive, kio-stash, kio-gopher. These
> should be built for both Qts, which should generally be possible, modulo
> some common files to be deconflicted
> 
> - "Applications" that ship a KIO worker as part of their offering.
> Examples that come to my mind are KDE Connect that ships a kdeconnect
> worker and bluedevil that ships the bluetooth and obexftp workers. These
> project are generally not designed to be coinstallable. i.e. if I have
> Qt6-based KDE Connect I can't use the kdeconnect worker from a KF5 app

There's another problem but also another solution: kio-fuse. With yesterday's
5.1.0 release it can be built against KF6 as well, but it's unversioned and has
stable API, so KF5 kio-fuse can be used with KF6 applications and vice versa.

It can be problem because kio-fuse is like an application in this regard, it's
tied to a specific major version of KIO workers. If a protocol is not
available, KIO falls back to plain kioexec.

It can be a solution because through FUSE, KF5 applications can use KF6 workers
and vice versa. There's no plumbing set up for this though and as there can
currently only be one kio-fuse, only one direction is available. If there are
KF5 only workers and KF6 only workers, that can't be fully bridged currently.

> The last case is the one giving me headaches. Possible options would be
> 
> - Ignore the problem and hope nobody misses some workers in some
> applications
> 
> - Make sure all (relevant) workers are available for both Qts, adding
> some complexity to the build, release, and packaging process

Currently it's effectively a mixture of those two options. Make most workers
available for both and hope that nobody misses the rest.

> - Since KIO workers run out of process it should be technically feasible
> to come up with a way to run KF5-based worker plugins from a KF6-based
> app and vice versa. I have not tried to implement that though, and it
> would require some non-trivial surgery on KIO internals

It shouldn't actually be that hard to achieve, but it's probably not a good
option for other reasons:

1. The QDataStream based protocol needs to be kept compatible. Even subtle
   differences in behaviour can cause major issues.
2. To get KF5 based applications to use KF6 workers, which is arguably the more
   impactful direction, it has to be implemented in KF5's KIO as well. That
   means also backwards-compat of the KF6 KIO worker protocol with KF5 KIO,
   which restricts KF6.

Cheers,
Fabian

Re: The KDE Patchset Collection has been rebased on top of Qt 5.15.3

[Fabian Vogt]

Moin,

Am Freitag, 4. M�rz 2022, 01:35:14 CET schrieb Albert Astals Cid:
> The KDE Patchset Collection has been rebased on top of Qt 5.15.3
> 
> Some patches have been droped, some are still needed since we carry patches 
> newer than 1 year old.
> 
> Since this are rebases (to clearly show which patches are "ours", i.e. those 
> on top of the v5.15.3-lts-lgpl tag)
> the kde/5.15 branches have been force-pushed to, don't get scared when 
> fetching tells you that a force-update happened.

With the rebase, the connection between the 5.15.2 and 5.15.3 states has been
lost, which broke all branches (and thus MRs) based on that, and the
non-linearity of the history makes it hard to get an accurate list of changes
(added, removed and changed commits). GC might eventually also break links to
particular commits on the old branch.

Would it be possible to just cherry-pick the patches from upstream's 5.15 which
were missing from kde/5.15 instead? I imagine that wouldn't be too hard, and
grant a better overview.

If not, could you please publish a full list of changes caused by the rebase of
affected repositories?

Thanks,
Fabian

> Cheers,
>   Albert

Re: Critical Denial of Service bugs in Discover

[Fabian Vogt]

Moin,

Am Sonntag, 6. Februar 2022, 21:54:13 CET schrieb Fabian Vogt:
> Am Sonntag, 6. Februar 2022, 19:27:11 CET schrieb Ben Cooksley:
> > On Sun, Feb 6, 2022 at 1:07 PM Fabian Vogt  wrote:
> > > The first URL is used by kfontinst.knsrc from plasma-workspace:
> > > ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml
> > >
> > > The second URL is used by multiple knsrc files in my VM:
> > > aurorae.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > comic.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > kwineffect.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > kwinscripts.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > kwinswitcher.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > > wallpaperplugin.knsrc:ProvidersUrl=
> > > https://download.kde.org/ocs/providers.xml
> > 
> > This makes me incredibly sad. We had a push to eliminate all usage of the
> > legacy download.kde.org endpoint many years ago...
> > I have now resolved the majority of these - if distributions could please
> > pick up those patches that would be appreciated.
> > 
> > Please note that I have now terminated the support on the server that was
> > making these legacy endpoints work, so those patches are necessary to
> > restore functionality.
> ...
> It's also possible that the requests aren't actually caused by Discover at 
> all,
> but just something which imitates it in a DDoS attack. In that case we 
> couldn't
> do anything on the client-side anyway. I don't think this is very likely, but
> until the issue was reproduced with disover it's a possibility.

I think I have a plausible explanation for what could've caused this.
While testing a MR for the notifier, I noticed odd behaviour: It always ran
plasma-discover-update twice!
https://invent.kde.org/plasma/discover/-/merge_requests/254#note_394584

The reason for that is that after the update process finishes, the notifier
realizes that it's idle again and if updates are available, it will immediately
trigger another update after the 15min idle time. Now here's the catch: If the
system has already been idle for >=15min (which is very likely at that point),
the idle timeout will immediately fire! This process repeats unlimited and
without delay, until the system is no longer idle or there aren't updates
available anymore. Here I have plasma-discover-update running approx. every
second, which amounts to ~4 req/s to download.kde.org.

This is mostly mitigated by the introduction of the 3h delay between updates
by d607e0c6f9, but not entirely. The check is only effective after the second
iteration, which is what I observed in my testing. (One of the commits in my MR
should address that as well.)

One of the conditions for running into this bug is that after the automatic
updater ran, there still have to be updates available to trigger the next run.
Initially I thought that this can mostly happen if updates fail to download or
install, this is unfortunately not true. The notifier by default counts all
available updates, but the updater only installs offline updates. So if there
is even a single non-offline update available, the loop continues.

So this probably affected a lot of users who enabled automatic installation of
updates :-/

Cheers,
Fabian

Re: Critical Denial of Service bugs in Discover

[Fabian Vogt]

Moin,

Am Sonntag, 6. Februar 2022, 19:27:11 CET schrieb Ben Cooksley:
> On Sun, Feb 6, 2022 at 1:07 PM Fabian Vogt  wrote:
> > The first URL is used by kfontinst.knsrc from plasma-workspace:
> > ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml
> >
> > The second URL is used by multiple knsrc files in my VM:
> > aurorae.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > comic.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > kwineffect.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > kwinscripts.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > kwinswitcher.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
> > wallpaperplugin.knsrc:ProvidersUrl=
> > https://download.kde.org/ocs/providers.xml
> 
> This makes me incredibly sad. We had a push to eliminate all usage of the
> legacy download.kde.org endpoint many years ago...
> I have now resolved the majority of these - if distributions could please
> pick up those patches that would be appreciated.
> 
> Please note that I have now terminated the support on the server that was
> making these legacy endpoints work, so those patches are necessary to
> restore functionality.

Does this decrease the server load noticably?

On IRC you wrote that the primary offender is
"KNewStuff/5.86.0-plasma-discover-update/", can you make the endpoints return
an error for the top entries only? Then we'll get bug reports only for the
likely cause(s) of the high traffic instead of everyone.

What might help is to have a lightweight proxy in front of Apache to handle
those paths in a way that doesn't stress the system much. That should probably
be investigated independently of the client side, as this is entirely under our
control and has immediate effects.

It's also possible that the requests aren't actually caused by Discover at all,
but just something which imitates it in a DDoS attack. In that case we couldn't
do anything on the client-side anyway. I don't think this is very likely, but
until the issue was reproduced with disover it's a possibility.

> > > Given that Sysadmin has raised issues with this component and it's
> > > behaviour in the past, it appears that issues regarding the behaviour of
> > > the OCS componentry within Discover remain unresolved.
> > >
> > > Due to the level of distress this is causing our systems, I am therefore
> > > left with no other option other than to direct the Plasma Discover
> > > developers to create and release without delay patches for all versions
> > in
> > > support, as well as for all those currently present in any actively
> > > maintained distributions, that disable all OCS functionality in the
> > > Discover updater. Distributions are requested to treat these patches as
> > > security patches and to distribute them to users without delay.
> >
> > Emergency workarounds for distributions might be to either not ship the KNS
> > backend by not building kns-backend.so or deleting it afterwards, or
> > disabling
> > the discover notifier
> > (/etc/xdg/autostart/org.kde.discover.notifier.desktop)
> > completely.
> 
> I have now committed patches to Discover going back to Plasma/5.18 which
> disable the build of the KNS backend.
> If distributions could please pick them up and distribute them as I
> previously indicated that would be much appreciated.
> 
> https://invent.kde.org/plasma/discover/-/commit/f66df3531670592960167f5060feeed6d6c792be

IMO we need a more targeted approach there. The main offenders aren't running
the latest version, so likely won't get those updates that quickly either.
If we have more data and can pinpoint it a bit better that would at least help
to speed patch delivery up.

> Please note that I intend to investigate whether it is possible to serve
> corrupted files from the server side that cause Discover to crash to help
> alleviate the load being created by those clients.

Sounds like a good way to DoS bugzilla instead and cause bad PR, both up- and
downstream. On top of that, it's possible that a resulting crashloop causes an
even higher frequency of requests.

Cheers,
Fabian

> Current load being generated by this is:
> 
> 789 requests/sec - 6.4 MB/second - 8.3 kB/request - 1.70113
> ms/request
> 217 requests currently being processed, 183 idle workers
> 
> > Cheers,
> > Fabian
> >
> Thanks,
> Ben

Re: Critical Denial of Service bugs in Discover

[Fabian Vogt]

Hi,

Am Samstag, 5. Februar 2022, 22:16:28 CET schrieb Ben Cooksley:
> Hi all,
> 
> Over the past week or so Sysadmin has been dealing with an extremely high
> volume of traffic directed towards both download.kde.org and
> distribute.kde.org.
> 
> This traffic volume is curious in so far that it is directed at two paths
> specifically:
> - distribute.kde.org/khotnewstuff/fonts-providers.xml
> - download.kde.org/ocs/providers.xml
> 
> The first path is an "internal only" host which we were redirecting a
> legacy path to prior to the resource being relocated to cdn.kde.org. The
> second path has been legacy for numerous years now (more than 5) and is
> replaced by autoconfig.kde.org.
> It is of extreme concern that these paths are still in use - especially the
> ocs/providers.xml one.
> 
>...
> 
> This indicates that the bug lies solely within Plasma's Discover component
> - more precisely it's updater.
> 
> Examining the origin of these requests has indicated that some clients are
> making requests to these paths well in excess of several times a minute
> with a number of IP addresses appearing more 60 times in a 1 minute sized
> sample window.

FWICT, this is caused by plasma-discover-update, which is triggered by the
DiscoverNotifier service if automatic updates are enabled in kcm_updates,
updates are available and the system idle for >=15min.

// If the system is untouched for 1 hour, trigger the unattened update
using namespace std::chrono_literals;
KIdleTime::instance()->addIdleTimeout(int(std::chrono::milliseconds(15min).count()));

(I wonder whether there's a bug about calling addIdleTimeout more than once.
It will then invoke triggerUpdate multiple times after 15min of idle.)

The Discover KNS backend creates instances for all available knsrc files,
which on construction call KNSReviews::setProviderUrl with the URL defined in
those files, triggering the requests.

The first URL is used by kfontinst.knsrc from plasma-workspace:
ProvidersUrl=https://distribute.kde.org/khotnewstuff/fonts-providers.xml

The second URL is used by multiple knsrc files in my VM:
aurorae.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
comic.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
kwineffect.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
kwinscripts.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
kwinswitcher.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml
wallpaperplugin.knsrc:ProvidersUrl=https://download.kde.org/ocs/providers.xml

> Given that Sysadmin has raised issues with this component and it's
> behaviour in the past, it appears that issues regarding the behaviour of
> the OCS componentry within Discover remain unresolved.
> 
> Due to the level of distress this is causing our systems, I am therefore
> left with no other option other than to direct the Plasma Discover
> developers to create and release without delay patches for all versions in
> support, as well as for all those currently present in any actively
> maintained distributions, that disable all OCS functionality in the
> Discover updater. Distributions are requested to treat these patches as
> security patches and to distribute them to users without delay.

Emergency workarounds for distributions might be to either not ship the KNS
backend by not building kns-backend.so or deleting it afterwards, or disabling
the discover notifier (/etc/xdg/autostart/org.kde.discover.notifier.desktop)
completely.

Cheers,
Fabian

Re: Can we get tags and tarballs for the KDE Qt patch collection

[Fabian Vogt]

Hi,

Am Dienstag, 8. Juni 2021, 12:51:35 CEST schrieb Neal Gompa:
> On Mon, Jun 7, 2021 at 4:52 PM Albert Astals Cid  wrote:
> >
> > El dilluns, 7 de juny de 2021, a les 20:46:25 (CEST), Nate Graham va 
> > escriure:
> > > Hello folks,
> > >
> > > The Fedora packagers were mentioning to me today that it would be a lot
> > > easier for them to ship Qt with our patch collection if we made tags and
> > > tarballs. Is this something we could look into doing?
> >
> > We explicitly do not want to make releases
> >   https://community.kde.org/Qt5PatchCollection#Will_there_be_releases.3F
> >
> > Making a release means having to use of a version number, and any version 
> > number we use will be wrong.
> >
> > Don't think this as a product, think of it as a central place where patches 
> > are collected.
> >
> > If they want a tarball because using git is a problem, they can always use 
> > https://invent.kde.org/qt/qt/qtbase/-/archive/kde/5.15/qtbase-kde-5.15.tar.bz2
> >  ?
> >
> 
> You *already* are using version numbers and bumped it to 5.15.3:
> https://blog.neon.kde.org/2021/06/04/kde-neons-qt-is-now-built-from-kdes-git-branches/

No, that was done in upstream Qt's 5.15 branch when 5.15.2 got prepared.

New version numbers and releases would be necessary if new features get
introduced by backported patches, but FWICT that is explicitly against the
goal.

See https://community.kde.org/Qt5PatchCollection.

Cheers,
Fabian

> This is unreasonable if you're going to make us need fixes from there.
> I'd rather we didn't pretend this is something other than what it is:
> a community maintained uplift of Qt 5.15 while Plasma works to move to
> Qt 6.
> 
> Also, that URL is unstable, you'd get different things each time you'd
> fetch from it based on the HEAD of that branch.
> 
> --
> 真実はいつも一つ！/ Always, there's only one truth!

Re: Requiring Qt 5.15 for KDE Frameworks 5?

[Fabian Vogt]

Moin,

Am Samstag, 27. M?rz 2021, 14:11:38 CET schrieb David Faure:
> On samedi 27 mars 2021 12:51:37 CET Kai Uwe Broulik wrote:
> > Hi everyone,
> > 
> > during the ongoing KDE Frameworks 6 sprint we were just contemplating
> > whether we can bump the required Qt dependency for Frameworks 5 to Qt 5.15.
> > 
> > Reason being that Qt 5.15 includes a set of porting aids and
> > forward-compatibility with Qt 6, such as version-less "Qt" rather than
> > "Qt5" CMake target, various QStringView-related features, and so on.
> > 
> > We would like to start working on KDE Frameworks 6 on Qt 6 but still
> > keep Frameworks 5 supported with as little overhead as possible, i.e.
> > not having a gazillion ifdefs or even dedicated branch, which we would
> > likely need, should we have to continue supporting Qt 5.14 in the process.
> > 
> > Are there any objections or concerns or potential release schedule
> > conflicts if we did that?
> 
> While at it, can we also get your feedback on
> * Requiring C++17

Which for GCC means at least g++ 9 in practice due to std::filesystem?

> * Requiring CMake >= 3.16
> 
> Obviously this only matters for distributions that update KF5 every month.

AFAICT it will affect all distros at some point in the future, but those which
update everything in a rolling manner should be fine anyway.

openSUSE Tumbleweed is one of those and so it's not an issue.

On Leap 15.x, the default compiler is GCC 7, so we'll have to switch to GCC9+
for that. That's fortunately fairly simple to do, so not really an issue
either.

Cheers,
Fabian

Re: Need xcb/xkb help for severe kglobalaccel_x11 issue

[Fabian Vogt]

Moin,

Am Samstag, 6. Februar 2021, 10:08:43 CET schrieb David Faure:
> Problem mostly fixed by 
> https://invent.kde.org/plasma/plasma-desktop/-/merge_requests/339
> but still seeing 15 notifications instead of 1 (already better than 145...).
> 
> Indeed a single call to
> /usr/bin/setxkbmap -layout us,fr -option -option compose:caps
> sends 12 XCB_XKB_NEW_KEYBOARD_NOTIFY events,
> all with changed & XCB_XKB_NKN_DETAIL_KEYCODES, which makes kglobalaccel
> ungrab+regrab all keys.

Here setxkbmap doesn't cause any XCB_XKB_NEW_KEYBOARD_NOTIFY events, only
XCB_XKB_MAP_NOTIFY. I tried this with Xwayland though, maybe that behaves
differently...

> Should I add another compression timer in kglobalaccel, or do you think this 
> is
> fixable in setxkbmap?

It's probably fixable somewhere in either setxkbmap/xkb or the X server, but a
compression timer would work even with older versions and also for other
causes like xmodmap, which can be much worse than just 15 events.

Cheers,
Fabian

Re: Need xcb/xkb help for severe kglobalaccel_x11 issue

[Fabian Vogt]

Hi,

Am Samstag, 30. Januar 2021, 18:32:32 CET schrieb David Faure:
> For years, I've noticed that when resuming a laptop from sleep, kglobalaccel 
> and X11 
> use 100% CPU for a few minutes, making everything crawl for a while.
> 
> I finally debugged why: kglobalaccel grabs and ungrabs all global shortcuts 
> many many times in a row.
> 
> KGlobalAccelImpl::nativeEventFilter event->response_type= 85 xkbEvent= 1 
> calling x11MappingNotify()
> KGlobalAccelImpl::x11MappingNotify Got XMappingNotify event
> KGlobalAccelInterface::ungrabKeys 
> KGlobalAccelInterface::grabKeys 
> KGlobalAccelImpl::nativeEventFilter event->response_type= 85 xkbEvent= 1 
> calling x11MappingNotify()
> KGlobalAccelImpl::x11MappingNotify Got XMappingNotify event
> KGlobalAccelInterface::ungrabKeys 
> KGlobalAccelInterface::grabKeys 
> and so on, and so on...
> 
> The reason why x11MappingNotify() does ungrabKeys+grabKeys is apparently 
> "Maybe the X modifier map has been changed."
> ... which is not the case at all...
>
> What's an XCB_XKB_MAP_NOTIFY anyway?  
> http://manpages.ubuntu.com/manpages/xenial/en/man3/xcb_xkb_map_notify_event_t.3.html
> is very much incomplete...

It's just the XCB equivalent of XkbMapNotify. If you search for the latter, you
should get more hits.

> Is it event really such an event that we're getting?
> The code says
> 
> } else if (m_xkb_first_event && responseType == m_xkb_first_event) {
> const uint8_t xkbEvent = event->pad0;
> switch (xkbEvent) {
> case XCB_XKB_MAP_NOTIFY:
> qDebug() << "event->response_type=" << event->response_type 
> << "xkbEvent=" << xkbEvent << "calling x11MappingNotify()";
> x11MappingNotify();
> break;
> 
> What sense does it make that this code is checking pad0, which looks like 
> some padding member? To avoid a downcast to a more specific event type maybe?

Yep. That's what an earlier revision did, but I was recommended to use pad0
instead: 
https://phabricator.kde.org/D16434?vs=44241=44247=ignore-most#toc
It's equivalent to xkbType, which indicates the exact type of this xkb event.

> I'm not sure:
> * if we're really getting a stream of XCB_XKB_MAP_NOTIFY events or if this 
> code misunderstands that

I don't think that the code would misunderstand that. AFAICT the check is
strict enough to not catch other events, and if it's wrong instead it wouldn't
catch the correct events either. If this was the cause of the issue, I would
expect misbehaviour outside of the post-resume state. I can't rule it out
though.

> * if so, why is X11 sending such a high number of those

I suspect that something else is triggering keyboard layout reloads or similar
in a loop. The keyboard kded module listens for new keyboards and mouse events
and configures them, which is likely to be called on resume when devices get
reenumerated. You could try to disable that before suspend/resume.

If I run "xmodmap .Xmodmap" here with "xev" open, I get quite a high count of
mapping change events, presumably for every assignment in the file. If the kded
calls xmodmap with many assignments, that would be enough to explain the issue.
Maybe xmodmap could be optimized to upload everything at once, like setxkbmap.

I actually had the issue that calling xmodmap here basically froze the whole
session for a while, which was probably caused by that behaviour.

> * why the code reacts the same to XCB_XKB_MAP_NOTIFY and to 
> XCB_MAPPING_NOTIFY, isn't one enough?

When an X client indicates that it uses Xkb instead of the X core protocol,
then it will only receive XKB events. In the case of kglobalaccel, the decision
is made by Qt (which is why it broke at some point). The XCB_MAPPING_NOTIFY
case is probably unused now, which could be ensured by just doing Xkb stuff
explicitly on initialization.

> Without outside help I guess I would just compress those events using a 
> timer, but that would be a "fix without really understanding the code", never 
> good.

Agreed. If those events are caused by something we can't fix, then this might
turn out to be the best option though.

Cheers,
Fabian

> I just noticed https://phabricator.kde.org/D16434 so CC'ing Fabian :)

D29800: Fix URL being passed as argument when launching a .desktop file

[Fabian Vogt]

fvogt marked an inline comment as done.
fvogt added a comment.


  Landed to invent - hopefully correctly: 
https://invent.kde.org/frameworks/kio/commit/84e9372f4fa2636f57dc456ac2fa2be271d6a7ec

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29800

To: fvogt, dfaure, marten
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29800: Fix URL being passed as argument when launching a .desktop file

[Fabian Vogt]

fvogt closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29800

To: fvogt, dfaure, marten
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29800: Fix URL being passed as argument when launching a .desktop file

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: dfaure, marten.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  When a .desktop file is executed directly, it doesn't receive a parameter.
  BUG: 421364

TEST PLAN
  Test passes, applications open normally again.

REPOSITORY
  R241 KIO

BRANCH
  execfi

REVISION DETAIL
  https://phabricator.kde.org/D29800

AFFECTED FILES
  autotests/openurljobtest.cpp
  autotests/openurljobtest.h
  src/gui/openurljob.cpp

To: fvogt, dfaure, marten
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29503: Pixel align children of GridViewInternal

[Fabian Vogt]

fvogt added a comment.


  In D29503#665612 , @ngraham wrote:
  
  > I can't reproduce it, but I wonder if this could fix or help 
https://bugs.kde.org/show_bug.cgi?id=417488?
  
  
  Yup, that's exactly what I was seeing before this change: F8293538: 
8sMM0Gq.png  vs F8293533: 
Screenshot_20200507_153104.png 

REPOSITORY
  R296 KDeclarative

REVISION DETAIL
  https://phabricator.kde.org/D29503

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29503: Pixel align children of GridViewInternal

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R296:9725a21bcd0e: Pixel align children of GridViewInternal 
(authored by fvogt).

REPOSITORY
  R296 KDeclarative

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29503?vs=82183=82184

REVISION DETAIL
  https://phabricator.kde.org/D29503

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

To: fvogt, #frameworks, broulik, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29503: Pixel align children of GridViewInternal

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Frameworks, broulik, mart.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  The scroll bar size can be odd (for breeze it's 21), which causes leftMargin
  to be 12.5. This causes every delegate inside to be blurred.

TEST PLAN
  Monkeypatched, now kcm_style is no longer blurred.

REPOSITORY
  R296 KDeclarative

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D29503

AFFECTED FILES
  src/qmlcontrols/kcmcontrols/qml/private/GridViewInternal.qml

To: fvogt, #frameworks, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28856: Save disabling of desktop file components in kglobalshortcutsrc

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> globalshortcutsregistry.cpp:274
> +auto disabledComponents = KConfigGroup(&_config, 
> "disabledComponents").readEntry("disabled", QStringList());
>  for (const QString  : groupList)
>  {

`disabledComponents` is the group name, right? It would also be part of 
`groupList`, so it would try to load it as shortcut...

> globalshortcutsregistry.cpp:333
>  for (const QString  : lstDesktopFiles) {
> -if (_components.contains(desktopFile)) {
> +if (_components.contains(desktopFile) || 
> disabledComponents.contains(desktopFile)) {
>  continue;

Is `desktopFile` the equivalent to `component->uniqueName()`? I would assume 
no, so this check might need to be moved after the `KServiceActionComponent` 
construction

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D28856

To: davidre, davidedmundson, fvogt, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D24956: Consider desktop files with NoDisplay attribute

[Fabian Vogt]

fvogt added a comment.


  In D24956#648968 , @davidedmundson 
wrote:
  
  > > [14:12]  DavidRedondo1: my understanding is that a system might 
ship "konsole opens with control+t". The UI allows you to remove that. This 
would remove the entry in kglobalshortcutsrc, but because it's still  in the 
system defaults file as soon as you log in again it'll add it back
  >
  > [14:25]  d_ed, fvogt Apparently the runtime writes the 
hidden thing when a component is cleanedUp 
https://cgit.kde.org/kglobalaccel.git/tree/src/runtime/kserviceactioncomponent.cpp#n135
  >  [14:27]  Does that fail or something when the file is not 
writeable?
  >  [14:31]  I think it fails
  >  [14:31]  I just tested it
  >
  > if it is indeed broken...then we may as well just merge this.
  
  
  It might not be broken for imported desktop files, in which case this would 
be a noticable regression. Not tested though.

REPOSITORY
  R268 KGlobalAccel

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24956

To: meven, mart, #plasma, fvogt, apol, davidedmundson
Cc: davidedmundson, davidre, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D24956: Consider desktop files with NoDisplay attribute

[Fabian Vogt]

fvogt added a comment.


  In D24956#648905 , @davidedmundson 
wrote:
  
  > kglobalshortcutseditor.cpp
  >  needs updating to match
  >
  > I think you're right with your reasoning about NoDisplay, but we do want 
something to be able to mask system files. From the spec should we be checking 
Hidden= ?
  
  
  `Hidden=true` is equivalent to not having the file at all according to the 
spec, so it would make sense. There's also D25088 
 open.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D24956

To: meven, mart, #plasma, fvogt, apol
Cc: davidedmundson, davidre, kde-frameworks-devel, LeGast00n, cblack, michaelh, 
ngraham, bruns

D28535: [KIO-MTP] Fix null pointer dereference

[Fabian Vogt]

fvogt added a comment.


  I assume there is a reason why `MTPDevice::getDevice()` has code for handling 
this very specific case, so I wouldn't just remove it without figuring out why: 
../https://i.redd.it/hfnl7xo8yovy.gif
  
  If not, that would indeed be the best option.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Fabian Vogt]

fvogt added a comment.


  In D28535#640682 , @feverfew wrote:
  
  > In D28535#640674 , @fvogt wrote:
  >
  > > What you're suggesting is to change `MTPDevice::getDevice` to return the 
old device if reopening fails - but reopening without releasing might not work.
  >
  >
  > This seems to be a robust solution IMO, why do you suspect this might not 
work?
  
  
  Because there can only be a single libusb session per device. So you have to 
release the old one before opening again.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Fabian Vogt]

fvogt added a comment.


  In D28535#640680 , @anthonyfieroni 
wrote:
  
  > I see we don't speak in same language :)
  >  `LIBMTP_Open_Raw_Device_Uncached(_rawdevice);`
  >  returns nullptr that's normal since device is inaccessible, i mean it does 
not need to call `LIBMTP_Release_Device` using `m_mtpdevice` is safe it's not 
nullptr, it's just a disconnected device and libmtp knows that.
  
  
  Yes, and until that point everything is fine.
  Only after `m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(_rawdevice);`, 
which sets `m_mtpdevice` to nullptr it goes down the path I outlined.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Fabian Vogt]

fvogt added a comment.


  There is no such thing as an "invalid device" at that point anymore. There's 
only nullptr.
  
LIBMTP_mtpdevice_t *MTPDevice::getDevice()
{
if (!m_mtpdevice->storage) {
qCDebug(LOG_KIOD_KMTPD) << "no storage found: reopen mtpdevice";
LIBMTP_Release_Device(m_mtpdevice);
m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(_rawdevice);
}

return m_mtpdevice;
}
  
  What you're suggesting is to change `MTPDevice::getDevice` to return the old 
device if reopening fails - but reopening without releasing might not work.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Fabian Vogt]

fvogt added a comment.


  In D28535#640656 , @anthonyfieroni 
wrote:
  
  > You're right about bug report, but it can fail in any other place, just in 
particular version it happen in  `updateStorageInfo` Can we cache `getDevice` 
in m_device (in constructor) then use it everywhere. I think libmtp has guard 
against disconnected device and will not crash.
  
  
  `MTPDevice` already does that.
  
  If `getDevice()` returns nullptr, this means that `MTPDevice::getDevice()` 
returns nullptr. This can only happen if `m_mtpdevice` is nullptr, which will 
crash in `MTPDevice::~MTPDevice` sooner or later anyway.
  
  So this patch will at most just delay the crash.
  
  AFAICT `MTPDevice` is supposed to be destroyed in `KMTPd::deviceRemoved` on 
disconnection, but this is obviously racy.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28535

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R268:8e14750977c6: Fix Make it compile against last 
qt5.15 without deprecated method. QProcess… (authored by fvogt).

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27863?vs=77022=77026

REVISION DETAIL
  https://phabricator.kde.org/D27863

AFFECTED FILES
  src/runtime/kserviceactioncomponent.cpp

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt marked an inline comment as done.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D27863

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt edited the test plan for this revision.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D27863

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt updated this revision to Diff 77022.
fvogt added a comment.


  Do it differently, just like it's done below

REPOSITORY
  R268 KGlobalAccel

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27863?vs=77009=77022

BRANCH
  somefix

REVISION DETAIL
  https://phabricator.kde.org/D27863

AFFECTED FILES
  src/runtime/kserviceactioncomponent.cpp

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Fix "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt retitled this revision from "Revert "Make it compile against last qt5.15 
without deprecated method. QProcess::execute(QString) is deprecated"" to "Fix 
"Make it compile against last qt5.15 without deprecated method. 
QProcess::execute(QString) is deprecated"".

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D27863

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt added a comment.


  In D27863#622655 , @mlaurent wrote:
  
  > if splitting is already done why this code re-call 
"m_desktopFile.desktopGroup().readEntry(QStringLiteral("Exec"), QString())" ? 
  >  => QProcess::startDetached(commands, parts) no ?
  
  
  I would assume that works, yes.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D27863

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt added a comment.


  The split arguments are already available as `parts` above, as used in the 
klauncher call AFAICT.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D27863

To: fvogt, #frameworks, mlaurent
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt added a reviewer: mlaurent.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D27863

To: fvogt, #frameworks, mlaurent
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27863: Revert "Make it compile against last qt5.15 without deprecated method. QProcess::execute(QString) is deprecated"

[Fabian Vogt]

fvogt created this revision.
fvogt added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  This port is broken AFAICT - it tries to run the full Exec= line as binary, 
without
  splitting arguments first.
  
  This reverts commit 59cbea835502428f30c1495abe4a1b3d133103e3 
.

TEST PLAN
  Not tested at all.

REPOSITORY
  R268 KGlobalAccel

BRANCH
  somefix

REVISION DETAIL
  https://phabricator.kde.org/D27863

AFFECTED FILES
  src/runtime/kserviceactioncomponent.cpp

To: fvogt, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt added a comment.


  In D27002#605999 , @ngraham wrote:
  
  > Could this be the fix for https://bugs.kde.org/show_bug.cgi?id=398908, or 
part of it?
  
  
  This leak presented itself by steadily growing memory use while something 
still unknown triggered solid's `onMtabChanged` excessively.
  If that's also the case for the reporter of that bug, this should've made a 
big difference, otherwise only a small one.
  
  The output of heaptrack would tell for sure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks, davidedmundson, meven
Cc: ngraham, anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:213ed50634c0: Fix memory leak in 
KUrlNavigatorPlacesSelector::updateMenu (authored by fvogt).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27002?vs=74645=74989

REVISION DETAIL
  https://phabricator.kde.org/D27002

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

To: fvogt, #frameworks, davidedmundson, meven
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt added a comment.


  In D27002#605860 , @meven wrote:
  
  > User feedback: "so far so good, 160 MB Memory usage"
  >  Does not sound reassuring, I guess the user meant 160 MB compared to 200MB 
or similar prior to patch.
  
  
  Updated - should be clearer now :-)

REPOSITORY
  R241 KIO

BRANCH
  noleak

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks, davidedmundson, meven
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt edited the test plan for this revision.

REPOSITORY
  R241 KIO

BRANCH
  noleak

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks, davidedmundson, meven
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt added a comment.


  I'll land tomorrow if no objections.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt marked 3 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kurlnavigatorplacesselector.cpp:76
> Why cast?

To only delete submenus, not anything else.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: anthonyfieroni, meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> meven wrote in kurlnavigatorplacesselector.cpp:75
> Shouldn't it be done before the call to `m_placesMenu->clear();`

How would that make a difference?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D27002

To: fvogt, #frameworks
Cc: meven, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt updated this revision to Diff 74645.
fvogt added a comment.


  Make a copy, QObject::children returns const & for some reason, so gets 
modified during iteration.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27002?vs=74611=74645

BRANCH
  noleak

REVISION DETAIL
  https://phabricator.kde.org/D27002

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

To: fvogt, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21606: RFC: ThreadWeaver Job Decorators not used properly and have no effect

[Fabian Vogt]

fvogt abandoned this revision.
fvogt added a comment.


  D22758  got merged

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D21606

To: fvogt
Cc: apol, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

D27002: Fix memory leak in KUrlNavigatorPlacesSelector::updateMenu

[Fabian Vogt]

fvogt created this revision.
fvogt added a reviewer: Frameworks.
Herald added a project: Frameworks.
fvogt requested review of this revision.

REVISION SUMMARY
  This method gets called each time solid notices a change, which can in some
  setups be very frequent. It leaked memory as the submenus and their actions
  were not deallocated properly.

TEST PLAN
  Builds, waiting for user feedback.

REPOSITORY
  R241 KIO

BRANCH
  noleak

REVISION DETAIL
  https://phabricator.kde.org/D27002

AFFECTED FILES
  src/filewidgets/kurlnavigatorplacesselector.cpp

To: fvogt, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25856: Show IOSlaves that return local files when in local file mode

[Fabian Vogt]

fvogt added a comment.


  In D25856#594088 , @meven wrote:
  
  > In D25856#575125 , @fvogt wrote:
  >
  > > In D25856#575083 , @ngraham 
wrote:
  > >
  > > > In D25856#575044 , @fvogt 
wrote:
  > > >
  > > > > IMO this should be done in KIO, so that all users benefit.
  > > >
  > > >
  > > > How would you do it in KIO?
  > >
  > >
  > > Add code to KFileDialog to allow specific protocols/slaves if the `file` 
scheme is supported.
  >
  >
  > I would guess something in the slave interface sort of like 
`KDE-KIO-Protocols` in json/.protocol files expect it would be about supported 
output scheme instead of input/handled.
  >  Something like `KDE-KIO-Output-Protocols`
  >  Then there would be an equivalent to `KProtocolInfoFactory::findProtocol` 
to get those, like `KProtocolInfoFactory::findOutputProtocol`
  >  Basically ioslaves `desktop` `file` `recentlyused` `trash` and `tags` 
would have `"file"` set in there.
  
  
  IMO an overgeneralization: I can't come up with any use for that new key 
other than `file`.
  There's the `mostLocalUrl()` method already, so maybe it should just be 
indicated that `mostLocalUrl()` returns `file://`?
  
  > And also `recentdocuments` `timeline` for those who use those.
  
  AFAIK those can also have non-local URLs inside, so it's not possible to 
guarantee that only locally-reachable files are shown that way.
  
  > What do you think ?
  
  It seems like there's no easy way to fully implement this :-/

REPOSITORY
  R229 KDialog

REVISION DETAIL
  https://phabricator.kde.org/D25856

To: ngraham, #frameworks, #dolphin
Cc: fvogt, broulik, meven

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Fabian Vogt]

fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R320 KIO Extras

BRANCH
  arcpatch-D26191

REVISION DETAIL
  https://phabricator.kde.org/D26191

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kio_sftp.cpp:1480
> +if (!attr) {
> +sftp_attributes_free(attr);
> +int errorCode = toKIOError(sftp_get_error(mSftp));

This entire block is duplicated - can that be improved?

> kio_sftp.cpp:1492
> +} else {
> +sftp_attributes_free(attr);
> +int errorCode = toKIOError(sftp_get_error(mSftp));

AFAICT, this has to be done in every case.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D26191

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D26157: Port QRegExp to QRegularExpression

[Fabian Vogt]

fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R299 KDESu

BRANCH
  l-qregularexpression (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26157

To: ahmadsamir, sitter, fvogt, jriddell
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25699: [PC3 ToolButton] Have the label take into account complementary color schemes

[Fabian Vogt]

fvogt added a comment.


  This fixed the button label, but the menu itself is unsuable due to a black 
text on dark background: 
https://openqa.opensuse.org/tests/1110939#step/start_wayland_plasma5/21

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D25699

To: filipf, #plasma, ngraham, davidedmundson
Cc: mart, davidedmundson, fvogt, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25699: [PC3 ToolButton] Have the label take into account complementary color schemes

[Fabian Vogt]

fvogt added a comment.


  In D25699#575278 , @davidedmundson 
wrote:
  
  > Please don't link external sites (GitHub) in the committed message.
  >
  > RE: Menu
  >  There is nothing in QQC2::Button to add a menu
  >
  > If we want that it would have to be a custom button subclass, rather than 
something we support in the style.
  
  
  Or as a simple hack for this case, adding "▾" to the button's label.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  ok-text-colo (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25699

To: filipf, #plasma, ngraham, davidedmundson
Cc: mart, davidedmundson, fvogt, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, ngraham, bruns

D25856: Show IOSlaves that return local files when in local file mode

[Fabian Vogt]

fvogt added a comment.


  In D25856#575083 , @ngraham wrote:
  
  > In D25856#575044 , @fvogt wrote:
  >
  > > IMO this should be done in KIO, so that all users benefit.
  >
  >
  > How would you do it in KIO?
  
  
  Add code to KFileDialog to allow specific protocols/slaves if the `file` 
scheme is supported.

REPOSITORY
  R229 KDialog

REVISION DETAIL
  https://phabricator.kde.org/D25856

To: ngraham, #frameworks, #dolphin
Cc: fvogt, broulik, meven

D25856: Show IOSlaves that return local files when in local file mode

[Fabian Vogt]

fvogt added a comment.


  IMO this should be done in KIO, so that all users benefit.
  
  In D25856#575011 , @broulik wrote:
  
  > Isn't that what `KProtocolInfo::protocolClass() == ":local"` is for?
  
  
  Almost, but not quite - it just means that the resource is on the same system 
and therefore there's no hostname in the URL.
  Protocols like `man`, `tar`, `zip`, etc. are all `:local` as well, but can't 
be translated to `file:///...`.
  I'd hope there is some other way to query for that property.

REPOSITORY
  R229 KDialog

REVISION DETAIL
  https://phabricator.kde.org/D25856

To: ngraham, #frameworks, #dolphin
Cc: fvogt, broulik, meven

D25699: [PC3 ToolButton] Have the label take into account complementary color schemes

[Fabian Vogt]

fvogt added a comment.


  The check for the prefix was added in 
`bf1d1cc6b2ad37cb586f44b56fa2438ed3a5dbfc`, while the `control.flat` one got 
added much earlier.
  
  The labels are visible again with just the `control.flat` condition, but the 
prefix one might be needed as well for non-breeze themes.
  
  There's a part missing though, the triangle (visible on 
https://openqa.opensuse.org/tests/1105226#step/start_wayland_plasma5/21) is 
gone. That seems to be a feature lost with PC3 :-(

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  ok-text-colo (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D25699

To: filipf, #plasma, ngraham
Cc: fvogt, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  In D23384#572198 , @ngraham wrote:
  
  > I just tested writing today, for files opened in 3rd-party apps that get 
the FUSE mount path. Results:
  >  ...
  
  
  Was that with or without the KIO::open merge request?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  In D23384#571276 , @ngraham wrote:
  
  > I'm afraid that even with that change, the issue is still present. I 
honestly don't think it would be the worst thing in the world if we always 
handed the kio-fuse paths to apps that don't use ioslaves.
  
  
  It would be. I like to have links like http://kde.org opened properly in the 
web browser, ftp://some/where opened in an FTP client and so on...
  Media players know more about the format and streaming it than kio-fuse ever 
could, so avoiding layers in between if possible is definitely an advantage.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  In D23384#570830 , @ngraham wrote:
  
  > In D23384#570735 , @fvogt wrote:
  >
  > > > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  > >
  > > Ignoring the elephant in the room which is that this diff forces 
everything through `mountUrl`, that's the expected behavior with a plain HTTP 
URL as the size isn't known until the file is downloaded. So `stat` reports a 
size of `1` until the file is actually opened.
  > >  This is unavoidable, otherwise every `ls` would trigger a download of 
all files. If handling this better is important, `HTTPProtocol::stat` could use 
a `HEAD` request to get the `Content-Length`, but that doesn't work in all 
cases either.
  >
  >
  > Well, we need to fix this or else it's a very serious regression that 
breaks a huge part of the desktop. Opening links in a web browser is pretty 
basic functionality.
  
  
  That issue is not in kio-fuse:
  
  > Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, ...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  In D23384#570331 , @ngraham wrote:
  
  > In D23384#570118 , @fvogt wrote:
  >
  > > Please try both of the following:
  >
  >
  > Done. Here are the log files:
  >
  > F7793378: kio-fuse.log 
  
  
  Just as expected:
  
unique: 4248, opcode: READ (15), nodeid: 13, insize: 80, pid: 24522
org.kde.kio.fuse: Read "Dean plays with Winnie.mov" 0 65536
org.kde.kio.fuse: Fetching cache for "Dean plays with Winnie.mov"
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
   unique: 4248, success, outsize: 65552
kf5.kio.core: KLocalSocket(0x212cfd0) Jumbo packet of 65534 bytes
unique: 4250, opcode: GETATTR (3), nodeid: 13, insize: 56, pid: 24522
   unique: 4250, success, outsize: 120
unique: 4252, opcode: READ (15), nodeid: 13, insize: 80, pid: 24522
org.kde.kio.fuse: Read "Dean plays with Winnie.mov" 153833472 36864
  
  It first reads the header and then jumps to the file's end to read a few KiB 
there.
  Nothing kio-fuse can do about that, except trying to use `KIO:open` in the 
future, but that has some other drawbacks...
  
  > F7793379: strace output.log 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  > Clicking a http link in my chat app now kiofuses it and then has the 
browser desperately try to open /run/user/1000/kio-fuse-bla/http/kde.org/ - it 
only gets the first character in the file, so I usually just see < for a HTML 
page
  
  Ignoring the elephant in the room which is that this diff forces everything 
through `mountUrl`, that's the expected behavior with a plain HTTP URL as the 
size isn't known until the file is downloaded. So `stat` reports a size of `1` 
until the file is actually opened.
  This is unavoidable, otherwise every `ls` would trigger a download of all 
files. If handling this better is important, `HTTPProtocol::stat` could use a 
`HEAD` request to get the `Content-Length`, but that doesn't work in all cases 
either.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  Unfortunately the `kio-fuse -d` output is incomplete, probably because Qt was 
too smart and logged to the journal instead...
  It's visible that there are multiple processes reading the file, maybe 
thumbnailing is in progress?
  Can you try with thumbnails in dolphin disabled?
  
  Please try both of the following:
  
  Add this to kio-fuse and rebuild:
  
diff --git a/kiofusevfs.cpp b/kiofusevfs.cpp
index 7755b56..a3a4c72 100644
--- a/kiofusevfs.cpp
+++ b/kiofusevfs.cpp
@@ -859,6 +859,7 @@ void KIOFuseVFS::read(fuse_req_t req, fuse_ino_t ino, 
size_t size, off_t off, fu
return;
case KIOFuseNode::NodeType::RemoteFileNode:
{
+   qDebug(KIOFUSE_LOG) << "Read" << node->m_nodeName << off << 
size;
auto remoteNode = 
std::dynamic_pointer_cast(node);
that->awaitBytesAvailable(remoteNode, off + size, [=](int 
error) {
if(error != 0 && error != ESPIPE)
  
  then
  
QT_LOGGING_RULES=*.debug=true QT_FORCE_STDERR_LOGGING=1 
~/kde/usr/lib64/libexec/kio-fuse -d $yourfavlocation &>somelog.file
  
  and to get just the syscalls which totem makes:
  
strace -fefile totem $yourfavlocation/where/the/video.is

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Fabian Vogt]

fvogt added a comment.


  **Issue #1:**
  
  That happens because the .desktop file sets 
`X-KDE-Protocols=ftp,http,https,mms,rtmp,rtsp,sftp,smb`:
  https://code.videolan.org/videolan/vlc/blob/master/share/vlc.desktop.in#L124
  
  **Issue #2:**
  
  The only explanation I have for that is that totem for some reason starts 
reading the file from the end for some reason.
  If you start `kio-fuse -d` manually (kill the other kio-fuse process first) 
and then use totem again, what's the debug output?
  
  If totem behaves like this, only `KIO::open` support might help a bit, with 
the cost of increased latency.
  That needs support for something like `KIO::truncate` though, so might take a 
while.
  
  **Issue #3:**
  
  Everything is in a separate process and async already. When/how does it 
happen?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23384

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D25107: Add install dir for systemd units

[Fabian Vogt]

fvogt accepted this revision.
fvogt added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> KDEInstallDirs.cmake:245
>  set(_LIBDIR_DEFAULT "lib")
> -# Override this default 'lib' with 'lib64' iff:
> +# Override this default 'lib' with 'lib64' if:
>  #  - we are on a Linux, kFreeBSD or Hurd system but NOT cross-compiling

IMO the `iff` here can stay

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25107

To: davidedmundson, apol, fvogt
Cc: cgiboudeaux, fvogt, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, 
bencreasy, michaelh, ngraham, bruns

D25107: Add install dir for systemd units

[Fabian Vogt]

fvogt reopened this revision.
fvogt added a comment.
This revision is now accepted and ready to land.


  AFAICT this breaks if `LIBDIR` != "lib". `systemd` only looks in `/usr/lib` 
AFAICT, so hardcoding to `$prefix/lib/systemd` might be better.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D25107

To: davidedmundson, apol
Cc: fvogt, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D25107: Add install dir for systemd units

[Fabian Vogt]

fvogt requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D25107

To: davidedmundson, apol, fvogt
Cc: fvogt, kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

D25159: Fix linking to libssh 0.9.1

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  The libssh maintainer is likely reverting the change, so this should not be 
necessary.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D25159

To: arojas, apol, sitter, fvogt
Cc: fvogt, sitter, asn, apol, asturmlechner, kde-frameworks-devel, kfm-devel, 
pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D24245: Add support for passing Unix file descriptors

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Unfortunately, this breaks public API and ABI by modifying KAuth::ActionReply.
  
  I'm not sure whether there has to be any compatibility for the DBus API, but 
I guess not (logging out and back in should suffice to make sure the backend 
used is the same).
  
  I wonder why you made a wrapper around `QDBusUnixFileDescriptor`, does it not 
work inside a `QVariant` OOTB?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23692: kdesu: set kernel flags to prevent ptrace instead of relying on setgid

[Fabian Vogt]

fvogt added a reviewer: Frameworks.

REPOSITORY
  R299 KDESu

REVISION DETAIL
  https://phabricator.kde.org/D23692

To: maltek, adridg, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23694: Add support for sshfs to the fstab backend

[Fabian Vogt]

fvogt added a comment.


  `fuse.sshfs` is used by kdeconnect as well, does that cause some kind of 
conflict?
  
  If not, LGTM.
  
  Could be improved by adding other filesystems (curlftpfs?) as well and using 
something like
  
  `QStringList{"nfs", "nfs4", "smbfs", "cifs", "fuse.sshfs"}.contains(fstype);` 
instead.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D23694

To: lbeltrame, bruns, broulik, fvogt
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23454: Fixing bug where MTP slave does not return error in stat()/mimetype()

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:8c42ec63200f: Fixing bug where MTP slave does not return 
error in stat()/mimetype() (authored by feverfew, committed by fvogt).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23454?vs=64614=64648

REVISION DETAIL
  https://phabricator.kde.org/D23454

AFFECTED FILES
  mtp/kio_mtp.cpp

To: feverfew, chinmoyr, akrutzler, elvisangelaccio, dfaure
Cc: kde-frameworks-devel, kfm-devel, fvogt, aprcela, vmarinescu, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D22758: Add a done signal to FindMatchesJob instead of using QObjectDecorator wrongly

[Fabian Vogt]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R308:54e18b0d9b0d: Add a done signal to FindMatchesJob instead 
of using QObjectDecorator wrongly (authored by fvogt).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22758?vs=62583=64191

REVISION DETAIL
  https://phabricator.kde.org/D22758

AFFECTED FILES
  src/runnerjobs.cpp
  src/runnerjobs_p.h
  src/runnermanager.cpp

To: fvogt, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23029: Fix the attica pkgconfig file.

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> libKF5Attica.pc.cmake:3
> +exec_prefix=${BIN_INSTALL_DIR}
>  libdir=${LIB_INSTALL_DIR}
> +includedir=${KF5_INCLUDE_INSTALL_DIR}

In ECM and Qt this is absolute, `${prefix}/lib64`, same for `exec_prefix`.

REPOSITORY
  R235 Attica

REVISION DETAIL
  https://phabricator.kde.org/D23029

To: cgiboudeaux
Cc: fvogt, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22979: Security: remove support for $(...) in config keys with [$e] marker.

[Fabian Vogt]

fvogt added a comment.


  In D22979#508493 , @kives wrote:
  
  > Does anyone think this can be easily backported to previous versions of KDE 
in upstream distros such as Kubuntu, etc.?
  
  
  I backported this down to KConfig 5.20 and KDELibs 4.14.18, differences were 
trivial to resolve.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D22979

To: dfaure, mdawson, aacid, broulik, davidedmundson, kossebau, apol, sitter, 
security-team
Cc: kives, ZaWertun, rikmills, fvogt, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D22723: Fix RunnerManager::queryFinished()

[Fabian Vogt]

fvogt added a comment.


  > QObjects live in their own thread and shouldn't be used outside.
  >  https://doc.qt.io/qt-5/qobject.html#thread
  > 
  > In your patch we are emitting the signal from the run thread instead of the 
actual object's thread. This is wrong.
  
  Needs a `Qt::QueuedConnection` indeed. Or the job has to be moved to a 
different thread, which means that Qt makes the connection a queued one 
automatically.
  
  > Also I'd say there's value in using tools (i.e. Threadweaver) as it's meant 
to be used, despite you don't liking its semantics.
  
  Yes, improving the Threadweaver API is the best option here.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D22723

To: apol, #frameworks, fvogt, davidedmundson
Cc: aacid, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22723: Fix RunnerManager::queryFinished()

[Fabian Vogt]

fvogt added a comment.


  In D22723#502365 , @aacid wrote:
  
  > I honestly don't see the problem with this patch, one may argue that the 
ThreadWeaver API is awkward, ok, but this is using it correctly AFAICS, i.e. 
have a ThreadWeaver::QObjectDecorator, give it a ThreadWeaver::Job on its 
constructor, and go on from there.
  
  
  Correctly as in "it's supposed to work" yes, but it's not as it was intended 
to be AFAICT.
  This now adds a custom private and friend class (ugh) which means that now 
there's a sandwich out of three classes:
  `FindMatchesJob -(inherits)> QObjectDecorator -(uses)> 
FindMatchesJobInternal` while only a single one would do the job.
  I did a PoC of dropping use of `QObjectDecorator`: 
https://phabricator.kde.org/D22758
  
  Here, one may argue that it reinvents the wheel, but if the current iteration 
of the wheel is square I'd say it's allowed.
  It also gets rid of a heap allocation.
  
  What do you think?
  
  If you don't like it I won't object landing this internal class, but please 
add a long comment explaining why it was done like this.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D22723

To: apol, #frameworks, fvogt, davidedmundson
Cc: aacid, kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22758: Add a done signal to FindMatchesJob instead of using QObjectDecorator wrongly

[Fabian Vogt]

fvogt created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Currently KRunner uses QObjectDecorator wrongly and changing the
  design to fix that would not only be a lot of work, but also include downsides
  like having two custom Job classes, making the code harder to read.
  So merge the part of QObjectDecorator that would've been used here into the
  FindMatchesJob class and drop use of QObjectDecorator.

TEST PLAN
  Added a debug statement in the jobDone slot, it's called.

REPOSITORY
  R308 KRunner

BRANCH
  betterhack

REVISION DETAIL
  https://phabricator.kde.org/D22758

AFFECTED FILES
  src/runnerjobs.cpp
  src/runnerjobs_p.h
  src/runnermanager.cpp

To: fvogt
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22723: Fix RunnerManager::queryFinished()

[Fabian Vogt]

fvogt added a comment.


  In D22723#501907 , @apol wrote:
  
  > In D22723#501690 , @fvogt wrote:
  >
  > > Looks like a hack still, with two Job objects for each job...
  > >
  > > What about just merging `QObjectDecorator` into `FindMatchesJobInternal` 
by basically just adding a custom `done` signal and ignoring the entire 
"decorators which are actually wrappers" business?
  > >
  > > IMO this new `FindMatchesJobInternal` class makes it even less obvious 
what's actually going on.
  >
  >
  > This is how ThreadWeaver and especially QObjectDecorator is meant to be 
used.
  
  
  The way `ThreadWeaver::QObjectDecorator` is apparently meant to be used is to 
wrap the custom job inside a `QObjectDecorator` and use only the wrapper from 
there on:
  
  
https://github.com/KDE/threadweaver/blob/239cf8fffe687c0a758f5170a40b26ae0acef7b0/autotests/QueueTests.cpp#L157
  
autoDeleteJob = new QObjectDecorator(new AppendCharacterJob(QChar('a'), 
));
[...]
QVERIFY(autoDeleteJob != nullptr);
QVERIFY(connect(autoDeleteJob, SIGNAL(done(ThreadWeaver::JobPointer)),
SLOT(deleteJob(ThreadWeaver::JobPointer;
  
  which is not great, to say the least.
  
  What this patch does on the surface is wrap the `QObjectDecorator` inside an 
object that fakes being the custom job itself.
  That's actually a slightly better design than the code above does as now the 
pointer passed from the `done` signal is not the `QObjectDecorator` pointer but 
the custom class.
  
  Still, IMO much better would be to just merge the `QObjectDecorator` into the 
custom job as that would both avoid creating two job objects per job and make 
the code clearer and shorter.
  
  > I don't really know why you say it's confusing. The confusing part so far 
was that jobDone slot was never called.
  
  Which likely happened because the author was confused by the code...

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D22723

To: apol, #frameworks, fvogt, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22723: Fix RunnerManager::queryFinished()

[Fabian Vogt]

fvogt added a reviewer: davidedmundson.
fvogt added a comment.


  Looks like a hack still, with two Job objects for each job...
  
  What about just merging `QObjectDecorator` into `FindMatchesJobInternal` by 
basically just adding a custom `done` signal and ignoring the entire 
"decorators which are actually wrappers" business?
  
  IMO this new `FindMatchesJobInternal` class makes it even less obvious what's 
actually going on.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D22723

To: apol, #frameworks, fvogt, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22555: [RFC] Add a kded module to manage various available fuse mount services

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> mountservicemanager.cpp:42
> +{
> +KConfigGroup cfg = 
> KConfigGroup(KSharedConfig::openConfig(QStringLiteral("fusemanagerrc")), 
> QStringLiteral("Fuse Services"));
> +return cfg.readEntry(url.scheme(), QString());

This line is the only place "fuse" is ever mentioned, so maybe this should be 
renamed?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D22555

To: chinmoyr, fvogt
Cc: kde-frameworks-devel, LeGast00n, sbergeron, michaelh, ngraham, bruns

D22147: Better use of Qt APIs in Plasma::Theme

[Fabian Vogt]

fvogt resigned from this revision.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D22147

To: apol, #plasma, #frameworks
Cc: bruns, tcanabrava, fvogt, broulik, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham

D22147: Better use of Qt APIs in Plasma::Theme

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> apol wrote in svg.cpp:317
> Please note this is only to make sure the regex was properly compiled. It 
> isn't matching there yet.

It really does not look that way as you're immediately using captures after 
that.

If that's really what you want (which I doubt), it should be 
`sizeHintedKeyExpr.isValid()` instead and be done before the foreach.

Currently it would just add `QSize(0, 0)` to `elementsWithSizeHints` even for 
"öoiawze9pv5z2p3v4" as key.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D22147

To: apol, #plasma, #frameworks, fvogt
Cc: tcanabrava, fvogt, broulik, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D22147: Better use of Qt APIs in Plasma::Theme

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> svg.cpp:317
> +const auto match = sizeHintedKeyExpr.match(key);
> +if (match.isValid()) {
> +QString baseElementId = match.captured(3);

`isValid` is always true, you probably want to use `hasMatch` instead.

This is not obvious, I only noticed this because I debugged this error before 
(https://phabricator.kde.org/D17359)

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D22147

To: apol, #plasma, #frameworks, fvogt
Cc: fvogt, broulik, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22080: [Fstab] Show mounted "overlay" filesystems

[Fabian Vogt]

fvogt accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  submit

REVISION DETAIL
  https://phabricator.kde.org/D22080

To: bruns, ngraham, fvogt, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22081: [Fstab] Select appropriate icon for home or root directory

[Fabian Vogt]

fvogt accepted this revision.
fvogt added a comment.
This revision is now accepted and ready to land.


  With this and D22080  applied, there's an 
entry for / now:
  
  F6932935: Screenshot_20190625_092256.png 

  
  On a regular system, nothing seems to have changed, so the changes seem to be 
fine.

REPOSITORY
  R245 Solid

BRANCH
  fstab_overlayfs

REVISION DETAIL
  https://phabricator.kde.org/D22081

To: bruns, ngraham, fvogt, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D15739: [Places panel] Don't show Root by default

[Fabian Vogt]

fvogt added a comment.


  In D15739#486043 , @bruns wrote:
  
  > In D15739#486020 , @fvogt wrote:
  >
  > > In D15739#486009 , @bruns 
wrote:
  > >
  > > > It actually already does, via the fstab backend (currently network 
(SMB/NFS) and various fuse mounts). Fstab in this case also includes everything 
in MTAB, should be quite easy to extend this.
  > > >
  > > > What is the output of `cat /proc/self/mounts` (fell free to remove 
anything irrelevant, like cgroups)?
  > >
  > >
  > > The entries involved in / are these:
  > >
  > >   /dev/sr0 /run/initramfs/live iso9660 ro,relatime 0 0
  > >   /dev/loop0 /run/initramfs/squashfs_container squashfs ro,relatime 0 0
  > >   /dev/loop1 /run/rootfsbase ext4 ro,relatime,data=ordered 0 0
  > >   LiveOS_rootfs / overlay 
rw,relatime,lowerdir=/run/rootfsbase,upperdir=/run/overlayfs/rw,workdir=/run/overlayfs/work
 0 0
  > >
  >
  >
  > Hm, loop1 is the `lowerdir` of the overlay - how are loop0 and sr0 
involved, are these the backing files?
  
  
  /dev/sr0 contains a squashfs, which is visible as /dev/loop0.
  /dev/loop0 contains an ext4 image, which is visible as /dev/loop1.
  
  So it's doubly indirect.
  
  > Though, the relevant part is `mntent.mnt.type == "overlay"` and 
`mntent.mnt_dir == "/"`. Matching for "overlay" is probably sufficient.
  
  If adding such a special case makes sense, yes. I'd even argue about 
'mntent.mnt_dir == "/" && isKnownFilesystem(mntent)' or something like that to 
ensure that an entry for `/` is always provided.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15739

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: andriusr, fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, 
ndavis, romangg, bruns, davidedmundson, abetts, svenmauch, broulik, 
acrouthamel, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D15739: [Places panel] Don't show Root by default

[Fabian Vogt]

fvogt added a comment.


  In D15739#486009 , @bruns wrote:
  
  > In D15739#485983 , @fvogt wrote:
  >
  > > Even if all (block) devices and their mountpoints were shown in the 
devices view, there would be no equivalent of "/". One loop device provides the 
read-only base for /, but that's actually more confusing than the current state 
as it looks like /, but is only a read-only view of "the past".
  > >
  > > I guess solid needs to gain support for mountpoints not backed by devices?
  >
  >
  > It actually already does, via the fstab backend (currently network 
(SMB/NFS) and various fuse mounts). Fstab in this case also includes everything 
in MTAB, should be quite easy to extend this.
  >
  > What is the output of `cat /proc/self/mounts` (fell free to remove anything 
irrelevant, like cgroups)?
  
  
  The entries involved in / are these:
  
/dev/sr0 /run/initramfs/live iso9660 ro,relatime 0 0
/dev/loop0 /run/initramfs/squashfs_container squashfs ro,relatime 0 0
/dev/loop1 /run/rootfsbase ext4 ro,relatime,data=ordered 0 0
LiveOS_rootfs / overlay 
rw,relatime,lowerdir=/run/rootfsbase,upperdir=/run/overlayfs/rw,workdir=/run/overlayfs/work
 0 0

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15739

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: andriusr, fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, 
ndavis, romangg, bruns, davidedmundson, abetts, svenmauch, broulik, 
acrouthamel, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D15739: [Places panel] Don't show Root by default

[Fabian Vogt]

fvogt added a comment.


  In D15739#485979 , @ngraham wrote:
  
  > In D15739#485978 , @fvogt wrote:
  >
  > > Well, the only mount of interest is the overlay mount of /, which is not 
backed by any (block) device but rather by two filesystems. AFAICT udisks 
doesn't care about this kind of mount, not sure what solid does.
  > >
  > > Showing an overlay mount as "Device" is also somewhat wrong IMO.
  >
  >
  > I'm saying that regardless of the technical details of the backend, it 
never makes sense to not show any devices. From the user's perspective, there 
is always a device of some sort, regardless of its underlying configuration. 
There can't not be a device. That doesn't make sense; software requires 
hardware.
  
  
  Even if all (block) devices and their mountpoints were shown in the devices 
view, there would be no equivalent of "/". One loop device provides the 
read-only base for /, but that's actually more confusing than the current state 
as it looks like /, but is only a read-only view of "the past".
  
  I guess solid needs to gain support for mountpoints not backed by devices?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15739

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham

D15739: [Places panel] Don't show Root by default

[Fabian Vogt]

fvogt added a comment.


  In D15739#485976 , @ngraham wrote:
  
  > In D15739#485975 , @fvogt wrote:
  >
  > > No, there's in fact no devices section at all as it would be empty. The 
live cd itself is marked as ignored as there's nothing useful on there and it 
can't be ejected and the other layers are mounted from loop devices which 
udisks apparently ignores as well.
  >
  >
  > That seems like a bug in the way the devices list is populated or 
displayed. There can't be no devices, that's silly.
  
  
  Well, the only mount of interest is the overlay mount of /, which is not 
backed by any (block) device but rather by two filesystems. AFAICT udisks 
doesn't care about this kind of mount, not sure what solid does.
  
  Showing an overlay mount as "Device" is also somewhat wrong IMO.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15739

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham

D15739: [Places panel] Don't show Root by default

[Fabian Vogt]

fvogt added a comment.


  In D15739#485960 , @ngraham wrote:
  
  > In D15739#485949 , @fvogt wrote:
  >
  > > I just noticed this in openQA runs from a live medium. There is no 
single-click way to get to the root directory anymore, as / is mounted as an 
overlayfs of a tmpfs and a squashfs container.
  > >
  > > Not sure what the best way to improve this is, any idea?
  >
  >
  > So there's no entry in the Devices section that goes to `/`?
  
  
  No, there's in fact no devices section at all as it would be empty. The live 
cd itself is marked as ignored as there's nothing useful on there and it can't 
be ejected and the other layers are mounted from loop devices which udisks 
apparently ignores as well.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15739

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham

D15739: [Places panel] Don't show Root by default

[Fabian Vogt]

fvogt added a comment.


  I just noticed this in openQA runs from a live medium. There is no 
single-click way to get to the root directory anymore, as / is mounted as an 
overlayfs of a tmpfs and a squashfs container.
  
  Not sure what the best way to improve this is, any idea?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15739

To: meven, #dolphin, #vdg, tcanabrava, ngraham, elvisangelaccio
Cc: fvogt, meven, elvisangelaccio, Codezela, davidc, tcanabrava, ndavis, 
romangg, bruns, davidedmundson, abetts, svenmauch, broulik, acrouthamel, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R308:a07027cd4f22: Dont delay emission of matchesChanged 
indefinitely (authored by fvogt).

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21607?vs=59252=60049

REVISION DETAIL
  https://phabricator.kde.org/D21607

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik, ngraham
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  Before I land this, I'd like if someone other than me tries krunner with this 
patch applied and judges the result with several runners. The difference is 
very noticable with the appstream runner as it does not batch results.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D21607

To: fvogt, #frameworks, broulik
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, michaelh

D8532: [WIP] Restrict file extractor with Seccomp

[Fabian Vogt]

fvogt added a comment.


  In D8532#478224 , @bruns wrote:
  
  > I totally agree with fvogt here - the extractors should just receive a 
readonly file descriptor.
  >
  > For this, there are several steps required:
  >
  > 1. let the extractors work with file descriptors (KFileMetaData)
  > 2. make sure the extractor plugins are fully initialized before receiving 
file descriptors
  > 3. actually feed file descriptors to the extractor
  >
  >   (1.) is trivial for some extractors (e.g. taglib), for others it may be 
hard. (2.) depends on several things - the plugins must be instantiated early 
(which clashes with the lazy loading), and the plugin may not load any external 
resources later on.
  >
  >   Using file descriptors has another benefit - currently, the file is 
stat'ed and so on, and then the corresponding path is fed to the extractor. It 
would be much better to open the file, use fstatat and friends, run the 
extractor and close the file again.
  
  
  What could also be done as an intermediate step is to whitelist opening 
read-only fds for metadata extractions. That way something like plugin loading 
is also covered and not many changes are required.
  The sandbox could be opt-in for plugins which just specify that they support 
sandboxing using the specified whitelist, with plugins which don't support 
sandboxing disabled by default.
  I used this approach in a (private so far) branch for sandboxing the 
thumbnail kio slave and it works well.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D8532

To: davidk, apol, ossi, #frameworks, smithjd, bruns
Cc: fvogt, mgallien, kde-frameworks-devel, michaelh, #baloo, detlefe, ngraham, 
nicolasfella, LeGast00n, domson, ashaposhnikov, astippich, spoorun, bruns, 
abrahams

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt updated this revision to Diff 59252.
fvogt added a comment.


  New algorithm with no delay if not necessary.

REPOSITORY
  R308 KRunner

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21607?vs=59203=59252

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21607

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  In D21607#474772 , @fvogt wrote:
  
  > So for the stable branches I'd like to keep the current version of the diff 
with a latency of [100,599] while for master something like the above with a 
latency of [0,500] can be tried.
  
  
  And I just now realize that I'm stupid and mixed this up with milou. krunner 
is a framework so there is no stable branch...
  
  I'll update the diff to the [0,250] case.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D21607

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  In D21607#474771 , @fvogt wrote:
  
  > I'm thinking about doing it completely differently now though, with a 0 
latency case (untested):
  >
  >   if(lastMatchChangeSignalled.hasExpired(250)) {
  >   matchChangeTimer.stop();
  >   emit q->matchesChanged(context.matches());
  >   } else {
  >   matchChangeTimer.start(250 - lastMatchChangeSignalled.elapsed());
  >   }
  >
  
  
  Just tried this and it's not too bad, but a noticable change in behaviour. As 
results are shown basically immediately once they're available, it's now 
visible how the entries are filled.
  
  So for the stable branches I'd like to keep the current version of the diff 
with a latency of [100,599] while for master something like the above with a 
latency of [0,500] can be tried.

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D21607

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt added a comment.


  In D21607#474763 , @bruns wrote:
  
  > This would emit the signal more often, but wouldn't
  >
  >   if (!matchChangeTimer.isActive())
  > matchChangeTimer.start(100)
  >
  >
  > achieve essentially the same?
  
  
  That would do it even more often.
  
  I'm thinking about doing it completely differently now though, with a 0 
latency case (untested):
  
if(lastMatchChangeSignalled.hasExpired(100)) {
matchChangeTimer.stop();
emit q->matchesChanged(context.matches());
} else {
matchChangeTimer.start(100 - lastMatchChangeSignalled.expired());
}
  
  What do you think?

REPOSITORY
  R308 KRunner

REVISION DETAIL
  https://phabricator.kde.org/D21607

To: fvogt, #frameworks, broulik
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21607: Don't delay emission of matchesChanged indefinitely

[Fabian Vogt]

fvogt created this revision.
fvogt added reviewers: Frameworks, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  Currently the signal is only emitted if there was no change to the matches in
  the last 100ms. Especially during krunner startup and early result collection,
  this is unlikely to happen though, so make sure that the signal is emitted
  at least once every ~500ms.

TEST PLAN
  Sometimes results only appeared after a noticable delay, now this
  delay is much shorter.

REPOSITORY
  R308 KRunner

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21607

AFFECTED FILES
  src/runnermanager.cpp

To: fvogt, #frameworks, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21606: RFC: ThreadWeaver Job Decorators not used properly and have no effect

[Fabian Vogt]

fvogt created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
fvogt requested review of this revision.

REVISION SUMMARY
  ThreadWeaver Job Decorators don't actually hook into an existing job.
  Instead, they provide the same interface as the job and just pass through
  all calls.
  
  The way the QObjectDecorator is used by KRunner results in no signals actually
  getting emitted, making most of the code pointless.
  
  This patch is just a quick hack to show what would happen if the jobDone 
signal
  worked and is mostly intended to present the issue.

TEST PLAN
  jobDone is called now.

REPOSITORY
  R308 KRunner

BRANCH
  hack

REVISION DETAIL
  https://phabricator.kde.org/D21606

AFFECTED FILES
  src/runnerjobs.cpp

To: fvogt
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D20659: Copy container in Component::cleanUp before interating

[Fabian Vogt]

fvogt added a comment.


  In D20659#452642 , @ngraham wrote:
  
  > Did this fix https://bugs.kde.org/show_bug.cgi?id=406642?
  
  
  No, that particular crash (bug 406426) is already fixed. I marked it as dup.

REPOSITORY
  R268 KGlobalAccel

REVISION DETAIL
  https://phabricator.kde.org/D20659

To: fvogt, #frameworks, davidedmundson
Cc: ngraham, lbeltrame, kde-frameworks-devel, michaelh, bruns