[componentchooser KCM] Make KIO browser option the fallback only and remove from the UI
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Plasma
VDG
Summary

Right now the componentchooser KCM's browser module has a really confusing setting ("In an application based on the contents of the url") that's the default. There are ancient Buzailla tickets about this being confusing, and some users report that it actually does the wrong thing for certain URLs.

This patch removes the UI for this feature and, in conjunction with D17371, makes it a hidden fallback-only behavior for the rare case that there aren't any web browsers installed.

BUG: 100016
FIXED-IN: 5.15.0

Test Plan

Here's what it looks like now:

Verifications performed:

  • There are no actual settings or behavioral changes for any existing users, since browsers ask to set themselves as the default, and distros ship config files already specifying their preferred browser.
  • Changing the browser in the combobox correctly updates both ~/.config/kdeglobals and ~/.configmimeapps.list.
  • The combobox's default setting is now correctly identical to what ~/.config/mimeapps.list specifies, if anything.

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D17372
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6508
Build 6526: arc lint + arc unit
ngraham created this revision.Dec 5 2018, 7:57 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 5 2018, 7:57 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Dec 5 2018, 7:57 PM
ngraham edited the summary of this revision. (Show Details)Dec 5 2018, 7:58 PM
ngraham edited the test plan for this revision. (Show Details)

and some users report that it actually does the wrong thing for certain URLs.

Bug reports are also an indication people use it.

I don't see a compelling reason to remove a feature.

and some users report that it actually does the wrong thing for certain URLs.

Bug reports are also an indication people use it.

Well, it's the default, so I think that goes without saying. :)

The reason for removing it is because it's not what people expect in the year 2018. When you open an HTTP or HTTPS link, you expect it to open in your browser, full stop.

abetts added a subscriber: abetts.Dec 11 2018, 3:51 PM

Can the section title be called something different instead of using the word "Component"?

dfaure added a subscriber: dfaure.Dec 22 2018, 11:24 PM

OK, here's a bit of historical information, and then my opinion.

The idea of that option was that when you click on a (e.g. HTTP) link to a PDF (e.g. in an email), the best application to view that PDF is Okular.
Similarly, if it's an image, the best application would be an image viewer.
Sure, those aren't browsers, that's the whole point of that option.
When the associated application is using KIO, there is no speed penalty because the job (and the kioslave) used to retrieve the URL is put on hold in the first app (e.g. kmail) and resumed in the application being launched.
I don't think this is confusing in any way (only be people who nitpick on the word "browser" in the section name), but that's a minor issue at this point, see below.

This used to be fine, at least with KDE applications. But that means a double HTTP request with non-KDE applications.
And these days there's no KIO-based browser anymore, so for the common case (HTML) it doesn't work anymore.

I agree to removing the option, then.
Even I don't use it anymore, even though it was one of my favourite features...
I'm just sad that clicking on a link to a PDF in kmail doesn't open okular but firefox's awful embedded PDF reader. But we can't trust extensions over HTTP, so there's not really any solution there. Thank you Apple, the killing of QtWebKit has killed support for this, indirectly.

If we are going to effectively remove it, I would like to see it removed fully.

Otherwise we'll get some crufty code that doesn't make sense forever as well as quirky bugs.

kcms/componentchooser/componentchooserbrowser.cpp
69

m_browserService is initialised after this.

It won't set the correct index.

ngraham edited the test plan for this revision. (Show Details)Dec 24 2018, 3:45 PM
ngraham updated this revision to Diff 48456.Dec 31 2018, 5:41 PM

Initialize m_browserCombo before accessing it

ngraham marked an inline comment as done.Dec 31 2018, 5:41 PM
ngraham updated this revision to Diff 48457.Dec 31 2018, 5:42 PM

Revert unintentional and unnecessary whitespace change

If folks really want, I can have a look at removing the feature entirely. However are we sure we don't want to keep it around as a fallback? It doesn't necessarily seem like a bad backup, it's just maybe not appropriate as a user-visible setting anymore in 2018.