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

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

Details

Reviewers
bport
ervin
crossi
davidedmundson
Group Reviewers
Plasma
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 22490
Build 22508: arc lint + arc unit
meven created this revision.Fri, Feb 14, 10:51 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Feb 14, 10:51 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Fri, Feb 14, 10:51 AM
bport requested changes to this revision.Fri, Feb 14, 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.Fri, Feb 14, 11:08 AM
meven updated this revision to Diff 75679.Fri, Feb 14, 11:39 AM
meven marked an inline comment as done.

Add a space

meven added inline comments.Fri, Feb 14, 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.Fri, Feb 14, 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.Fri, Feb 14, 2:43 PM
kcms/componentchooser/componentchooseremail.cpp
154

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

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.Fri, Feb 14, 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.Fri, Feb 14, 4:54 PM
kcms/componentchooser/componentchooseremail.cpp
154

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

kcms/componentchooser/componentchooserfilemanager.cpp
117

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.Fri, Feb 14, 5:19 PM
kcms/componentchooser/componentchooseremail.cpp
154

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

looks ok to me

meven added a comment.Fri, Feb 14, 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