Details

Reviewers

broulik
cfeck
elvisangelaccio
dfaure

Group Reviewers

Frameworks

Commits

R241:7bd7f38400b9: [KRun] when asked to open link in external browser, fall back to mimeapps.list…

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

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)
Set BrowserApplication[$e]= in ~/.config/kdeglobals
Ensure that ~/.config/mimeapps/list has a default browser set
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

default-browser-fallback-to-mimeapps-if-nothing-is-set-in-kdeglobals (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 6266
Build 6284: 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

Harbormaster completed remote builds in B5729: Diff 46913.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 mentioned this in D17372: [componentchooser KCM] Make KIO browser option the fallback only and remove from the UI.Dec 5 2018, 7:57 PM

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

ngraham mentioned this in D17727: Get default web browser from xdg-settings if it’s not set in KDE.Dec 22 2018, 2:55 PM

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

src/widgets/krun.cpp
1421–1431 ↗	(On Diff #46913)	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.

src/widgets/krun.cpp

1421–1431 ↗

(On Diff #46913)

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.

Reduce unnecessary nesting with an early return

Harbormaster completed remote builds in B6266: Diff 48021.Dec 22 2018, 5:53 PM

Reduce unnecessary nesting with an early return

Harbormaster completed remote builds in B6267: Diff 48022.Dec 22 2018, 5:55 PM

ngraham marked an inline comment as done.Dec 22 2018, 5:56 PM

rdieter added a subscriber: rdieter.Dec 22 2018, 6:47 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 ↗	(On Diff #46913)	no space after "!"

This revision is now accepted and ready to land.Dec 22 2018, 11:30 PM

Style fix

Harbormaster completed remote builds in B6276: Diff 48040.Dec 23 2018, 4:13 AM

Closed by commit R241:7bd7f38400b9: [KRun] when asked to open link in external browser, fall back to mimeapps.list… (authored by ngraham). · Explain WhyDec 23 2018, 4:14 AM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			src/core/copyjob.h (2 lines)
M			src/core/copyjob.cpp (14 lines)
M			src/core/global.h (3 lines)
M			src/core/job.cpp (44 lines)
M			src/core/slaveinterface.cpp (48 lines)
M			src/core/slaveinterface_p.h (19 lines)
M			src/widgets/krun.cpp (13 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	46913	020761d		Dec 5 2018, 7:51 PM	★	★
Diff 2	48021	e523ced	Reduce unnecessary nesting with an early return	Dec 22 2018, 5:53 PM	★	★
Diff 3	48022	4e2a815	Reduce unnecessary nesting with an early return	Dec 22 2018, 5:55 PM	★	★
Diff 4	48040	4e2a815	Style fix	Dec 23 2018, 4:13 AM	★	★
Diff 5	48041	4e2a815	R241:7bd7f38400b953a988a2cc942a518339e0b094a8	Dec 23 2018, 4:14 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
96404b870198	92d170a4f568	4e2a815b9a10	Nate Graham	[KRun] when asked to open link in external browser, ball back to mimeapps.list… (Show More…)	Dec 5 2018, 7:42 PM
4e2a815b9a10	295fa2de222e	ec31566e4cc7	Chinmoy Ranjan Pradhan	Refactor SlaveInterface::calcSpeed (Show More…)	Dec 12 2018, 9:07 AM
ec31566e4cc7	5381181be07a	a48a277a8296	David Edmundson	Add error value for job owner dying (Show More…)	Dec 21 2018, 1:38 PM
a48a277a8296	a163f15ff1dd	a4905c5994a7	Kai Uwe Broulik	Cache translated strings in Job::emit functions (Show More…)	Dec 20 2018, 7:55 AM
a4905c5994a7	53cf4ef5e9c1	1d8e4e6fa29c	Kai Uwe Broulik	Port connect of subjob in copyNextFile to new connect syntax (Show More…)	Dec 20 2018, 7:53 AM
1d8e4e6fa29c	256ec90f87ae	e523cedb0748	Kai Uwe Broulik	Use qobject_cast instead of dynamic_cast (Show More…)	Dec 19 2018, 9:57 AM

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

Details

Diff Detail

Revision Contents
Changeset List

Diff 48021

src/core/copyjob.h

src/core/copyjob.cpp

src/core/global.h

src/core/job.cpp

src/core/slaveinterface.cpp

src/core/slaveinterface_p.h

src/widgets/krun.cpp

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

Details

Diff Detail

Revision ContentsChangeset List

Diff 48021

src/core/copyjob.h

src/core/copyjob.cpp

src/core/global.h

src/core/job.cpp

src/core/slaveinterface.cpp

src/core/slaveinterface_p.h

src/widgets/krun.cpp

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

Revision Contents
Changeset List