Bug 423857

When clicked, Discover's notifier runs an ApplicationLauncherJob:

auto *job = new KIO::ApplicationLauncherJob(KService::serviceByDesktopName(QStringLiteral("org.kde.discover")));
    job->setUiDelegate(new KNotificationJobUiDelegate(KJobUiDelegate::AutoErrorHandlingEnabled));
    job->start();

Clicking on it when Discover is already open causes Discover's Task Manager entry to turn orange, so clearly the job or something underneath it knows that there's an existing instance already open.

I wonder if this is a case of KWin inappropriately preventing the application from coming to the front.

Are you using wayland? On wayland we don't have a mechanism to raise windows yet. That's a known and important issue in KWin.

No, on X11. I know this feature isn't available yet on Wayland.

It should be working in X11.

Could you see that this line gets called?
mainWindow->rootObject()->raise();

This code hasn't changed over the years, so it's possible that it's a KWin issue.

Yep, it does. I guess this is a KWin regression then. Moving over there for now.

Of note, if I disable focus stealing prevention, everything works properly. So I guess KWin is mis-characterizing this action as focus stealing, which is not accurate since it's an explicitly user-initiated action.

...Where by "disable focus stealing prevention" I mean "change focus stealing prevention from Low to None in the Window Behavior KCM".

There has been one potentially relevant change in kwin wrt focus.

But there clearly have also been changes on the discover side as  ApplicationLaunchJob didn't exist a few months ago. If it doesn't pass focus correctly then kwin could still well be doing the right thing.

The potentially relevant kwin commit: a0c4a8e766a2160213838daf6f71c7ae6c3705df

Reverting a0c4a8e766a2160213838daf6f71c7ae6c3705df has no effect for me, BTW.

Then I very much doubt it's kwin at fault.

So then the problem is in ApplicationLauncherJob or Discover?

What exactly is the problem? Why does KWin think focus is being stolen?

*** Bug 426632 has been marked as a duplicate of this bug. ***

Seems like the bug is somewhere in ApplicationLauncherJob or KWin; either the job isn't marking the request correctly so KWin doesn't know that it's not focus stealing, or else it's doing stuff right but KWin does the wrong thing anyway. Don't know which one yet.

The problem is that discover doesn't provide any startup id. I doubt it's a kwin bug.

Created attachment 131824 [details]
PoC

Could somebody please verify that this patch fixes the bug? Note that it's just a poc patch.

I was debugging something similar elsewhere (for something else)

There is a bug is the KIO side, but in the part that's now disabled again.

Bug is:

KStartupInfo::setupStartupEnv does "qputenv" this updates the env of the application about to launch.

later we call

systemdprocessrunner.cpp
            { QStringLiteral("Environment"), m_process->environment() },


but m_process->environment() is just an empty array. There's some QProcess code to say if it's empty we take the system env, which is why it normally works, but that doesnt' help here. We need to refactor the startup env side a bit.

> but m_process->environment() is just an empty array.

as expected, KProcessRunner needs to call QProcess::setProcessEnvironment(), doesn't it?

Yes, but ideally we want that to only include the environment values that are worth exporting, not the entire thing that process currently has.

(In reply to Vlad Zahorodnii from comment #15)
> Created attachment 131824 [details]
> PoC
> 
> Could somebody please verify that this patch fixes the bug? Note that it's
> just a poc patch.
Doesn't seem to help, sorry.

@Nate maybe you could try Vlad's patch but adding the setter on the ::showDiscoverUpdates() method instead of ::showDiscover()?

No, that doesn't change anything for me.

*** Bug 429573 has been marked as a duplicate of this bug. ***

*** Bug 430561 has been marked as a duplicate of this bug. ***

*** Bug 430547 has been marked as a duplicate of this bug. ***

Dupes piling up; marking as VHI.

Nothing indicates this is kwin at fault, and nothing indicates ApplicationLauncherJob is at fault (https://invent.kde.org/frameworks/kio/-/merge_requests/298)

KDBusService still has multiple things wrong.

I have some working code:

https://phabricator.kde.org/P655
in discover

https://phabricator.kde.org/P656
in KDBusService

With that, startupID is passed correctly.

Thanks David!

Note that we have duplicate bug reports for the same issue in Elisa, Konversation, and the Emoji picker. Will the KDBusService patch fix all of them, or will they  each need app-specific fixes as well, like Discover apparently does?

Note that I cannot merge that KDBusService patch as-is. 

I can't depend on KWindowsystem, but somehow we need to relay that startup id value to the outside.

The rest of those clients do need to do something in their handling of KDBusService::activate

In fairness, the docs for KDBusService to specify this, so hopefully clients should be following that.

A possibly relevant merge request was started @ https://invent.kde.org/plasma/discover/-/merge_requests/61

Git commit 57dc3b725d1bfed713dffbc13543f79300503d61 by David Edmundson.
Committed on 04/01/2021 at 15:58.
Pushed by ngraham into branch 'master'.

Set startupID before raising

Otherwise it will be blocked correctly by window manager focus policies

M  +1    -1    CMakeLists.txt
M  +1    -0    discover/CMakeLists.txt
M  +12   -1    discover/main.cpp

https://invent.kde.org/plasma/discover/commit/57dc3b725d1bfed713dffbc13543f79300503d61