Conversation
| assert(m_calculator); | ||
| } | ||
|
|
||
| virtual T get() const override { return m_calculator(); } |
There was a problem hiding this comment.
I don't believe callbacks in reactivity API are the best solution. We'll have to define them every time we want to bind value with state, won't we?
There was a problem hiding this comment.
There is an Observable class which should be used in most trivial cases (where value is stored as is in the widget). It is not tested yet though. CalculatedObservable exists for cases where this is not possible, as for TextEditor, which stores its content as vector of lines, so we need to calculate the resulting content every time we need it.
|
|
||
| // Read-only wrapper for Observable. Expose this if you want to make | ||
| // some field public but read only | ||
| template<std::copyable T> class ReadOnlyObservable { |
There was a problem hiding this comment.
Observables should always be read only. User is supposed to change widget parameters, not its access classes
There was a problem hiding this comment.
What about CheckBox::checked which can be written programatically or switched by user, so it needs to be both Observable (to allow reading it) and writable (to allow changing state)
There was a problem hiding this comment.
But yeah, most observables are read only :)
| Read& operator=(Reader&& t) { | ||
| assert(t); | ||
| // TODO: Deregister observer? | ||
| assert(!m_observing); |
There was a problem hiding this comment.
Maybe make asserts only for debug???
There was a problem hiding this comment.
assert(t); ensures that you don't pass a null std::function which is unfortunately possible in C++
assert(!m_observing) is because otherwise Read would be observing an Observable and a function at the same, leading to unpredictable behavior (in most cases taking value from Function, but sometimes from Observer when it runs notify).
Note: Eventually we should deregister observers in destructor to avoid UAF
| T get() const { | ||
| return std::visit( | ||
| Util::Overloaded { | ||
| [&](std::monostate) -> T { ESSA_UNREACHABLE; }, |
There was a problem hiding this comment.
How is this even possible to return template result from ESSA_UNREACHABLE in lambda expression while std::monostate is in variant?
There was a problem hiding this comment.
ESSA_UNREACHABLE always crashes, as leaving an empty Read is almost certainly a programming error (forgetting to initialize it with either value, function or Observable)
There was a problem hiding this comment.
hmm, maybe we should require user to initialize Read with at least default-constructed value... ? or default constructed Read would be default constructed value (so e.g empty string). E.g. currently it crashes when commenting out line 15 in examples/essa-gui/reactivity.cpp because of non-bound Read
| bool m_content_changed = false; | ||
| bool m_multiline = true; | ||
|
|
||
| CalculatedObservable<Util::UString> m_content_observable; |
There was a problem hiding this comment.
I'd add helper template base class, just define observable type in inheritance and call its constructor
There was a problem hiding this comment.
For this purpose there is a Observable<T> type. Here I need to use CalculatedObservable<T> because content is internally stored as vector of lines
It is not well tested yet and I suspect that there is a use after return somewhere there
2 participants
THis commit introduces classes that allow you to observe variable changes. This works only for copyable objects. Reactive states (Observable/Read) are typically immutable, but this can be "bypassed" with CalculatedObservable and manual notify() calls as done in TextEditor.
Eventually this API should replace all on_change callbacks, in worst case you can manually register handler with
observable.observe(handler)This is a draft as a "simple"
Observableis not tested yet, only ReadOnlyObservable and CalculatedObservable.TODO: