[KRun] when asked to open link in external browser, fall back to mimeapps.list if nothing is set in kdeglobals
ClosedPublic

Authored by ngraham on Dec 5 2018, 7:51 PM.

Details

Summary

Right now, when KRun is invoked to open an http or https link in a browser, it checks the BrowserApplication key in ~/.config/kdeglobals. If nothing is set there (which is the default), then it introspects the link and figures out for itself what app to open, which is slow and can cause problems with certain links (see CCBUGs below).

This patch improves the browser discovery logic by additionally looking for a default browser in ~/.config/mimeapps.list, which is the XDG file and it's where browsers set themselves as the default. So if there is a default browser set in there, KRun will consume that information immediately instead of doing the time-consuming and possibly error-inducing link introspection round-trip.

CCBUG: 347870
CCBUG: 100016

Test Plan
  1. Open System Settings > Applications > Default Applications > Browser and click "In an application based on the contents of the url" (which is the default setting, but you might have changed it)
  2. Set BrowserApplication[$e]= in ~/.config/kdeglobals
  3. Ensure that ~/.config/mimeapps/list has a default browser set
  4. Open any KDE app > Help menu > About KDE > Click on one of the links in the dialog

Without this patch, a KRun job is spawned that shows up in the notification widget and the link may take a second or two to open in your default browser.

With this patch, the link instantly opens in the browser.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D17371
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6276
Build 6294: arc lint + arc unit
ngraham created this revision.Dec 5 2018, 7:51 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 5 2018, 7:51 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Dec 5 2018, 7:51 PM
ngraham edited the summary of this revision. (Show Details)Dec 5 2018, 7:53 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham retitled this revision from [KRun] when asked to open link in external browser, ball back to mimeapps.list if nothing is set in kdeglobals to [KRun] when asked to open link in external browser, fall back to mimeapps.list if nothing is set in kdeglobals.Dec 22 2018, 2:53 PM
ngraham added reviewers: cfeck, elvisangelaccio.

Can I get a review on this (or another version someone else submitted: D17727)? Frameworks people? Dolphin? Anyone?

I think it’s better than what I proposed (without knowing about yours) in D17727.

src/widgets/krun.cpp
1421–1431

I personally would have gone for

// If a default browser isn't set in kdeglobals, fall back to mimeapps.list
if (!d->m_externalBrowser.isEmpty()) {
    return;
}
KSharedConfig::Ptr profile = KSharedConfig::openConfig(QStringLiteral("mimeapps.list"), KConfig::NoGlobals, QStandardPaths::GenericConfigLocation);
KConfigGroup defaultApps(profile, "Default Applications");

d->m_externalBrowser = defaultApps.readEntry("x-scheme-handler/https");
if (d->m_externalBrowser.isEmpty()) {
    d->m_externalBrowser = defaultApps.readEntry("x-scheme-handler/http");
}

to avoid too much nesting.

ngraham updated this revision to Diff 48021.Dec 22 2018, 5:53 PM

Reduce unnecessary nesting with an early return

ngraham updated this revision to Diff 48022.Dec 22 2018, 5:55 PM

Reduce unnecessary nesting with an early return

ngraham marked an inline comment as done.Dec 22 2018, 5:56 PM
dfaure accepted this revision.Dec 22 2018, 11:30 PM
dfaure added a subscriber: dfaure.
dfaure added inline comments.
src/widgets/krun.cpp
1423

no space after "!"

This revision is now accepted and ready to land.Dec 22 2018, 11:30 PM
ngraham updated this revision to Diff 48040.Dec 23 2018, 4:13 AM
ngraham marked an inline comment as done.

Style fix

This revision was automatically updated to reflect the committed changes.