KCM/ComponentChooser Treat cases when there is no app for a usage
ClosedPublic

Authored by meven on Feb 14 2020, 10:51 AM.

Details

Summary

BUG: 417276

Test Plan

Have no email client installed.
In kcm componentchooser

  • select another browser
  • Save

Before:

  • Crash

After:

  • No crash

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D27384
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22474
Build 22492: arc lint + arc unit
meven created this revision.Feb 14 2020, 10:51 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 14 2020, 10:51 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Feb 14 2020, 10:51 AM
bport requested changes to this revision.Feb 14 2020, 11:08 AM
bport added inline comments.
kcms/componentchooser/componentchooserbrowser.cpp
107

add a space between - and 1

By the way this check seems strange to me, can you confirm it still allow to save custom browser ?

This revision now requires changes to proceed.Feb 14 2020, 11:08 AM
meven updated this revision to Diff 75679.Feb 14 2020, 11:39 AM
meven marked an inline comment as done.

Add a space

meven added inline comments.Feb 14 2020, 11:41 AM
kcms/componentchooser/componentchooserbrowser.cpp
107

Once the user has added browsers to the list, they appear as before last entries.
insertItem(count() -1, QIcon::fromTheme(icon), service->name(), service->storageId()) insert before count() - 1 the custom browser, line 58
The last entry is always "Other", whose index is count() - 1.
So this still allows custom browsers.

meven updated this revision to Diff 75691.Feb 14 2020, 2:26 PM

standardize how we check if the combobox has a value to save, allow to mark the value as changed when there was no value selected at the beginning

crossi added inline comments.Feb 14 2020, 2:43 PM
kcms/componentchooser/componentchooseremail.cpp
153

While you are here, can you had a check on this pointer. I don't understand why there is one below but not here.

kcms/componentchooser/componentchooserfilemanager.cpp
117 ↗(On Diff #75691)

I would have kept this test that is more clearer than if (currentIndex() == count() - 1) {} if you confirm it works the same.

meven updated this revision to Diff 75695.Feb 14 2020, 3:17 PM
meven marked an inline comment as done.

Clean a redundant check, ensure that if the user cancels selecting a custom app, the right entry is selected and hasChanged reflects the state correctly

meven added inline comments.Feb 14 2020, 4:54 PM
kcms/componentchooser/componentchooseremail.cpp
153

I would rather do the opposite the check below is for historical reasons from before my refactoring.

kcms/componentchooser/componentchooserfilemanager.cpp
117 ↗(On Diff #75691)

It would test twice the same thing : only the last value in the combobox can have an empty currendData.
I'd rather have the same combo accross the CfoPlugin.

meven added inline comments.Feb 14 2020, 5:19 PM
kcms/componentchooser/componentchooseremail.cpp
153

I meant to remove the check below (which I did).

looks ok to me

meven added a comment.Feb 14 2020, 6:05 PM

How about you @bport ?

I thought the bug was that we weren't checking the RC from KService::serviceByStorageId before using it

ervin added inline comments.Feb 17 2020, 11:24 AM
kcms/componentchooser/componentchooseremail.cpp
155

This actually feels less safe now. What if KService returned a nullptr for some reason?

I understand from the logic of early returns you introduced everywhere that you *expect* something. Still I wonder if that's really guaranteed by KService, we could expect bad config somewhere maybe?

kcms/componentchooser/componentchooserterminal.cpp
119

This comes back often in this patch and feels a bit cryptic, probably worth having a properly named method for it.

meven updated this revision to Diff 75920.Feb 18 2020, 2:38 PM
meven marked 2 inline comments as done.

Add a validLastCurrentIndex function, check emailClientService is not null before using it

meven marked an inline comment as done.Feb 18 2020, 2:39 PM
anthonyfieroni added inline comments.
kcms/componentchooser/componentchooser.h
51 ↗(On Diff #75920)

{ in new line, add const

meven updated this revision to Diff 75926.Feb 18 2020, 3:55 PM

Make validLastCurrentIndex const, newline before {

meven marked an inline comment as done.Feb 18 2020, 3:55 PM
bport accepted this revision.Feb 19 2020, 9:07 AM
This revision is now accepted and ready to land.Feb 19 2020, 9:07 AM

Is it fine for other reviewers ?

ervin accepted this revision.Feb 24 2020, 12:33 PM
meven marked an inline comment as done.Feb 24 2020, 2:08 PM

Is it fine for other reviewers ?

crossi accepted this revision.Feb 24 2020, 2:09 PM
This revision was automatically updated to reflect the committed changes.