From 31f5ca4546bb130e49088e04ddde0e5265fbfa24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Fri, 9 Aug 2019 21:25:27 +0200 Subject: [PATCH] Platform: check and return a reference from Screen::application(). Giving out a pointer implied excessive error checking in user code. There's a new hasApplication() accessor that can be used to check for application presence, moreover the ScreenedApplication is now convertible to a pointer to provide backward compatiblity. This conversion is marked as deprecated and will be removed in a future release. --- doc/changelog.dox | 13 +++++++ src/Magnum/Platform/Screen.h | 39 +++++++++++++++++---- src/Magnum/Platform/ScreenedApplication.h | 12 +++++++ src/Magnum/Platform/ScreenedApplication.hpp | 24 +++++++++++-- 4 files changed, 78 insertions(+), 10 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 82aa53ad9..b5bdca4b6 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -387,6 +387,11 @@ See also: in a RAII manner, without needing to explicitly call @ref Platform::BasicScreenedApplication::addScreen() and @ref Platform::BasicScreen::setPropagatedEvents() later +- @ref Platform::BasicScreen::application() now returns a reference instead + of a pointer, checking that the screen is actually added to the + application. Users are encouraged to call + @ref Platform::BasicScreen::hasApplication() to check if the application is + accessible. @subsubsection changelog-latest-changes-text Text library @@ -575,6 +580,14 @@ See also: - Passing @cpp nullptr @ce to @ref ImageView constructors is deprecated and will print a warning at runtime. Use a constructor without the @p data parameter instead. +- @ref Platform::BasicScreen::application() now returns a reference instead + of a pointer and together with @ref Platform::BasicScreen::redraw() checks + that the screen is actually added to the application instead of returning + @cpp nullptr @ce (and requiring excessive error checking in user code) or, + in case of @ref Platform::BasicScreen::redraw() "redraw()" silently doing + the wrong thing. For backwards compatibility + @ref Platform::BasicScreenedApplication is convertible to a pointer and + implements @cpp operator-> @ce, but this conversion is deprecated. @subsection changelog-latest-compatibility Potential compatibility breakages, removed APIs diff --git a/src/Magnum/Platform/Screen.h b/src/Magnum/Platform/Screen.h index c72dde1c3..e464164dd 100644 --- a/src/Magnum/Platform/Screen.h +++ b/src/Magnum/Platform/Screen.h @@ -252,13 +252,33 @@ template class BasicScreen: */ void setPropagatedEvents(PropagatedEvents events) { _propagatedEvents = events; } - /** @brief Application holding this screen */ - template> T* application() { - return static_cast(Containers::LinkedListItem, BasicScreenedApplication>::list()); + /** + * @brief Whether the screen is added to an application + * + * If not, the @ref application() accessor can't be used. + * @see @ref BasicScreenedApplication::addScreen(), + * @ref BasicScreenedApplication::removeScreen() + */ + bool hasApplication() { + return Containers::LinkedListItem, BasicScreenedApplication>::list(); } + + /** + * @brief Application holding this screen + * + * Expects that the screen is added to an application. + * @see @ref hasApplication() + */ + BasicScreenedApplication& application(); /** @overload */ - template> const T* application() const { - return static_cast(Containers::LinkedListItem, BasicScreenedApplication>::list()); + const BasicScreenedApplication& application() const; + /** @overload */ + template> T& application() { + return static_cast(application()); + } + /** @overload */ + template> const T& application() const { + return static_cast(application()); } /** @@ -294,8 +314,13 @@ template class BasicScreen: /** @{ @name Screen handling */ protected: - /** @brief Request redraw */ - virtual void redraw() { application()->redraw(); } + /** + * @brief Request redraw + * + * Expects that the screen is added to an application. + * @see @ref hasApplication() + */ + virtual void redraw(); private: /** diff --git a/src/Magnum/Platform/ScreenedApplication.h b/src/Magnum/Platform/ScreenedApplication.h index 9f1a816e3..bc77523d9 100644 --- a/src/Magnum/Platform/ScreenedApplication.h +++ b/src/Magnum/Platform/ScreenedApplication.h @@ -265,6 +265,18 @@ template class BasicScreenedApplication: return static_cast>&>(*this); } + #if defined(MAGNUM_BUILD_DEPRECATED) && !defined(DOXYGEN_GENERATING_OUTPUT) + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") BasicScreenedApplication* operator->() { return this; } + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") const BasicScreenedApplication* operator->() const { return this; } + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") BasicScreenedApplication& operator*() { return *this; } + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") const BasicScreenedApplication& operator*() const { return *this; } + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") operator BasicScreenedApplication*() { return this; } + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") operator const BasicScreenedApplication*() const { return this; } + template, T>::value>::type> CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") operator T*() { return static_cast(this); } + template, T>::value>::type> CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now") operator const T*() const { return static_cast(this); } + CORRADE_DEPRECATED("Platform::Screen::application() returns a reference now, use hasApplication() instead") bool operator!() const { return false; } + #endif + protected: /* Nobody will need to have (and delete) ScreenedApplication*, thus this is faster than public pure virtual destructor */ diff --git a/src/Magnum/Platform/ScreenedApplication.hpp b/src/Magnum/Platform/ScreenedApplication.hpp index e2ef62e1a..08e7083fe 100644 --- a/src/Magnum/Platform/ScreenedApplication.hpp +++ b/src/Magnum/Platform/ScreenedApplication.hpp @@ -117,6 +117,24 @@ template BasicScreen::BasicScreen(BasicScreenedA template BasicScreen::~BasicScreen() = default; +template BasicScreenedApplication& BasicScreen::application() { + BasicScreenedApplication* application = Containers::LinkedListItem, BasicScreenedApplication>::list(); + CORRADE_ASSERT(application, "Platform::Screen::application(): the screen is not added to any application", *application); + return *application; +} + +template const BasicScreenedApplication& BasicScreen::application() const { + const BasicScreenedApplication* application = Containers::LinkedListItem, BasicScreenedApplication>::list(); + CORRADE_ASSERT(application, "Platform::Screen::application(): the screen is not added to any application", *application); + return *application; +} + +template void BasicScreen::redraw() { + BasicScreenedApplication* application = Containers::LinkedListItem, BasicScreenedApplication>::list(); + CORRADE_ASSERT(application, "Platform::Screen::redraw(): the screen is not added to any application", ); + application->redraw(); +} + template void BasicScreen::focusEvent() {} template void BasicScreen::blurEvent() {} @@ -149,7 +167,7 @@ template BasicScreenedApplication::BasicScreened template BasicScreenedApplication::~BasicScreenedApplication() = default; template BasicScreenedApplication& BasicScreenedApplication::addScreen(BasicScreen& screen) { - CORRADE_ASSERT(!screen.application(), + CORRADE_ASSERT(!screen.hasApplication(), "Platform::ScreenedApplication::addScreen(): screen already added to an application", *this); /* A subset of this (except focusEvent()) is done in @@ -162,7 +180,7 @@ template BasicScreenedApplication& BasicScreened } template BasicScreenedApplication& BasicScreenedApplication::removeScreen(BasicScreen& screen) { - CORRADE_ASSERT(screen.application() == this, + CORRADE_ASSERT(screen.hasApplication() && &screen.application() == this, "Platform::ScreenedApplication::removeScreen(): screen not owned by this application", *this); screen.blurEvent(); @@ -172,7 +190,7 @@ template BasicScreenedApplication& BasicScreened } template BasicScreenedApplication& BasicScreenedApplication::focusScreen(BasicScreen& screen) { - CORRADE_ASSERT(screen.application() == this, + CORRADE_ASSERT(screen.hasApplication() && &screen.application() == this, "Platform::ScreenedApplication::focusScreen(): screen not owned by this application", *this); /* Already focused, nothing to do */