Give the PlasmaComponents3 TextField the ability to have a Clear button
AbandonedPublic

Authored by ngraham on Jul 24 2018, 11:06 PM.

Details

Reviewers
mart
davidedmundson
Group Reviewers
Plasma
Summary

The PlasmaComponents2 TextField can draw its own Clear button, but the PlasmaComponents3 version can't, which impedes our ability to port code to use it without losing features.

This patch implements the same built-in Clear button in the PlasmaComponents3 TextField (code is basically a copy-paste) so that it will be a drop-in replacement for the PlasmaComponents2 version for text fields that need a clear button.

BUG: 396828
CCBUG: 396813
FIXED-IN: 5.49

Test Plan

Tested with KRunner patched locally in RunCommand.qml to use the TextField from PlasmaComponents3 rather than PlasmaComponents2. Results:

Placeholder text is now visible (See https://bugs.kde.org/show_bug.cgi?id=396813):

There's still a clear button when you type some text (and it still works :)):

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
pc3-textfield-clear-button (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1141
Build 1154: arc lint + arc unit
ngraham created this revision.Jul 24 2018, 11:06 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 24 2018, 11:06 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.Jul 24 2018, 11:06 PM
ngraham edited the summary of this revision. (Show Details)Jul 24 2018, 11:09 PM
ngraham edited the test plan for this revision. (Show Details)
davidedmundson requested changes to this revision.Jul 24 2018, 11:21 PM

PC3 was previously designed to be a drop in QQC2 implementation. There is currently no new API.
It /might/ have changed after we made plasmastyle instead. Check with Marco.

If we were to do this (as a subclass with a new name or whatever) we shouldn't have the text overlap the button that this will have.

This revision now requires changes to proceed.Jul 24 2018, 11:21 PM

PC3 was previously designed to be a drop in QQC2 implementation. There is currently no new API.
It /might/ have changed after we made plasmastyle instead. Check with Marco.

Hmm, the PC3 version doesn't seem to be a drop-in replacement right now, as it lacks the ability to display a clear button or a little eye toggle (when used as a password field). If I try to use the PC3 one in KRunner, for example, I get the following:

"file:///usr/share/plasma/look-and-feel/org.kde.breeze.desktop/contents/runcommand/RunCommand.qml" 
 "Error loading QML file.\n70: Cannot assign to non-existent property \"clearButtonShown\"\n"

If we were to do this (as a subclass with a new name or whatever) we shouldn't have the text overlap the button that this will have.

Ah, good point.

Oh. Maybe I don't know the history here. Why do we have PC2 and PC3 versions of this control, with the PC2 version using QQC1, and the PC3 version using QQC2, but not having all the features of the PC2 one? And why can't we change the PC3 version at all?

Story time!

First we had PlasmaComponents (based on some Nokia crap and some custom things from late Plasma4)


Then came along Plasma 5

PC2 which was a continuation of above


Then QQC1 came along
plasmastyle which was a QQC1 theme and you could write QQC1 and style as this.

PC2 was ported to wrap Plasmastyle but keep it's old API compatibility with some stuff removed, some stuff extending QQC1.


Then QQC2 came along

You don't theme items you implement them with the template magic.

PC3 is a QQC2 implementation.
If one wrote code for QQC2 it'd use the PC3 code.
It was made an import as at this point in time QQC2 didn't have the style support it does now.
Now it's also co-installed to /QtQuick/Controls.2/Plasma exposed as a style
With some intention of dropping the PC3 import in the future.


Then came along kirigami

which creates new templates for widget addons which then use the underlying QQC2 style.

Thanks for the history! Two questions:

  1. Does this mean that PlasmaComponents is semi or fully deprecated or "legacy", and we should be porting Plasma stuff to Kirigami instead?
  2. Since there's no Kirigami TextField, what do we do with this patch? Is there any reason why we can't improve the PC3 TextField to match the features that the PC2 TextField has? Or should we create a Kirigami TextField instead and port current PC2/3 clients to use it?

Great! Now I know why the port to PC3 didn't start :D
Now to the patch. First, by this we will allow the user to get text under the icon, which will be impossible to read.
Second is what I'm here for :)
The clear button placement and icon should not follow the locale, but the actual text imh. You can write Arabic in an English system, and vice versa.

I've made this simple file to explain my point:

import QtQuick 2.7
import QtQuick.Controls 2.3
import QtQuick.Layouts 1.3

import org.kde.plasma.core 2.0 as PlasmaCore

Item {
    width: 300
    height: 200

    LayoutMirroring.enabled: true // This value (true or false) shouldn’t affect anything regarding the Clear button!
    LayoutMirroring.childrenInherit: true

    Frame {
        anchors.centerIn: parent
        width: 200
        RowLayout {
            LayoutMirroring.enabled: false
            anchors.fill: parent
            layoutDirection: textInput.isRightToLeft(0, textInput.length) ? Qt.RightToLeft : Qt.LeftToRight
            TextField {
                id: textInput
                Layout.fillWidth: true
            }
            AbstractButton {
                contentItem: PlasmaCore.IconItem {
                    source: textInput.isRightToLeft(0, textInput.length) ? "edit-clear-locationbar-ltr" : "edit-clear-locationbar-rtl"
                }
            }
        }
    }
}

Whatever the locale is, or the mirroring is, things inside the Frame shouldn't change (well, this won't work fully as the context menu might not get rendered correctly, but I didn't want to write 4 if statments in the rightMargin and leftMargin properties, as you can't set it to be absolute. With mirroring, right is treated as left, and vice versa)

Results:

safaalfulaij added a comment.EditedJul 29 2018, 11:03 AM
  1. Does this mean that PlasmaComponents is semi or fully deprecated or "legacy", and we should be porting Plasma stuff to Kirigami instead?
  2. Since there's no Kirigami TextField, what do we do with this patch? Is there any reason why we can't improve the PC3 TextField to match the features that the PC2 TextField has? Or should we create a Kirigami TextField instead and port current PC2/3 clients to use it?

https://cgit.kde.org/kirigami.git/tree/src/controls/Label.qml#n45

I'll take the liberty and say that we just need to use QQC2 directly, and it will be themed correctly with all the enviroment variable tricks in Plasma :)

EDIT: While doing some tests in the lookscreen component, it seems that QQC2-based labels are black, while they should be white (as PC2). I have no idea what is going on.

I feel like the whole point of having these Plasma-ish wrappers is because they have extra features not present in the base Qt classes. If we just use QQC2 TextFields everywhere, then every one that wants a clear button (for example) needs to implement it for itself. That doesn't seem desirable.

@davidedmundson ping:

Thanks for the history! Two questions:

  1. Does this mean that PlasmaComponents is semi or fully deprecated or "legacy", and we should be porting Plasma stuff to Kirigami instead?
  2. Since there's no Kirigami TextField, what do we do with this patch? Is there any reason why we can't improve the PC3 TextField to match the features that the PC2 TextField has? Or should we create a Kirigami TextField instead and port current PC2/3 clients to use it?

So from in-person conversations this week, it seems like we have a few paths forward here:

  • Upstream the features we wrote into our PlasmaComponents TextField and then just use that. Downsides: lengthy process, will take forever before we can actually use it here.
  • Finally fix the Qt bug that prevents QQC1 and PC2 components with text from working properly. Downsides: we already tried this and accidentally made it worse. Seems tricky.
  • Work around the text rendering issue in the QQC1 style as well so that all QQC1 and PC2 components with text look fine with no other modifications. Downsides: maybe none? Am I not seeing them? @davidedmundson?

Upstream the features we wrote into our PlasmaComponents TextField and then just use that. Downsides: lengthy process, will take forever before we can actually use it here.

Yes, but there's also no real rush from a PC POV.

Finally fix the Qt bug that prevents QQC1 and PC2 components with text from working properly
Work around the text rendering issue in the QQC1 style as well so that all QQC1 and PC2

That's unrelated to this patch.
QQC1 desktop theme has that bug,
PC2 (despite being QQC1) does not.

So I'm not sure what we actually fix?

To back up a bit, the issue is that we currently have no TextField with has the following characteristics:

  1. Has built-in functionality for clear and password reveal buttons, so each client doesn't need to re-invent the wheel (PC2 one has this, PC3, QQC1, and QQC2 ones do not)
  2. Placeholder text looks good with fractional scale factors (PC3 and QQC2 ones have this, PC2 and QQC1 ones do not)

My approach with this patch was to try to port the missing features to the PC3 TextField to solve #1, but maybe that's not the right approach? Do you have any suggestions for how we resolve the issue?

Placeholder text looks good with fractional scale factors (PC3 and QQC2 ones have this, PC2 and QQC1 ones do not)

No, this is where we have crossed wires.

PC3, QQC2 and PC2 look good. We control the renderType.
Only QQC1 desktop theme has the font issue.

Ah, I'm sorry. I'm getting my patches confused. The PC2 TextField's bug is that it doesn't show placeholder text properly with light themes when software rendering isn't being used. I'm open to other ways of fixing that.

ngraham abandoned this revision.Aug 23 2018, 1:01 PM

Don't need this; not the right approach. The original motivation for doing this (to work around the broken placeholder text in the PC2 TextField) is being fixed in the PC2 TextField with D15021 and upstream with https://bugreports.qt.io/browse/QTBUG-70138