Differential D25462

[Colors / Desktop Theme KCM] Use Kirigami.SearchField
ClosedPublic
Actions

Authored by broulik on Nov 22 2019, 1:32 PM.

Details

Reviewers
ngraham
Group Reviewers
Plasma
VDG
Commits
R119:c743d36b0372: [Colors / Desktop Theme KCM] Use Kirigami.SearchField
Summary

Gets rid of some code duplication.

Test Plan
  • Filtering works
  • Clear button works

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Nov 22 2019, 1:32 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 22 2019, 1:32 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Nov 22 2019, 1:32 PM
aspotashev added a subscriber: aspotashev.Nov 22 2019, 2:17 PM

The translation part of SearchField is indeed buggy: https://bugs.kde.org/show_bug.cgi?id=413900

May be it's not time to adopt it yet?

ngraham accepted this revision.Nov 22 2019, 2:29 PM
ngraham added a subscriber: ngraham.
In D25462#566149, @aspotashev wrote:

The translation part of SearchField is indeed buggy: https://bugs.kde.org/show_bug.cgi?id=413900

May be it's not time to adopt it yet?

IMO, more like time to fix that bug. :) In the worst-case scenario, you can override placeholderText here and provide a translated string.

However there is a deeper problem: now that System Settings uses a SearchField in its own toolbar, the Ctrl+F shortcut to trigger it becomes ambiguous. I guess we need to fix that too. However I see that this is already the case even without this patch, so there's no regression here at least.

Therefore, +1.

This revision is now accepted and ready to land.Nov 22 2019, 2:29 PM
aspotashev added a comment.Nov 22 2019, 2:34 PM
In D25462#566157, @ngraham wrote:

In the worst-case scenario, you can override placeholderText here and provide a translated string.

Ah, right. Thanks for the reminder!

How about including the workaround in this patch?

broulik planned changes to this revision.Dec 5 2019, 9:10 PM

I'd prefer if the translation were fixed, but I can override placeholderText

broulik updated this revision to Diff 78891.Mar 30 2020, 12:56 PM
broulik retitled this revision from [Colors KCM] Use Kirigami.SearchField to [Colors / Desktop Theme KCM] Use Kirigami.SearchField.
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
  • Add i18n to work around translation loading bug
  • Also port desktoptheme KCM which has a copy of this
This revision is now accepted and ready to land.Mar 30 2020, 12:56 PM
broulik requested review of this revision.Mar 30 2020, 12:56 PM
ngraham accepted this revision.Mar 30 2020, 3:49 PM
This revision is now accepted and ready to land.Mar 30 2020, 3:49 PM
broulik updated this revision to Diff 83044.May 18 2020, 3:18 PM
broulik edited the summary of this revision. (Show Details)
  • Drop placeholderText since the Kirigami issue has been resolved
ngraham accepted this revision.May 18 2020, 4:14 PM
Closed by commit R119:c743d36b0372: [Colors / Desktop Theme KCM] Use Kirigami.SearchField (authored by broulik). · Explain WhyMay 21 2020, 9:17 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 83100

View Options

kcms/colors/package/contents/ui/main.qml

Loading...
View Options

kcms/desktoptheme/package/contents/ui/main.qml

Loading...