Bug 423857 - Discover window not raised when activated by clicking on the notifier tray item
Summary: Discover window not raised when activated by clicking on the notifier tray item
Status: RESOLVED FIXED
Alias: None
Product: Discover
Classification: Applications
Component: Notifier (show other bugs)
Version: 5.19.2
Platform: Neon Linux
: VHI normal
Target Milestone: ---
Assignee: Aleix Pol
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2020-07-04 01:46 UTC by Michael
Modified: 2021-01-05 02:49 UTC (History)
9 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.21
Sentry Crash Report:


Attachments
PoC (1.85 KB, patch)
2020-09-21 07:32 UTC, Vlad Zahorodnii
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael 2020-07-04 01:46:46 UTC
SUMMARY

When clicking on the update notifier in the system tray, Discover doesn't become frontmost if Discover is already open on the desktop but hidden behind other windows. This is confusing, as it appears that nothing happens.


STEPS TO REPRODUCE

1. I see that the update notifier is present so I click on it. Discover launches.
2. I do other work and the Discover window gets buried. I forget about it.
3. I click on the update notifier and nothing happens. Discover happens to be buried under other windows but it doesn't become frontmost. So it seems something is broken.


EXPECTED BEHAVIOUR

Discover should become frontmost if it's already running and I click on the update notifier.


SOFTWARE/OS VERSIONS

Operating System: KDE neon 5.19
KDE Plasma Version: 5.19.2
KDE Frameworks Version: 5.71.0
Qt Version: 5.14.2
Kernel Version: 5.3.0-62-generic
OS Type: 64-bit
Processors: 4 × Intel® Core™ i5-2467M CPU @ 1.60GHz
Memory: 7.7 GiB of RAM
Graphics Processor: Mesa DRI Intel® Sandybridge Mobile
Comment 1 Nate Graham 2020-07-09 19:43:07 UTC
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.
Comment 2 Aleix Pol 2020-07-09 20:12:10 UTC
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.
Comment 3 Nate Graham 2020-07-09 20:14:42 UTC
No, on X11. I know this feature isn't available yet on Wayland.
Comment 4 Aleix Pol 2020-07-09 20:32:14 UTC
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.
Comment 5 Nate Graham 2020-07-09 20:45:02 UTC
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.
Comment 6 Nate Graham 2020-07-09 21:14:22 UTC
...Where by "disable focus stealing prevention" I mean "change focus stealing prevention from Low to None in the Window Behavior KCM".
Comment 7 David Edmundson 2020-07-09 21:52:03 UTC
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.
Comment 8 David Edmundson 2020-07-09 21:54:05 UTC
The potentially relevant kwin commit: a0c4a8e766a2160213838daf6f71c7ae6c3705df
Comment 9 Nate Graham 2020-07-09 22:29:55 UTC
Reverting a0c4a8e766a2160213838daf6f71c7ae6c3705df has no effect for me, BTW.
Comment 10 David Edmundson 2020-07-10 00:06:40 UTC
Then I very much doubt it's kwin at fault.
Comment 11 Nate Graham 2020-07-10 02:59:26 UTC
So then the problem is in ApplicationLauncherJob or Discover?

What exactly is the problem? Why does KWin think focus is being stolen?
Comment 12 Nate Graham 2020-09-19 00:09:00 UTC
*** Bug 426632 has been marked as a duplicate of this bug. ***
Comment 13 Nate Graham 2020-09-19 00:10:41 UTC
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.
Comment 14 Vlad Zahorodnii 2020-09-21 07:18:22 UTC
The problem is that discover doesn't provide any startup id. I doubt it's a kwin bug.
Comment 15 Vlad Zahorodnii 2020-09-21 07:32:04 UTC
Created attachment 131824 [details]
PoC

Could somebody please verify that this patch fixes the bug? Note that it's just a poc patch.
Comment 16 David Edmundson 2020-09-21 09:00:58 UTC
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.
Comment 17 Vlad Zahorodnii 2020-09-21 13:09:36 UTC
> but m_process->environment() is just an empty array.

as expected, KProcessRunner needs to call QProcess::setProcessEnvironment(), doesn't it?
Comment 18 David Edmundson 2020-09-21 13:14:44 UTC
Yes, but ideally we want that to only include the environment values that are worth exporting, not the entire thing that process currently has.
Comment 19 Nate Graham 2020-09-22 02:23:57 UTC
(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.
Comment 20 Aleix Pol 2020-10-07 00:04:07 UTC
@Nate maybe you could try Vlad's patch but adding the setter on the ::showDiscoverUpdates() method instead of ::showDiscover()?
Comment 21 Nate Graham 2020-10-07 14:47:44 UTC
No, that doesn't change anything for me.
Comment 22 Nate Graham 2020-11-24 15:31:58 UTC
*** Bug 429573 has been marked as a duplicate of this bug. ***
Comment 23 Nate Graham 2020-12-19 16:53:28 UTC
*** Bug 430561 has been marked as a duplicate of this bug. ***
Comment 24 Nate Graham 2020-12-19 16:53:30 UTC
*** Bug 430547 has been marked as a duplicate of this bug. ***
Comment 25 Nate Graham 2020-12-19 16:54:05 UTC
Dupes piling up; marking as VHI.
Comment 26 David Edmundson 2021-01-04 12:34:07 UTC
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.
Comment 27 David Edmundson 2021-01-04 14:39:46 UTC
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.
Comment 28 Nate Graham 2021-01-04 14:43:47 UTC
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?
Comment 29 David Edmundson 2021-01-04 15:06:10 UTC
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.
Comment 30 Bug Janitor Service 2021-01-04 15:58:37 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/discover/-/merge_requests/61
Comment 31 David Edmundson 2021-01-05 02:49:47 UTC
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