fontinst quits after KJob is done
ClosedPublic

Authored by mathiastillman on Mon, Jul 30, 5:13 PM.

Details

Summary

As the summary says, when installing/removing multiple fonts to system the expected behaviour would be for fontinst to keep running after the first font has been installed or removed, instead it quits which causes a bunch of issues. fontinst uses KJob to authorize and internally KJob uses a QEventLoopLocker which causes the main event loop to quit when it's done.
I'm not entirely sure why the event loop locker is enabled by default for KJob, but the patch I have attached works around this by completely disabling that functionality for fontinst. There's a timer that runs in the background which checks for any connected clients, so it will quit after a little while regardless.

BUG: 379524
BUG: 379324
BUG: 349673
BUG: 361960
BUG: 392267

Test Plan

Make sure fonts are still installed and removed properly.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
mathiastillman created this revision.Mon, Jul 30, 5:13 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMon, Jul 30, 5:13 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mathiastillman requested review of this revision.Mon, Jul 30, 5:13 PM
mathiastillman edited the summary of this revision. (Show Details)Tue, Jul 31, 6:56 AM

It does indeed fix both of those bugs. I have added that in the summary now, sorry about that.

wbauer added a subscriber: wbauer.Tue, Jul 31, 7:17 AM

I can confirm that this patch does fix the problem with installing several fonts at once systemwide here. And fontinst still exits after some time of idleness.

Btw, there's also BUG:349673 about the same problem.

mathiastillman edited the summary of this revision. (Show Details)Tue, Jul 31, 7:22 AM
mathiastillman edited the summary of this revision. (Show Details)Tue, Jul 31, 7:32 AM

The idea of KJob reference-counting the event loop is that if you quit the application while a kjob is executing, then the process will wait for the job to finish before quitting. (Use case back then, konqueror and a HTTP download).
The reason why we don't just quit after the first job is that KMainWindow itself uses QEventLoopLocker too, increasing the refcount as long as there are visible windows.

If kfontinst has no way to quit while a job is happening, this patch is ok.
But maybe it would be better to create a QEventLoopLocker in whichever GUI class kfontinst creates?

Okay, that makes sense. kfontinst doesn't actually display a UI at all, instead it just receives dbus commands from kcm - so there is no real GUI class, it just uses QCoreApplication. I could add a QEventLoopLocker to KFI::FontInst though, but, from reading the code it seems as though there's no way to actually cancel the running kjob (nor the running fontinst). That might be a cleaner approach than completely disabling the quit lock, however.

Use a QEventLoopLocker instead of completely disabling the quit lock functionality.

Ah so it's kind of a daemon, that should keep running until logout? I wasn't aware of that, I thought it was a KCM. Disabling as in the first patch made sense then ;)

Either version is fine with me.

kcms/kfontinst/dbus/FontInst.h
161

"which would otherwise cause fontinst...."

Don't describe the past as the present, for future readers of this code ;)

Well, it's sort of a daemon, but it doesn't run until the user logs out - there's a built-in timeout of 30 seconds that will check for any connected clients and if none are found it will kill itself. The KCM communicates with the fontinst daemon via dbus :)

Grammatical fixes ;)

dfaure accepted this revision.Tue, Jul 31, 9:58 AM

With your explanations, I actually have a better feeling about the first patch after all (if a job is stuck for more than 30s, then the process will never exit).
But really, I'm nitpicking. These jobs don't get stuck ;)

This revision is now accepted and ready to land.Tue, Jul 31, 9:58 AM
mathiastillman marked an inline comment as done.Tue, Jul 31, 10:01 AM

Either one is fine by me as well, so pick whichever one you fancy :)

Please push the first patch ;)

Wouldn't it make sense to push this to the 5.12 and 5.13 branches as well?
After all, there are quite a lot of bug reports about this...

Should be answered by some Plasma developer though, I suppose. ;-)

It would indeed, but I will wait for input before pushing it as I'm not entirely sure how it's decided what release a patch goes in to.

Generally speaking, bugfixes go into the stable branches. If the bugfix is particularly large, complex, or risky, it might be worth it to commit to master only. For this one, I think it's okay to commit it to the Plasma/5.12 branch too.

The preferred workflow here is to commit to the earliest branch and then merge; so that would mean committing to the Plasma/5.12 branch, and then merging the Plasma/5.12 into the Plasma/5.13 branch, and then merging the Plasma/5.13 branch into master.

However since you already committed it to master, that's not an option, and it will need to be cherry-picked to Plasma/5.12, and then Plasma/5.12 merged into Plasma/5.13, with no final merge to master.

But I'll let one of the more experienced plasma developers have the final say in this matter.

Okay, so should I wait for someone else to reply before committing it to the 5.12 and 5.13 branches?

aacid added a comment.Wed, Aug 1, 11:36 PM

you still need to merge 5.13 to master because eventually someone will, it's the trouble with cherry-picking.

I'd suggest you use -x so it adds a line about it being a cherry-pick