Port DesktopGrid QtQuickWindow to EffectQuickView
ClosedPublic

Authored by davidedmundson on Jan 2 2020, 10:53 PM.

Details

Summary

Lots of red, does the same thing in a simplified way avoiding creating a
real window and having to hack round it.

Test Plan

Click add and remove, remove button disables appropriately

Diff Detail

Repository
R108 KWin
Branch
master
Lint
Lint ErrorsExcuse: asdf
SeverityLocationCodeMessage
Errorlibkwineffects/kwinglobals.h:150CppcheckunknownMacro
Unit
No Unit Test Coverage
Build Status
Buildable 21481
Build 21499: arc lint + arc unit
davidedmundson created this revision.Jan 2 2020, 10:53 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 2 2020, 10:53 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Jan 2 2020, 10:53 PM
zzag added a subscriber: zzag.Jan 6 2020, 10:33 AM

Hmm, I have a dual-monitor setup. On the "primary" screen, add/remove buttons turn black while they fade out. On the second screen, they seem to be fine. They just fade out.

I run NVIDIA btw.

That's surprising :/

Do you see an equivalent with the PresentWindows close button?
If you can do anything to help debug that, I would appreciate it.

zzag added a comment.Jan 6 2020, 10:42 AM

Do you see an equivalent with the PresentWindows close button?

Nope, looks fine.

meven added a subscriber: meven.Jan 6 2020, 12:55 PM

Suggest to convert foreach loops.
Not necessary though. Feel free to ignore/mark as done.

effects/desktopgrid/desktopgrid.cpp
104–105

Could you convert those foreach you touch

194–196

Same

458–460

Could you convert those foreach you touch

1034

Same

port for loops

davidedmundson marked 4 inline comments as done.Jan 6 2020, 3:32 PM
zzag added a comment.Jan 13 2020, 12:08 PM

I'm not able to build kwin with patch

[640/1715] Building CXX object effects/CMakeFiles/kwin4_effect_builtins.dir/kwin4_effect_builtins_autogen/mocs_compilation.cpp.o
FAILED: effects/CMakeFiles/kwin4_effect_builtins.dir/kwin4_effect_builtins_autogen/mocs_compilation.cpp.o 
/usr/lib/ccache/bin/c++  -DEFFECT_BUILTINS -DKCOREADDONS_LIB -DQT_CONCURRENT_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_DISABLE_DEPRECATED_BEFORE=0 -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_NO_URL_CAST_FROM_STRING -DQT_QMLMODELS_LIB -DQT_QML_LIB -DQT_QUICK_LIB -DQT_USE_QSTRINGBUILDER -DQT_WIDGETS_LIB -DQT_X11EXTRAS_LIB -DQT_XML_LIB -DTRANSLATION_DOMAIN=\"kwin_effects\" -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -Dkwin4_effect_builtins_EXPORTS -Ieffects -I/home/vlad/Workspace/KDE/src/kde/workspace/kwin/effects -Ieffects/kwin4_effect_builtins_autogen/include -I/home/vlad/Workspace/KDE/src/kde/workspace/kwin/platformsupport -I/home/vlad/Workspace/KDE/src/kde/workspace/kwin/tabbox -I/home/vlad/Workspace/KDE/src/kde/workspace/kwin/libkwineffects -I. -Ilibkwineffects -I/home/vlad/Workspace/KDE/src/kde/workspace/kwin -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KConfigGui -isystem /home/vlad/Workspace/KDE/usr/include/KF5 -isystem /home/vlad/Workspace/qt5/qtbase/include -isystem /home/vlad/Workspace/qt5/qtbase/include/QtGui -isystem /home/vlad/Workspace/qt5/qtbase/include/QtCore -isystem /home/vlad/Workspace/qt5/qtbase/./mkspecs/linux-g++ -isystem /home/vlad/Workspace/qt5/qtbase/include/QtXml -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KConfigCore -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KConfigWidgets -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KCodecs -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KWidgetsAddons -isystem /home/vlad/Workspace/qt5/qtbase/include/QtWidgets -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KAuth -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KCoreAddons -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KGlobalAccel -isystem /home/vlad/Workspace/qt5/qtbase/include/QtDBus -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KI18n -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KIconThemes -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KNotifications -isystem /home/vlad/Workspace/KDE/usr/include/KF5/Plasma -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KService -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KPackage -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KWindowSystem -isystem /home/vlad/Workspace/qt5/qtbase/include/QtQuick -isystem /home/vlad/Workspace/qt5/qtbase/include/QtQmlModels -isystem /home/vlad/Workspace/qt5/qtbase/include/QtQml -isystem /home/vlad/Workspace/qt5/qtbase/include/QtNetwork -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KDeclarative -isystem /home/vlad/Workspace/KDE/usr/include/KF5/KWayland/Server -isystem /home/vlad/Workspace/qt5/qtbase/include/QtConcurrent -isystem /home/vlad/Workspace/qt5/qtbase/include/QtX11Extras -pipe -fno-operator-names -fno-exceptions -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -Wvla -Wdate-time -Wsuggest-override -Wlogical-op -fdiagnostics-color=always -g -fPIC -fvisibility=hidden -fvisibility-inlines-hidden   -fPIC -std=gnu++14 -MD -MT effects/CMakeFiles/kwin4_effect_builtins.dir/kwin4_effect_builtins_autogen/mocs_compilation.cpp.o -MF effects/CMakeFiles/kwin4_effect_builtins.dir/kwin4_effect_builtins_autogen/mocs_compilation.cpp.o.d -o effects/CMakeFiles/kwin4_effect_builtins.dir/kwin4_effect_builtins_autogen/mocs_compilation.cpp.o -c effects/kwin4_effect_builtins_autogen/mocs_compilation.cpp
In file included from effects/kwin4_effect_builtins_autogen/mocs_compilation.cpp:12:
effects/kwin4_effect_builtins_autogen/OGFQ2SQVNZ/moc_flipswitch.cpp: In static member function ‘static void KWin::FlipSwitchEffect::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)’:
effects/kwin4_effect_builtins_autogen/OGFQ2SQVNZ/moc_flipswitch.cpp:130:85: error: invalid use of incomplete type ‘class QKeySequence’
  130 |         case 2: _t->globalShortcutChanged((*reinterpret_cast< QAction*(*)>(_a[1])),(*reinterpret_cast< QKeySequence(*)>(_a[2]))); break;
      |                                                                                    ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/vlad/Workspace/qt5/qtbase/include/QtCore/qmetatype.h:1,
                 from /home/vlad/Workspace/qt5/qtbase/src/corelib/kernel/qobject.h:54,
                 from /home/vlad/Workspace/qt5/qtbase/include/QtCore/qobject.h:1,
                 from /home/vlad/Workspace/qt5/qtbase/src/corelib/kernel/qcoreapplication.h:46,
                 from /home/vlad/Workspace/qt5/qtbase/include/QtCore/qcoreapplication.h:1,
                 from /home/vlad/Workspace/qt5/qtbase/include/QtCore/QCoreApplication:1,
                 from /home/vlad/Workspace/KDE/src/kde/workspace/kwin/libkwineffects/kwinglobals.h:24,
                 from /home/vlad/Workspace/KDE/src/kde/workspace/kwin/libkwineffects/kwineffects.h:29,
                 from /home/vlad/Workspace/KDE/src/kde/workspace/kwin/effects/backgroundcontrast/contrast.h:24,
                 from effects/kwin4_effect_builtins_autogen/34U44GHX75/moc_contrast.cpp:10,
                 from effects/kwin4_effect_builtins_autogen/mocs_compilation.cpp:2:
/home/vlad/Workspace/qt5/qtbase/src/corelib/kernel/qmetatype.h:1999:1: note: forward declaration of ‘class QKeySequence’
 1999 | QT_FOR_EACH_STATIC_GUI_CLASS(QT_FORWARD_DECLARE_STATIC_TYPES_ITER)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from effects/kwin4_effect_builtins_autogen/OGFQ2SQVNZ/moc_flipswitch.cpp:10,
                 from effects/kwin4_effect_builtins_autogen/mocs_compilation.cpp:12:
/home/vlad/Workspace/KDE/src/kde/workspace/kwin/effects/flipswitch/flipswitch.h:89:62: note:   initializing argument 2 of ‘void KWin::FlipSwitchEffect::globalShortcutChanged(QAction*, QKeySequence)’
   89 |     void globalShortcutChanged(QAction *action, QKeySequence shortcut);
      |                                                 ~~~~~~~~~~~~~^~~~~~~~

Qt version: 5.15

That error is about a missing include on flipswitch.h
It doesn't seem relevant to this patch?

zzag added a comment.Jan 14 2020, 4:45 PM

That error is about a missing include on flipswitch.h
It doesn't seem relevant to this patch?

Yep... See D26668

zzag added a comment.Jan 14 2020, 5:22 PM

Alright, here are some videos to demonstrate the issue

without this patch

with this patch

On the second screen, they seem to be fine. They just fade out.

Nope, it also happens on the second monitor.

Modernise QML

Port avoids issues caused by Text with NativeRendering not blending properly
which vlad was seeing above

zzag added a comment.EditedJan 16 2020, 12:22 AM

Sweet! Desktop grid buttons look good during transitions, although they are smaller than used to be. Not a serious problem, though.

effects/desktopgrid/desktopgrid.cpp
104–105

Please don't use auto here, it obfuscates code rather than erases redundancy. Also, there has to be whitespace before :.

Remove unused destructor

Lifespan is scoped by EffectQuickView parent. The deleteLater was another
hack that we no longer need.

davidedmundson added inline comments.Jan 16 2020, 3:20 PM
effects/desktopgrid/desktopgrid.cpp
104–105

I've said before that if we are to have a policy on auto, can we try to start a thread on https://techbase.kde.org/Policies/Frameworks_Coding_Style about auto.

Same for the colon.
And maybe for your potential comment about defining empty methods :P

zzag added inline comments.Jan 16 2020, 3:57 PM
effects/desktopgrid/desktopgrid.cpp
104–105

Well, it's true, usage of auto is not covered by any coding convention in KDE afaik, but we could use common sense to decide whether it's okay to use auto here

Given

for (EffectQuickScene *view : m_desktopButtonsViews) {

and

for (auto *view : m_desktopButtonsViews) {

Which one is more readable?


view is not an iterator and there is no repetition.

zzag accepted this revision.Jan 22 2020, 11:24 AM
zzag added inline comments.
effects/desktopgrid/desktopgrid.cpp
194

Align * to right.

This revision is now accepted and ready to land.Jan 22 2020, 11:24 AM
This revision was automatically updated to reflect the committed changes.