sebas accepted this revision.
sebas added a reviewer: sebas.
sebas added a comment.
This revision is now accepted and ready to land.
a few niggles, but generally, look good to me.
INLINE COMMENTS
> outputmanagement.cpp:94
> {
> + Q_UNUSED(parent);
> OutputConfiguration *config = new OutputConfiguration(this);
Seems unrelated (not a problem, unless I'm missing something)
> plasmawindowmanagement.cpp:128
> windows.removeAll(window);
> + if (activeWindow == window) {
> + activeWindow = nullptr;
Seems unrelated (not a problem, unless I'm missing something)
> plasmawindowmanagement.cpp:357
> Private *p = cast(data);
> - if (p->desktop == number) {
> + if (p->desktop == static_cast<quint32>(number)) {
> return;
Seems unrelated (not a problem, unless I'm missing something)
> plasmawindowmodel.cpp:60
>
> - QObject::connect(window, &PlasmaWindow::unmapped,
> - [window, this] {
> - const int row = windows.indexOf(window);
> - if (row != -1) {
> - q->beginRemoveRows(QModelIndex(), row, row);
> - windows.removeAt(row);
> - q->endRemoveRows();
> - }
> + auto removeWindow = [window, this] {
> + const int row = windows.indexOf(window);
the changes in this file are unrelated, but do make sense (I don't care much if
you push them together)
> plasma-window-management.xml:20
>
> - <interface name="org_kde_plasma_window_management" version="3">
> + <interface name="org_kde_plasma_window_management" version="4">
> <description summary="application windows management">
changes here are also unrelated?
> text-input-unstable-v2.xml:153
> + <entry name="phone" value="4" summary="input a phone number"/>
> + <entry name="url" value="5" summary="input an URL"/>
> + <entry name="email" value="6" summary="input an email address"/>
"a URL"
> registry.h:451
> + *
> + * Prefer using createTextInputManagerUnstableV0 instead.
> + * @see createTextInputManagerUnstableV0
A small note about the protocol (i.e. which version of the interface should be
used) probably wouldn't hurt (as the two creators may end up being confusing).
This is a bit high-level though, for APIDOCs, but still raises this question.
> textinput.h:49
> + *
> + * The TextInput allows to have text composed by the Compositor and be sent
> to
> + * the client.
move this above the previous paragraph, more logical order (general purpose,
then implementation caveats)
> textinput.h:81
> + /**
> + * Enable text input in a @p surface (usually when a text entry inside
> of it has focus).
> + *
Does this actually show the panel then? (Could be more clear what's expected
here, what's the relationship between enable/disable and show*/hide*)
> textinput.h:157
> + */
> + Lowercase = 1 << 3,
> + /**
I think LowerCase, UpperCase and TitleCase feel more in line with the other
fields here?
> textinput_interface.h:134
> + */
> + Lowercase = 1 << 3,
> + /**
LowerCase, UpperCase, etc. as well?
> textinput_interface.h:216
> + */
> + Datetime,
> + /**
DateTime?
REPOSITORY
rKWAYLAND KWayland
BRANCH
graesslin/text-input
REVISION DETAIL
https://phabricator.kde.org/D1631
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: graesslin, Plasma, sebas
Cc: sebas, broulik, plasma-devel
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
