From 14046eb1892869e61c413c8614fe94a3697ef8ab Mon Sep 17 00:00:00 2001 From: Dawid Wojtas Date: Fri, 4 Aug 2023 12:02:06 +0200 Subject: [PATCH] [BH-1745] Fix a deep press crash during popups Rewrite the popups filter to remove unnecessary popups. Remove callbacks from activate and deactivate popups which cause problems when the popups was on the stack. --- harmony_changelog.md | 1 + module-apps/application-call/ApplicationCall.cpp | 4 ++-- .../application-desktop/ApplicationDesktop.cpp | 5 +++-- .../ApplicationOnBoarding.cpp | 5 +++-- module-apps/apps-common/ApplicationCommon.cpp | 11 ++++++++--- module-apps/apps-common/WindowsPopupFilter.cpp | 14 ++++++++++++-- module-apps/apps-common/WindowsPopupFilter.hpp | 15 ++++++++++++--- .../tests/windows/test-WindowsPopupFilter.cpp | 8 +++++--- products/BellHybrid/apps/Application.cpp | 2 +- .../ApplicationBellMain.cpp | 10 ++++++++-- .../include/common/models/AbstractAlarmModel.hpp | 3 ++- .../common/include/common/models/AlarmModel.hpp | 3 ++- .../common/popups/AlarmDeactivatedWindow.hpp | 2 +- .../popups/presenter/AlarmActivatedPresenter.hpp | 8 +------- .../BellHybrid/apps/common/src/AlarmModel.cpp | 10 +++++++++- .../common/src/popups/AlarmActivatedWindow.cpp | 5 +---- .../common/src/popups/AlarmDeactivatedWindow.cpp | 1 - .../popups/presenter/AlarmActivatedPresenter.cpp | 16 +--------------- .../BellHybrid/services/time/AlarmOperations.cpp | 8 ++++---- products/BellHybrid/sys/SystemManager.cpp | 2 +- 20 files changed, 77 insertions(+), 56 deletions(-) diff --git a/harmony_changelog.md b/harmony_changelog.md index 3847b30bfa340c055d13076a5ad4166f3b3cf99c..352125e08080dce791bf7d84a303746378e87fd1 100644 --- a/harmony_changelog.md +++ b/harmony_changelog.md @@ -15,6 +15,7 @@ * Fixed "Next alarm will ring in 24h" popup on shutdown screen * Fixed redundant clock face display while shutting down Harmony * Fixed problem with disabling the frontlight in pre-wake up +* Fixed occasional crash when a deep press occurs during popups ### Added diff --git a/module-apps/application-call/ApplicationCall.cpp b/module-apps/application-call/ApplicationCall.cpp index 4e412917e99813dcc97c9223c5df9287122daed2..0345cb2fe1f080b4e65bb769de6656157cd17cf3 100644 --- a/module-apps/application-call/ApplicationCall.cpp +++ b/module-apps/application-call/ApplicationCall.cpp @@ -43,9 +43,9 @@ namespace app getPopupFilter().addAppDependentFilter([&](const gui::PopupRequestParams &popupParams) { if (popupParams.getPopupId() == gui::popup::ID::Volume) { - return true; + return gui::popup::FilterType::Show; } - return true; + return gui::popup::FilterType::Show; }); statusBarManager->enableIndicators( {Indicator::Signal, Indicator::Time, Indicator::Battery, Indicator::SimCard}); diff --git a/module-apps/application-desktop/ApplicationDesktop.cpp b/module-apps/application-desktop/ApplicationDesktop.cpp index 90d73dc61b94ef71a41aa0ddef7de01c36057324..904ba612fad6b066bc26666f594865811a23db57 100644 --- a/module-apps/application-desktop/ApplicationDesktop.cpp +++ b/module-apps/application-desktop/ApplicationDesktop.cpp @@ -33,8 +33,9 @@ namespace app std::make_shared(SIMConfiguration::DisplayMode::OnlyInactiveState)); bus.channels.push_back(sys::BusChannel::ServiceDBNotifications); - getPopupFilter().addAppDependentFilter( - [&](const gui::PopupRequestParams & /*popupParams*/) { return !blockAllPopups; }); + getPopupFilter().addAppDependentFilter([&](const gui::PopupRequestParams & /*popupParams*/) { + return blockAllPopups ? gui::popup::FilterType::Ignore : gui::popup::FilterType::Show; + }); addActionReceiver(app::manager::actions::ShowMMIResponse, [this](auto &&data) { switchWindow(app::window::name::desktop_mmi_pull, std::move(data)); diff --git a/module-apps/application-onboarding/ApplicationOnBoarding.cpp b/module-apps/application-onboarding/ApplicationOnBoarding.cpp index 7a406d81ecb44b52d4f52f910342bae10669e365..beac6f1de304678cec3c25a8b564e07f34001ed3 100644 --- a/module-apps/application-onboarding/ApplicationOnBoarding.cpp +++ b/module-apps/application-onboarding/ApplicationOnBoarding.cpp @@ -47,7 +47,8 @@ namespace app bus.channels.push_back(sys::BusChannel::ServiceCellularNotifications); getPopupFilter().addAppDependentFilter([&](const gui::PopupRequestParams & /*popupParams*/) { - return gui::name::window::main_window != getCurrentWindow()->getName(); + return gui::name::window::main_window != getCurrentWindow()->getName() ? gui::popup::FilterType::Show + : gui::popup::FilterType::Ignore; }); } @@ -146,7 +147,7 @@ namespace app const auto eulaDirPath = purefs::dir::getSystemDataDirPath() / "licenses"; const auto eulaFilename = "eula.txt"; auto eulaRepository = std::make_unique(eulaDirPath, eulaFilename); - auto presenter = std::make_unique([&]() { acceptEULA(); }, + auto presenter = std::make_unique([&]() { acceptEULA(); }, std::move(eulaRepository)); return std::make_unique(app, std::move(presenter)); }); diff --git a/module-apps/apps-common/ApplicationCommon.cpp b/module-apps/apps-common/ApplicationCommon.cpp index 8d5574d4940b005f0d45077f9bde51adba612b36..b19418b840a2df58d85f989ab5397be939b7d249 100644 --- a/module-apps/apps-common/ApplicationCommon.cpp +++ b/module-apps/apps-common/ApplicationCommon.cpp @@ -864,9 +864,14 @@ namespace app data->setDisposition(gui::popup::Disposition{ gui::popup::Disposition::Priority::Normal, gui::popup::Disposition::WindowType::Popup, id}); } - windowsPopupQueue->pushRequest(gui::popup::Request(id, std::move(data), *blueprint)); - auto result = tryShowPopup(); - LOG_INFO("tryShowPopup %s status: %s", magic_enum::enum_name(id).data(), result ? "shown" : "ignored"); + if (popupFilter->addPopup(gui::PopupRequestParams(id))) { + windowsPopupQueue->pushRequest(gui::popup::Request(id, std::move(data), *blueprint)); + auto result = tryShowPopup(); + LOG_INFO("Try to show Popup %s status: %s", magic_enum::enum_name(id).data(), result ? "shown" : "ignored"); + } + else { + LOG_INFO("Popup %s removed", magic_enum::enum_name(id).data()); + } } gui::popup::Filter &ApplicationCommon::getPopupFilter() const diff --git a/module-apps/apps-common/WindowsPopupFilter.cpp b/module-apps/apps-common/WindowsPopupFilter.cpp index 074c10a96f81914d2609d150e5d0405c8550dea8..244e52c147eaa538d9217dfca4b56a628f5db30c 100644 --- a/module-apps/apps-common/WindowsPopupFilter.cpp +++ b/module-apps/apps-common/WindowsPopupFilter.cpp @@ -16,7 +16,7 @@ namespace gui::popup bool Filter::isPermitted(const gui::PopupRequestParams ¶ms) const { for (const auto &filter : appDependentFilter) { - if (filter != nullptr && not filter(params)) { + if (filter != nullptr && filter(params) != FilterType::Show) { return false; } } @@ -40,7 +40,17 @@ namespace gui::popup return true; } - void Filter::addAppDependentFilter(std::function f) + bool Filter::addPopup(const gui::PopupRequestParams ¶ms) const + { + for (const auto &filter : appDependentFilter) { + if (filter == nullptr || filter(params) == FilterType::Remove) { + return false; + } + } + return true; + } + + void Filter::addAppDependentFilter(std::function f) { appDependentFilter.push_back(f); } diff --git a/module-apps/apps-common/WindowsPopupFilter.hpp b/module-apps/apps-common/WindowsPopupFilter.hpp index 10eb0f3cc4bf3a7e2ffae34284d7f502a424265d..7e9f7f410bbd951a9751303b4045434a4d934582 100644 --- a/module-apps/apps-common/WindowsPopupFilter.hpp +++ b/module-apps/apps-common/WindowsPopupFilter.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -16,12 +16,20 @@ namespace app } namespace gui::popup { + + enum class FilterType + { + Show = 0, // Show popup + Ignore, // Ignore popup but add to the popup's queue + Remove // Remove and don't add to the popup's queue + }; + /// This filter class is used just to filter next popup to handle if any /// it can and should be overriden to filter out only desired ones at the time class Filter { private: - std::list> appDependentFilter; + std::list> appDependentFilter; /// non-owning pointer to existing stack - @see attachWindowsStack() app::WindowsStack *stack = nullptr; @@ -29,7 +37,8 @@ namespace gui::popup virtual ~Filter() = default; void attachWindowsStack(app::WindowsStack *stack); - void addAppDependentFilter(std::function f); + void addAppDependentFilter(std::function f); virtual bool isPermitted(const gui::PopupRequestParams ¶ms) const; + virtual bool addPopup(const gui::PopupRequestParams ¶ms) const; }; } // namespace gui::popup diff --git a/module-apps/tests/windows/test-WindowsPopupFilter.cpp b/module-apps/tests/windows/test-WindowsPopupFilter.cpp index 63f97c22cf64f63d4568f9c81a27dae99729cee1..1128ab1e6a06671c485b93b45030cd0318821c23 100644 --- a/module-apps/tests/windows/test-WindowsPopupFilter.cpp +++ b/module-apps/tests/windows/test-WindowsPopupFilter.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include "WindowsPopupFilter.hpp" @@ -14,7 +14,8 @@ TEST_CASE("WindowsPopupQueue::isPermitted", "[!mayfail]") gui::popup::Filter filter; SECTION("filter is ok, there is not stack") { - filter.addAppDependentFilter([](const gui::PopupRequestParams &) -> bool { return true; }); + filter.addAppDependentFilter( + [](const gui::PopupRequestParams &) -> gui::popup::FilterType { return gui::popup::FilterType::Show; }); REQUIRE(filter.isPermitted(prp)); } @@ -84,6 +85,7 @@ TEST_CASE("WindowsPopupQueue::addAppDependentFilter") gui::popup::Filter filter; auto prp = gui::PopupRequestParams(gui::popup::ID::Alarm); // create filter that accepts nothing - filter.addAppDependentFilter([](const gui::PopupRequestParams &) -> bool { return false; }); + filter.addAppDependentFilter( + [](const gui::PopupRequestParams &) -> gui::popup::FilterType { return gui::popup::FilterType::Ignore; }); REQUIRE(not filter.isPermitted(prp)); } diff --git a/products/BellHybrid/apps/Application.cpp b/products/BellHybrid/apps/Application.cpp index 13a165a0abc41bcdfbfa4e8fcca325512aa314d9..403c0fbb310e439a1139b8c25cc63c2b8426e95a 100644 --- a/products/BellHybrid/apps/Application.cpp +++ b/products/BellHybrid/apps/Application.cpp @@ -34,7 +34,7 @@ namespace app if (val == true) { LOG_ERROR("block popup - as curent window is in higher order popup"); } - return !val; + return val ? gui::popup::FilterType::Ignore : gui::popup::FilterType::Show; }); } diff --git a/products/BellHybrid/apps/application-bell-main/ApplicationBellMain.cpp b/products/BellHybrid/apps/application-bell-main/ApplicationBellMain.cpp index d77f80755b7bdc93219337b02cb3f401a948cbfc..2505edefb411991ba3865872489e876572ca512e 100644 --- a/products/BellHybrid/apps/application-bell-main/ApplicationBellMain.cpp +++ b/products/BellHybrid/apps/application-bell-main/ApplicationBellMain.cpp @@ -42,9 +42,10 @@ namespace app const auto popupId = params.getPopupId(); if (popupId == gui::popup::ID::Alarm || popupId == gui::popup::ID::AlarmActivated || popupId == gui::popup::ID::AlarmDeactivated) { - return gui::name::window::main_window != getCurrentWindow()->getName(); + return gui::name::window::main_window != getCurrentWindow()->getName() ? gui::popup::FilterType::Show + : gui::popup::FilterType::Remove; } - return true; + return gui::popup::FilterType::Show; }); bus.channels.push_back(sys::BusChannel::ServiceDBNotifications); @@ -84,6 +85,11 @@ namespace app }); connect(typeid(AlarmDeactivated), [this](sys::Message *request) -> sys::MessagePointer { alarmModel->turnOff(); + alarmModel->activateAlarm(false); + return sys::msgHandled(); + }); + connect(typeid(AlarmActivated), [this](sys::Message *request) -> sys::MessagePointer { + alarmModel->activateAlarm(true); return sys::msgHandled(); }); } diff --git a/products/BellHybrid/apps/common/include/common/models/AbstractAlarmModel.hpp b/products/BellHybrid/apps/common/include/common/models/AbstractAlarmModel.hpp index 4e128edf471d0bf09c5b449a772a5033965fb6d0..d5a71ba7c2529cc6a818c970861105ad24031526 100644 --- a/products/BellHybrid/apps/common/include/common/models/AbstractAlarmModel.hpp +++ b/products/BellHybrid/apps/common/include/common/models/AbstractAlarmModel.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -33,6 +33,7 @@ namespace app virtual std::chrono::seconds getTimeToNextSnooze() = 0; virtual TimePoint getTimeOfNextSnooze() = 0; virtual alarms::AlarmStatus getAlarmStatus() = 0; + virtual void activateAlarm(bool state) = 0; /// Command model to update its internal data virtual void update(AlarmModelReadyHandler callback = AlarmModelReadyHandler()) = 0; }; diff --git a/products/BellHybrid/apps/common/include/common/models/AlarmModel.hpp b/products/BellHybrid/apps/common/include/common/models/AlarmModel.hpp index 4ea6a8489300be6e70b3945e170849efe0524968..cc81138e0ea19167572ab4db40bffeed9c02cb54 100644 --- a/products/BellHybrid/apps/common/include/common/models/AlarmModel.hpp +++ b/products/BellHybrid/apps/common/include/common/models/AlarmModel.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -37,6 +37,7 @@ namespace app bool isSnoozeActive() override; void turnOff() override; void snooze() override; + void activateAlarm(bool state) override; std::chrono::seconds getTimeToNextSnooze() override; TimePoint getTimeOfNextSnooze() override; alarms::AlarmStatus getAlarmStatus() override; diff --git a/products/BellHybrid/apps/common/include/common/popups/AlarmDeactivatedWindow.hpp b/products/BellHybrid/apps/common/include/common/popups/AlarmDeactivatedWindow.hpp index 06d79117d7da4e694d7938c9d336ab6b29414b04..aaddddc694e0ab4f2d6686cc8b3ea8533379978f 100644 --- a/products/BellHybrid/apps/common/include/common/popups/AlarmDeactivatedWindow.hpp +++ b/products/BellHybrid/apps/common/include/common/popups/AlarmDeactivatedWindow.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once diff --git a/products/BellHybrid/apps/common/include/common/popups/presenter/AlarmActivatedPresenter.hpp b/products/BellHybrid/apps/common/include/common/popups/presenter/AlarmActivatedPresenter.hpp index 4a29462cfb2ac51fe9f5685276ae652c68fe793c..58b30e17a8e5afbb956303e2948eba0fc4c8c513 100644 --- a/products/BellHybrid/apps/common/include/common/popups/presenter/AlarmActivatedPresenter.hpp +++ b/products/BellHybrid/apps/common/include/common/popups/presenter/AlarmActivatedPresenter.hpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once @@ -36,11 +36,8 @@ namespace app::popup { public: virtual ~Presenter() noexcept = default; - virtual void updateAlarmModel(AlarmModelReadyHandler callback) = 0; virtual time_t getAlarmTime() const noexcept = 0; virtual bool isAlarmActive() const noexcept = 0; - virtual void activate() = 0; - virtual void deactivate() = 0; }; }; @@ -49,11 +46,8 @@ namespace app::popup public: AlarmActivatedPresenter(AbstractAlarmModel &alarmModel); - void updateAlarmModel(AlarmModelReadyHandler callback); time_t getAlarmTime() const noexcept; bool isAlarmActive() const noexcept; - void activate(); - void deactivate(); private: AbstractAlarmModel &alarmModel; diff --git a/products/BellHybrid/apps/common/src/AlarmModel.cpp b/products/BellHybrid/apps/common/src/AlarmModel.cpp index 533ef447dbf6b931af98c1de88bacde64e35669e..2206302d0c81100c002df33ebfdbfed9bbbd2c99 100644 --- a/products/BellHybrid/apps/common/src/AlarmModel.cpp +++ b/products/BellHybrid/apps/common/src/AlarmModel.cpp @@ -153,7 +153,9 @@ namespace app { snoozeCount = 0; nextSnoozeTime = TIME_POINT_INVALID; - alarms::AlarmServiceAPI::requestTurnOffRingingAlarm(app, cachedRecord.parent->ID); + if (cachedRecord.parent != nullptr) { + alarms::AlarmServiceAPI::requestTurnOffRingingAlarm(app, cachedRecord.parent->ID); + } } void AlarmModel::snooze() @@ -220,6 +222,12 @@ namespace app updateAlarm(*alarmEventPtr); } + void AlarmModel::activateAlarm(bool state) + { + auto callback = [this, state]() { activate(state); }; + update(callback); + } + alarms::AlarmStatus AlarmModel::getAlarmStatus() { return alarmStatus; diff --git a/products/BellHybrid/apps/common/src/popups/AlarmActivatedWindow.cpp b/products/BellHybrid/apps/common/src/popups/AlarmActivatedWindow.cpp index 5a0bc6371f11296d7ecb737813ba27e41e73094a..af8461d4ef2b1f73d0954a6ae794f80ca16e8523 100644 --- a/products/BellHybrid/apps/common/src/popups/AlarmActivatedWindow.cpp +++ b/products/BellHybrid/apps/common/src/popups/AlarmActivatedWindow.cpp @@ -28,10 +28,6 @@ namespace gui std::move(presenter)) { getPresenter()->attach(this); - getPresenter()->updateAlarmModel([&]() { - setAlarmTime(getPresenter()->getAlarmTime()); - getPresenter()->activate(); - }); buildInterface(); timerCallback = [this](Item &, sys::Timer &) { @@ -56,6 +52,7 @@ namespace gui void AlarmActivatedWindow::onBeforeShow(ShowMode mode, SwitchData *data) { + setAlarmTime(getPresenter()->getAlarmTime()); WindowWithTimer::onBeforeShow(mode, data); } diff --git a/products/BellHybrid/apps/common/src/popups/AlarmDeactivatedWindow.cpp b/products/BellHybrid/apps/common/src/popups/AlarmDeactivatedWindow.cpp index 3879a89d6c27d04edbf82651fbd2908c7b84ad9f..3e0cea524e2afb2bf208943b71cd6b638c3ac940 100644 --- a/products/BellHybrid/apps/common/src/popups/AlarmDeactivatedWindow.cpp +++ b/products/BellHybrid/apps/common/src/popups/AlarmDeactivatedWindow.cpp @@ -23,7 +23,6 @@ namespace gui { getPresenter()->attach(this); buildInterface(); - getPresenter()->updateAlarmModel([&]() { getPresenter()->deactivate(); }); timerCallback = [this](Item &, sys::Timer &) { returnToPreviousWindow(); diff --git a/products/BellHybrid/apps/common/src/popups/presenter/AlarmActivatedPresenter.cpp b/products/BellHybrid/apps/common/src/popups/presenter/AlarmActivatedPresenter.cpp index e2fa16310e72cdfe3a2b534ee7ed376cdf940cb2..973ffe7daefebe3dec18d3ebc370190c983ca93c 100644 --- a/products/BellHybrid/apps/common/src/popups/presenter/AlarmActivatedPresenter.cpp +++ b/products/BellHybrid/apps/common/src/popups/presenter/AlarmActivatedPresenter.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2017-2022, Mudita Sp. z.o.o. All rights reserved. +// Copyright (c) 2017-2023, Mudita Sp. z.o.o. All rights reserved. // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #include @@ -21,18 +21,4 @@ namespace app::popup return alarmModel.getAlarmTime(); } - void AlarmActivatedPresenter::activate() - { - return alarmModel.activate(true); - } - - void AlarmActivatedPresenter::deactivate() - { - return alarmModel.activate(false); - } - - void AlarmActivatedPresenter::updateAlarmModel(AlarmModelReadyHandler callback) - { - alarmModel.update(callback); - } } // namespace app::popup diff --git a/products/BellHybrid/services/time/AlarmOperations.cpp b/products/BellHybrid/services/time/AlarmOperations.cpp index f933d77cf8915ff965cdda5e82e00bdcdd1e6112..f72cbf49b0cfe7e60d96603969b609d1a9380305 100644 --- a/products/BellHybrid/services/time/AlarmOperations.cpp +++ b/products/BellHybrid/services/time/AlarmOperations.cpp @@ -181,10 +181,6 @@ namespace alarms bool AlarmOperations::processPreWakeUp(TimePoint now) { - if (nextSingleEvents.empty()) { - return false; - } - auto nextEvent = getNextPreWakeUpEvent(); if (!nextEvent.isValid()) { return false; @@ -209,6 +205,10 @@ namespace alarms SingleEventRecord AlarmOperations::getNextPreWakeUpEvent() { + if (nextSingleEvents.empty()) { + return {}; + } + const auto event = *(nextSingleEvents.front()); if (getAlarmEventType(event) != alarms::AlarmType::Clock) { return {}; diff --git a/products/BellHybrid/sys/SystemManager.cpp b/products/BellHybrid/sys/SystemManager.cpp index 753cd6e96e390ef0aa7b17f28d1f3aa7ab78e2fd..6442a36a18d60afbd1372d2a951b925e0bdad17e 100644 --- a/products/BellHybrid/sys/SystemManager.cpp +++ b/products/BellHybrid/sys/SystemManager.cpp @@ -32,7 +32,7 @@ namespace sys { switch (request->getStatus()) { case AlarmActivationStatus::ACTIVATED: - bus.sendUnicast(std::make_shared(), service::name::appmgr); + bus.sendMulticast(std::make_shared(), sys::BusChannel::AlarmNotifications); break; case AlarmActivationStatus::DEACTIVATED: bus.sendMulticast(std::make_shared(), sys::BusChannel::AlarmNotifications);