slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
16.94k stars 568 forks source link

C++ filter model: defer first execution of filter function #4983

Closed mous16 closed 5 months ago

mous16 commented 5 months ago

With current implementation of C++ slint::FilterModel it's easy to trigger undefined behavior, or obtain wrong results, due to invocation of filtering callable before slint::FilterModel children finalize initialization. Moreover, if not hitting undefined behavior, a spurious additional filtering can be required.

As a comparison, slint::SortModel already implements the deferred policy.

Discussed in https://github.com/slint-ui/slint/discussions/4972

Originally posted by **mous16** March 28, 2024 Playing around with custom models, I found a peculiar behavior of `slint::FilterModel` C++ implementation. Specifically, when the constructor of `slint::FilterModel` is executed, it performs contextually the filtering on origin model, executing the callable passed as argument. While this is perfectly fine when using directly `slint::FilterModel`, one should be very careful when subclassing it, because it can lead to accessing child members before their initialization, and potentially undefined behavior. On the other hand, `slint::SortModel` defers sorting computation at first model usage, preventing the cited scenario. Here an example: ```Slint // appwindow.slint import { ListView, VerticalBox, Switch } from "std-widgets.slint"; export component AppWindow inherits Window { in property <[int]> model; in-out property only-evens <=> only-evens-switch.checked; in-out property ascending <=> ascending-switch.checked; callback only-evens-changed <=> only-evens-switch.toggled; callback ascending-changed <=> ascending-switch.toggled; VerticalBox { only-evens-switch := Switch { text: "Only evens"; } ascending-switch := Switch { text: "Ascending"; } ListView { for item in root.model: Text { text: item; } } } } ``` ```C++ // main.cpp #include #include #include #include "appwindow.h" #include "slint.h" class MyFilterModel: public slint::FilterModel { bool onlyEvens{false}; [[nodiscard]] auto filter(const int& element) const -> bool { std::println("Filtering with onlyEvens={}; {} -> {}", onlyEvens, element, (!onlyEvens || element % 2 == 0)); return !onlyEvens || element % 2 == 0; } public: MyFilterModel(std::shared_ptr> source_model): slint::FilterModel{std::move(source_model), [this](const int& element) { return filter(element); }} { std::println("MyFilterModel initialized, onlyEvens={}", onlyEvens); } [[nodiscard]] auto getOnlyEvens() const -> bool { return onlyEvens; } void setOnlyEvens(bool value) { std::println("Updating filter; {} -> {}", onlyEvens, value); onlyEvens = value; reset(); } }; class MySortModel: public slint::SortModel { bool ascending{true}; [[nodiscard]] auto sort(const int& first, const int& second) const -> bool { std::println("Sorting with ascending={}; {} | {} -> {}", ascending, first, second, (ascending ? first < second : first >= second)); return ascending ? first < second : first >= second; } public: MySortModel(std::shared_ptr> source_model): slint::SortModel{std::move(source_model), [this](const int& first, const int& second) { return sort(first, second); }} { std::println("MySortModel initialized, ascending={}", ascending); } [[nodiscard]] auto getAscending() const -> bool { return ascending; } void setAscending(bool value) { std::println("Updating sort; {} -> {}", ascending, value); ascending = value; reset(); } }; auto main([[maybe_unused]] int argc, [[maybe_unused]] char** argv) -> int { auto appWindow{AppWindow::create()}; const auto model{std::make_shared>(std::vector{1, 2, 3, 4, 5})}; const auto filterModel{std::make_shared(model)}; const auto sortModel{std::make_shared(filterModel)}; appWindow->set_only_evens(filterModel->getOnlyEvens()); appWindow->on_only_evens_changed([appWindow, filterModel]() { filterModel->setOnlyEvens(appWindow->get_only_evens()); }); appWindow->set_ascending(sortModel->getAscending()); appWindow->on_ascending_changed([appWindow, sortModel]() { sortModel->setAscending(appWindow->get_ascending()); }); appWindow->set_model(sortModel); appWindow->run(); return 0; } ``` The output produced by the `std::println`, is not what one could expect, filtering function is called before initialization of `onlyEvens`, producing wrong results: ``` Filtering with onlyEvens=true; 1 -> false Filtering with onlyEvens=true; 2 -> true Filtering with onlyEvens=true; 3 -> false Filtering with onlyEvens=true; 4 -> true Filtering with onlyEvens=true; 5 -> false MyFilterModel initialized, onlyEvens=false MySortModel initialized, ascending=true Sorting with ascending=true; 4 | 2 -> false Sorting with ascending=true; 4 | 2 -> false ``` A possible workaround is to add a `FilterModel::reset()` invocation in `MyFilterModel::MyFilterModel(std::shared_ptr>)` body, but it will perform twice the computation, and still not prevent UB on first invocation. Could be a good idea to move filtering function invocation at first usage, as is done for `slint::SortModel`.
ogoffart commented 5 months ago

Fixed by #4984 (thank you)