KCM/Component Revamp email config
ClosedPublic

Authored by meven on Jan 10 2020, 4:59 PM.

Details

Summary

Simplify UI:

  • Select option in the available email clients
  • use mimeapps.list preferred service as source of truth regarding default email client
  • keep kmail as default email client when it is installed
  • Can select any service as email client alternatively
  • Ensure kbuildsycoca cache is updated after saving

BUG: 292606
FIXED-IN: 5.19

Test Plan

Before:

After:

Diff Detail

Repository
R119 Plasma Desktop
Branch
email-config
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21260
Build 21278: arc lint + arc unit
meven created this revision.Jan 10 2020, 4:59 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 10 2020, 4:59 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Jan 10 2020, 4:59 PM
meven edited the summary of this revision. (Show Details)Jan 10 2020, 5:01 PM
meven edited the test plan for this revision. (Show Details)
meven added reviewers: bport, crossi.
meven updated this revision to Diff 73212.Jan 10 2020, 5:04 PM

minor change

Very nice, I think this is the way to go, not only for this component but for the others too. We should drop the group box too because with only one item, there's no longer anything to group! :) We can just switch it out for a label to the left of the combobox.

meven added a comment.Jan 10 2020, 6:07 PM

Very nice, I think this is the way to go, not only for this component but for the others too. We should drop the group box too because with only one item, there's no longer anything to group! :) We can just switch it out for a label to the left of the combobox.

Do you mean, we should drop command line text fields, like we have in the terminal and browser section ? Only comboxbox ?
I would be encline to do so.
That would mean we could regroup everything in one tab also just like other DE do.

Do you mean, we should drop command line text fields, like we have in the terminal and browser section ? Only comboxbox ?

We don't want to lose this feature, but the text field that lets you choose a command doesn't need to be always visible, either.

The combobox could have an entry entitled "Terminal Program" and selecting that option would make the text field allowing you to enter a command appear next to the combobox.

That would mean we could regroup everything in one tab also just like other DE do.

Exactly. Then we don't need tabs at all.

meven updated this revision to Diff 73375.Jan 13 2020, 8:45 AM

Clean reference to unused and not existant .config/emails directory

meven added a comment.Jan 13 2020, 9:04 AM

Do you mean, we should drop command line text fields, like we have in the terminal and browser section ? Only comboxbox ?

We don't want to lose this feature, but the text field that lets you choose a command doesn't need to be always visible, either.

It means we will need to write .desktop file on the fly which KOpenWithDialog can do.
Do we want the previously entered commands to appear in the list ?

The combobox could have an entry entitled "Terminal Program" and selecting that option would make the text field allowing you to enter a command appear next to the combobox.

Or open a KOpenWithDialog window.

That would mean we could regroup everything in one tab also just like other DE do.

Exactly. Then we don't need tabs at all.

I will probably do this incrementaly, refactor UIs in all tabs, then get rid of tabs, because this will move quite a bit of code.

It means we will need to write .desktop file on the fly which KOpenWithDialog can do.
Do we want the previously entered commands to appear in the list ?

I think so. We don't want to break anyone who's currently using this feature.

The combobox could have an entry entitled "Terminal Program" and selecting that option would make the text field allowing you to enter a command appear next to the combobox.

Or open a KOpenWithDialog window.

Yeah that might be better.

That would mean we could regroup everything in one tab also just like other DE do.

Exactly. Then we don't need tabs at all.

I will probably do this incrementaly, refactor UIs in all tabs, then get rid of tabs, because this will move quite a bit of code.

Sounds like a plan! Thanks a bunch for tackling this.

meven updated this revision to Diff 73547.Jan 14 2020, 4:26 PM

Allow to select any app as email client, mimeapps.list preferred email is the source of truth regarding default mail client, depends on KF5.67 (post plasma 5.18)

meven edited the test plan for this revision. (Show Details)Jan 14 2020, 4:27 PM
meven edited the test plan for this revision. (Show Details)
meven updated this revision to Diff 73548.Jan 14 2020, 4:33 PM

clean some unused incudes

meven edited the summary of this revision. (Show Details)Jan 14 2020, 4:42 PM
meven edited the test plan for this revision. (Show Details)

Let me know if and when this is reviewable, or if it's still WIP.

meven edited the summary of this revision. (Show Details)Jan 15 2020, 8:03 AM
meven added a reviewer: dvratil.
meven added a comment.Jan 15 2020, 8:11 AM

Let me know if and when this is reviewable, or if it's still WIP.

This is good for review.

I am now working towards preventing a potential issue :
If a user set say thunderbird as default email client, (it will show up now in componentchooser) but KToolInvocation::invokeMailer will keep using the mail client set in emaildefaults.
KToolInvocation::invokeMailer needs to use the parameter set in mimeapps.list.
And fixing https://bugs.kde.org/show_bug.cgi?id=416257
KService would need to be extended to better handle default scheme-handler, currently it does not follow specs : it does not use the "Default Application" section of mimeapps.list at all.

meven updated this revision to Diff 73695.Jan 16 2020, 1:12 PM

Add a couple of const

Hmm, I don't have KMail installed yet it shows up in the combobox anyway. I don't think that makes sense.

meven added a comment.Jan 16 2020, 4:33 PM

Hmm, I don't have KMail installed yet it shows up in the combobox anyway. I don't think that makes sense.

Everything in the combobox comes from /usr/share/applications/*desktop (or similar path) or mimeapps.list's "Added Applications"

So Are you sure ?
Does it have an icon ?
Do you have a /usr/share/applications/org.kde.kmail2.desktop file ?
Is kmail mentioned in ~/.config/mimeapps.list ?

ngraham accepted this revision.Jan 16 2020, 5:10 PM

I'm so sorry, this was my mistake. Somehow KMail got re-installed without my knowledge. Probably https://bugzilla.suse.com/show_bug.cgi?id=1035802 biting me again.

Once I've removed it again, everything works perfectly.

This revision is now accepted and ready to land.Jan 16 2020, 5:10 PM
meven added inline comments.Jan 17 2020, 8:31 AM
kcms/componentchooser/componentchooseremail.cpp
144

remove

meven updated this revision to Diff 73747.Jan 17 2020, 8:32 AM
meven marked an inline comment as done.

Remove a bad line

ervin added inline comments.Jan 20 2020, 12:50 PM
kcms/componentchooser/componentchooseremail.cpp
67

Not a huge fan of static consts appearing like this in the middle of the file. Could you please move them up before the ctor? Bonus point for putting them in the anonymous namespace.

106

To simplify reading this I'd go for:

if (!service) {
    continue;
}

and then I'd put the result of none_of in a properly named intermediate variable before checking it in its own if. Will make the intents clearer.

meven updated this revision to Diff 73948.Jan 20 2020, 3:20 PM
meven marked 2 inline comments as done.

Move static variable to a proper namespace block, refactor some code for clarity, fix selecting a newly selected email client

ervin accepted this revision.Jan 20 2020, 3:50 PM
ervin added inline comments.
kcms/componentchooser/componentchooseremail.cpp
106

I really meant if (!service) + continue to avoid yet another indentation level but OK, fair enough.

meven updated this revision to Diff 73957.Jan 20 2020, 3:59 PM

Avoid indentation level

meven marked an inline comment as done.Jan 20 2020, 3:59 PM
ervin accepted this revision.Jan 20 2020, 4:04 PM
This revision was automatically updated to reflect the committed changes.