Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48735 --- This review has been submitted with commit 2bcd51e65f143d995ad9ef93f97ad4bbc2a480d6 by Martin Gräßlin to branch master. - Commit Hook On Jan. 29, 2014, 7:02 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 29, 2014, 7:02 a.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt 39783988a292cb0dc62e3de91421e36930821fe2 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Feb. 1, 2014, 8:03 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt 39783988a292cb0dc62e3de91421e36930821fe2 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48712 --- Ship it! I'm not a huge fan of the dark templating magic, and I'm not entirely convinced the gains are worth it, but you and Aaron know the major users (Plasma and KWin) better than I do, so I'll defer to your judgement there. Otherwise, it seems pretty sound, and the unit test you created passes with this patch. - Alex Merry On Jan. 29, 2014, 7:02 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 29, 2014, 7:02 a.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt 39783988a292cb0dc62e3de91421e36930821fe2 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 29, 2014, 8:02 a.m.) Review request for KDE Frameworks and kdewin. Changes --- Removed the default ctor which is just going to crash. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs (updated) - src/CMakeLists.txt 39783988a292cb0dc62e3de91421e36930821fe2 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 24, 2014, 9:04 a.m.) Review request for KDE Frameworks and kdewin. Changes --- changed to the suggested way of having a class as template argument and delegate to impl-classes. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs (updated) - src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48200 --- Looks good to me, however I think someone else should give the ship it. - Alexander Richardson On Jan. 24, 2014, 9:04 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 24, 2014, 9:04 a.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 23, 2014, 9:09 a.m.) Review request for KDE Frameworks and kdewin. Changes --- I gave a try to a templated approach to not have the virtual methods in the Private. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs (updated) - src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48124
---
Wouldn't this be easier using methods with the subclass as a template parameter
instead of the enum. I.e:
class KWindowSystemPrivate {
//
template class Impl
inline unsigned long state() const
{
// use a different name to prevent infinite recursion in case this is
called with KWindowSystemPrivate as a template parameter
return static_castImpl*(this)-stateImpl();
}
//
};
class KWindowSystemPrivateDummy : public KWindowSystemPrivate {
//
inline unsigned long stateImpl() const {
return 0;
}
//...
};
class KWindowSystemPrivateX11 : public KWindowSystemPrivate {
//
inline unsigned long stateImpl() const {
return m_info-state();
}
//...
}
Then there is no more need for #define X11 static_castconst
KWindowInfoPrivateX11*(this) anymore.
DELEGATE would look like this then:
#define DELEGATE(name, args) \
switch (d-platform()) { \
case KWindowInfoPrivate::XcbPlatform: \
return d-nameKWindowInfoPrivateX11( args ); \
default: \
return d-nameKWindowInfoPrivateDummy( args ); \
}
This should also work, however I am just typing it from my head without having
it compiled.
- Alexander Richardson
On Jan. 23, 2014, 9:09 a.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/
---
(Updated Jan. 23, 2014, 9:09 a.m.)
Review request for KDE Frameworks and kdewin.
Repository: kwindowsystem
Description
---
Add runtime platform support to KWindowInfo
Main idea of this change is to only pick the X11 implementation in case
that the application is running on the X11 platform. So far it was a
compile time switch which meant that if compiled with X11 support but
not running on the X11 platform it would have caused runtime errors.
To make this possible a KWindowInfoPrivate class with a dummy
implementation is provided. This is used as d-ptr for KWindowInfo.
Thus there is one generic implementation and the implementation of
KWindowInfo is no longer ifdefed for the supported platforms.
The platform specific code can inherit from KWindowInfoPrivate and
overwrite the dummy method implementation. KWindowInfoPrivate provides
a factory method where the platform specific implementation can be
hooked into. There we can have both compile time and runtime checking.
If there is no platfom specific implementation available the dummy
implementation is used.
NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND!
Windows and Mac is excluded from build. At the moment they get the
dummy implementation. Unfortunately I don't have the possibility to
compile the changes and thus don't dare to touch the code. Fixes from
the teams are highly appreciated.
Diffs
-
src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507
src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a
src/kwindowinfo.cpp PRE-CREATION
src/kwindowinfo_p.h PRE-CREATION
src/kwindowinfo_p_x11.h PRE-CREATION
src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f
Diff: https://git.reviewboard.kde.org/r/115225/diff/
Testing
---
Unit test from https://git.reviewboard.kde.org/r/115190/ is still working.
Now you can guess why I wrote that test ;-)
Thanks,
Martin Gräßlin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
On Jan. 23, 2014, 3:47 p.m., Alexander Richardson wrote:
Wouldn't this be easier using methods with the subclass as a template
parameter instead of the enum. I.e:
class KWindowSystemPrivate {
//
template class Impl
inline unsigned long state() const
{
// use a different name to prevent infinite recursion in case this is
called with KWindowSystemPrivate as a template parameter
return static_castImpl*(this)-stateImpl();
}
//
};
class KWindowSystemPrivateDummy : public KWindowSystemPrivate {
//
inline unsigned long stateImpl() const {
return 0;
}
//...
};
class KWindowSystemPrivateX11 : public KWindowSystemPrivate {
//
inline unsigned long stateImpl() const {
return m_info-state();
}
//...
}
Then there is no more need for #define X11 static_castconst
KWindowInfoPrivateX11*(this) anymore.
DELEGATE would look like this then:
#define DELEGATE(name, args) \
switch (d-platform()) { \
case KWindowInfoPrivate::XcbPlatform: \
return d-nameKWindowInfoPrivateX11( args ); \
default: \
return d-nameKWindowInfoPrivateDummy( args ); \
}
This should also work, however I am just typing it from my head without
having it compiled.
return static_castImpl*(this)-stateImpl();
This obviously has to be static_castconst Impl*(this)
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48124
---
On Jan. 23, 2014, 9:09 a.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/
---
(Updated Jan. 23, 2014, 9:09 a.m.)
Review request for KDE Frameworks and kdewin.
Repository: kwindowsystem
Description
---
Add runtime platform support to KWindowInfo
Main idea of this change is to only pick the X11 implementation in case
that the application is running on the X11 platform. So far it was a
compile time switch which meant that if compiled with X11 support but
not running on the X11 platform it would have caused runtime errors.
To make this possible a KWindowInfoPrivate class with a dummy
implementation is provided. This is used as d-ptr for KWindowInfo.
Thus there is one generic implementation and the implementation of
KWindowInfo is no longer ifdefed for the supported platforms.
The platform specific code can inherit from KWindowInfoPrivate and
overwrite the dummy method implementation. KWindowInfoPrivate provides
a factory method where the platform specific implementation can be
hooked into. There we can have both compile time and runtime checking.
If there is no platfom specific implementation available the dummy
implementation is used.
NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND!
Windows and Mac is excluded from build. At the moment they get the
dummy implementation. Unfortunately I don't have the possibility to
compile the changes and thus don't dare to touch the code. Fixes from
the teams are highly appreciated.
Diffs
-
src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507
src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a
src/kwindowinfo.cpp PRE-CREATION
src/kwindowinfo_p.h PRE-CREATION
src/kwindowinfo_p_x11.h PRE-CREATION
src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f
Diff: https://git.reviewboard.kde.org/r/115225/diff/
Testing
---
Unit test from https://git.reviewboard.kde.org/r/115190/ is still working.
Now you can guess why I wrote that test ;-)
Thanks,
Martin Gräßlin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48024 --- src/kwindowinfo_p.h https://git.reviewboard.kde.org/r/115225/#comment34006 Why are we ref counting ourselves instead of just using QExplicitlySharedDataPointer? - David Edmundson On Jan. 22, 2014, 1:09 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 22, 2014, 1:09 p.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
On Jan. 22, 2014, 2:39 p.m., David Edmundson wrote: src/kwindowinfo_p.h, line 80 https://git.reviewboard.kde.org/r/115225/diff/1/?file=235187#file235187line80 Why are we ref counting ourselves instead of just using QExplicitlySharedDataPointer? because it was that way in the old code. I didn't spend much thought on it and just kept the pattern. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48024 --- On Jan. 22, 2014, 2:09 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 22, 2014, 2:09 p.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 22, 2014, 3:03 p.m.) Review request for KDE Frameworks and kdewin. Changes --- changed to QExplicitlySharedDataPointer. d_ed please confirm that the usage is correct, I think I have never used this class before. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs (updated) - src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48044
---
Looks quite nice, other than the missing win/mac support which you can do
little about at this point.
Use of the explicitly shared pointer approach is a simple and nice performance
booster when passing these objects around (as these tend to). +1 for that.
I took a moment to ponder if there is enough duplication of window info objects
for the same window ID to make it worthwhile to have a global hash of winid to
dptrs for re-use between separately created instances (and not just copies of
the same instance). In the desktop shell there is at least one per window for
the taskbar and the pager (assuming the pager is active, anyways); the window
runner usually isn't loaded in the shell directly but were it to be that'd be a
third instance. So the highest duplication likely to be seen is probably 3 and
so it isn't worth it.
It's a bit of a shame that the runtime detection requires a dptr full of
virtuals; this is probably only required on Linux/UNIX where there are multiple
window info protocols (xcb, wayland, ..) so sucks for the other platforms, but
I also suspect that this will never actually be used in practice even on Linux
as one is either going to be in a Wayland or X11 (or whatever) session and
switching between them requires logging in/out. It's private API so it can be
changed later if this were ever to actually show up on in runtime performance
which I seriously doubt it will. (I can imagine sth horrible like a
struct/union which has a pointer to an xcb and a wayland implementation, both
of which have the same literal API for consistency but no base class and then a
macro that calls whichever one is actually instantiated: #define
callimpl(method) if (d-xcb) { d-xcb-method(); } else { d-wayland-method();
} win/mac/etc would have a rather simpler callimpl macro. yes, ugly as #($*
but it would get rid of the virtuals and allow win/mac/android/etc to be
simple compile-time classes without unnecessary runtime detection features ..
but probably not worth the uglification w/out good justification, which
currently doesn't exist.
I haven't done anything more than add it to the compile, so I can't mark it as
ShipIt with a clean conscience (as I haven't tested it at runtime), but the
code looks good and the design is reasonable imho.
- Aaron J. Seigo
On Jan. 22, 2014, 2:03 p.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/
---
(Updated Jan. 22, 2014, 2:03 p.m.)
Review request for KDE Frameworks and kdewin.
Repository: kwindowsystem
Description
---
Add runtime platform support to KWindowInfo
Main idea of this change is to only pick the X11 implementation in case
that the application is running on the X11 platform. So far it was a
compile time switch which meant that if compiled with X11 support but
not running on the X11 platform it would have caused runtime errors.
To make this possible a KWindowInfoPrivate class with a dummy
implementation is provided. This is used as d-ptr for KWindowInfo.
Thus there is one generic implementation and the implementation of
KWindowInfo is no longer ifdefed for the supported platforms.
The platform specific code can inherit from KWindowInfoPrivate and
overwrite the dummy method implementation. KWindowInfoPrivate provides
a factory method where the platform specific implementation can be
hooked into. There we can have both compile time and runtime checking.
If there is no platfom specific implementation available the dummy
implementation is used.
NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND!
Windows and Mac is excluded from build. At the moment they get the
dummy implementation. Unfortunately I don't have the possibility to
compile the changes and thus don't dare to touch the code. Fixes from
the teams are highly appreciated.
Diffs
-
src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507
src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a
src/kwindowinfo.cpp PRE-CREATION
src/kwindowinfo_p.h PRE-CREATION
src/kwindowinfo_p_x11.h PRE-CREATION
src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f
Diff: https://git.reviewboard.kde.org/r/115225/diff/
Testing
---
Unit test from https://git.reviewboard.kde.org/r/115190/ is still working.
Now you can guess why I wrote that test ;-)
Thanks,
Martin Gräßlin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
On Jan. 22, 2014, 3:22 p.m., Aaron J. Seigo wrote:
Looks quite nice, other than the missing win/mac support which you can do
little about at this point.
Use of the explicitly shared pointer approach is a simple and nice
performance booster when passing these objects around (as these tend to).
+1 for that.
I took a moment to ponder if there is enough duplication of window info
objects for the same window ID to make it worthwhile to have a global hash
of winid to dptrs for re-use between separately created instances (and not
just copies of the same instance). In the desktop shell there is at least
one per window for the taskbar and the pager (assuming the pager is active,
anyways); the window runner usually isn't loaded in the shell directly but
were it to be that'd be a third instance. So the highest duplication likely
to be seen is probably 3 and so it isn't worth it.
It's a bit of a shame that the runtime detection requires a dptr full of
virtuals; this is probably only required on Linux/UNIX where there are
multiple window info protocols (xcb, wayland, ..) so sucks for the other
platforms, but I also suspect that this will never actually be used in
practice even on Linux as one is either going to be in a Wayland or X11 (or
whatever) session and switching between them requires logging in/out. It's
private API so it can be changed later if this were ever to actually show
up on in runtime performance which I seriously doubt it will. (I can
imagine sth horrible like a struct/union which has a pointer to an xcb and
a wayland implementation, both of which have the same literal API for
consistency but no base class and then a macro that calls whichever one is
actually instantiated: #define callimpl(method) if (d-xcb) {
d-xcb-method(); } else { d-wayland-method(); } win/mac/etc would have
a rather simpler callimpl macro. yes, ugly as
#($* but it would get rid of the virtuals and allow win/mac/android/etc to be
simple compile-time classes without unnecessary runtime detection features ..
but probably not worth the uglification w/out good justification, which
currently doesn't exist.
I haven't done anything more than add it to the compile, so I can't mark it
as ShipIt with a clean conscience (as I haven't tested it at runtime), but
the code looks good and the design is reasonable imho.
... or a template class instead of the #define, though that adds a layer of
function call indirection I bet it could be 100% inlined functions and be both
fast and non-#define-hackery.
- Aaron J.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48044
---
On Jan. 22, 2014, 2:03 p.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/
---
(Updated Jan. 22, 2014, 2:03 p.m.)
Review request for KDE Frameworks and kdewin.
Repository: kwindowsystem
Description
---
Add runtime platform support to KWindowInfo
Main idea of this change is to only pick the X11 implementation in case
that the application is running on the X11 platform. So far it was a
compile time switch which meant that if compiled with X11 support but
not running on the X11 platform it would have caused runtime errors.
To make this possible a KWindowInfoPrivate class with a dummy
implementation is provided. This is used as d-ptr for KWindowInfo.
Thus there is one generic implementation and the implementation of
KWindowInfo is no longer ifdefed for the supported platforms.
The platform specific code can inherit from KWindowInfoPrivate and
overwrite the dummy method implementation. KWindowInfoPrivate provides
a factory method where the platform specific implementation can be
hooked into. There we can have both compile time and runtime checking.
If there is no platfom specific implementation available the dummy
implementation is used.
NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND!
Windows and Mac is excluded from build. At the moment they get the
dummy implementation. Unfortunately I don't have the possibility to
compile the changes and thus don't dare to touch the code. Fixes from
the teams are highly appreciated.
Diffs
-
src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507
src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a
src/kwindowinfo.cpp PRE-CREATION
src/kwindowinfo_p.h PRE-CREATION
src/kwindowinfo_p_x11.h PRE-CREATION
src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f
Diff: https://git.reviewboard.kde.org/r/115225/diff/
Testing
---
Unit test from
Re: Review Request 115225: Add runtime platform support to KWindowInfo
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48048 --- QSharedData code looks right to me. Thanks. src/kwindowinfo.cpp https://git.reviewboard.kde.org/r/115225/#comment34009 This seems wrong. KWindowInfo cheese; cheese.state(); //CRASH - David Edmundson On Jan. 22, 2014, 2:03 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 22, 2014, 2:03 p.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115225: Add runtime platform support to KWindowInfo
On Jan. 22, 2014, 4:22 p.m., Aaron J. Seigo wrote:
Looks quite nice, other than the missing win/mac support which you can do
little about at this point.
Use of the explicitly shared pointer approach is a simple and nice
performance booster when passing these objects around (as these tend to).
+1 for that.
I took a moment to ponder if there is enough duplication of window info
objects for the same window ID to make it worthwhile to have a global hash
of winid to dptrs for re-use between separately created instances (and not
just copies of the same instance). In the desktop shell there is at least
one per window for the taskbar and the pager (assuming the pager is active,
anyways); the window runner usually isn't loaded in the shell directly but
were it to be that'd be a third instance. So the highest duplication likely
to be seen is probably 3 and so it isn't worth it.
It's a bit of a shame that the runtime detection requires a dptr full of
virtuals; this is probably only required on Linux/UNIX where there are
multiple window info protocols (xcb, wayland, ..) so sucks for the other
platforms, but I also suspect that this will never actually be used in
practice even on Linux as one is either going to be in a Wayland or X11 (or
whatever) session and switching between them requires logging in/out. It's
private API so it can be changed later if this were ever to actually show
up on in runtime performance which I seriously doubt it will. (I can
imagine sth horrible like a struct/union which has a pointer to an xcb and
a wayland implementation, both of which have the same literal API for
consistency but no base class and then a macro that calls whichever one is
actually instantiated: #define callimpl(method) if (d-xcb) {
d-xcb-method(); } else { d-wayland-method(); } win/mac/etc would have
a rather simpler callimpl macro. yes, ugly as
#($* but it would get rid of the virtuals and allow win/mac/android/etc to be
simple compile-time classes without unnecessary runtime detection features ..
but probably not worth the uglification w/out good justification, which
currently doesn't exist.
I haven't done anything more than add it to the compile, so I can't mark it
as ShipIt with a clean conscience (as I haven't tested it at runtime), but
the code looks good and the design is reasonable imho.
Aaron J. Seigo wrote:
... or a template class instead of the #define, though that adds a layer
of function call indirection I bet it could be 100% inlined functions and be
both fast and non-#define-hackery.
I took a moment to ponder if there is enough duplication of window info
objects for the same window ID to make it worthwhile to have a global hash of
winid to dptrs for re-use between separately created instances
Probably won't work as KWindowInfo doesn't update if the window is changed. One
of the next things I want to do is significantly improve the API documentation
of that class.
or a template class
that sounds like fun (yes I have a strange understanding of what is fun), I'll
think about it.
- Martin
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48044
---
On Jan. 22, 2014, 3:03 p.m., Martin Gräßlin wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/
---
(Updated Jan. 22, 2014, 3:03 p.m.)
Review request for KDE Frameworks and kdewin.
Repository: kwindowsystem
Description
---
Add runtime platform support to KWindowInfo
Main idea of this change is to only pick the X11 implementation in case
that the application is running on the X11 platform. So far it was a
compile time switch which meant that if compiled with X11 support but
not running on the X11 platform it would have caused runtime errors.
To make this possible a KWindowInfoPrivate class with a dummy
implementation is provided. This is used as d-ptr for KWindowInfo.
Thus there is one generic implementation and the implementation of
KWindowInfo is no longer ifdefed for the supported platforms.
The platform specific code can inherit from KWindowInfoPrivate and
overwrite the dummy method implementation. KWindowInfoPrivate provides
a factory method where the platform specific implementation can be
hooked into. There we can have both compile time and runtime checking.
If there is no platfom specific implementation available the dummy
implementation is used.
NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND!
Windows and Mac is excluded from build. At the moment they get the
dummy implementation. Unfortunately I don't have the possibility
Re: Review Request 115225: Add runtime platform support to KWindowInfo
On Jan. 22, 2014, 4:32 p.m., David Edmundson wrote: src/kwindowinfo.cpp, line 191 https://git.reviewboard.kde.org/r/115225/diff/2/?file=235203#file235203line191 This seems wrong. KWindowInfo cheese; cheese.state(); //CRASH I kept the ctor as it's documented as makes QList happy. I don't really get why it is there and it looks wrong to me and I think it's a stupid idea to put KWindowInfo into a QList at all. If other people agree that we can do an API break (hello Kevin or David F.) I'll happily remove the ctor and document the API break. - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/#review48048 --- On Jan. 22, 2014, 3:03 p.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115225/ --- (Updated Jan. 22, 2014, 3:03 p.m.) Review request for KDE Frameworks and kdewin. Repository: kwindowsystem Description --- Add runtime platform support to KWindowInfo Main idea of this change is to only pick the X11 implementation in case that the application is running on the X11 platform. So far it was a compile time switch which meant that if compiled with X11 support but not running on the X11 platform it would have caused runtime errors. To make this possible a KWindowInfoPrivate class with a dummy implementation is provided. This is used as d-ptr for KWindowInfo. Thus there is one generic implementation and the implementation of KWindowInfo is no longer ifdefed for the supported platforms. The platform specific code can inherit from KWindowInfoPrivate and overwrite the dummy method implementation. KWindowInfoPrivate provides a factory method where the platform specific implementation can be hooked into. There we can have both compile time and runtime checking. If there is no platfom specific implementation available the dummy implementation is used. NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND! Windows and Mac is excluded from build. At the moment they get the dummy implementation. Unfortunately I don't have the possibility to compile the changes and thus don't dare to touch the code. Fixes from the teams are highly appreciated. Diffs - src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507 src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a src/kwindowinfo.cpp PRE-CREATION src/kwindowinfo_p.h PRE-CREATION src/kwindowinfo_p_x11.h PRE-CREATION src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f Diff: https://git.reviewboard.kde.org/r/115225/diff/ Testing --- Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-) Thanks, Martin Gräßlin ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
