Repository navigation
Notify observers on in-place operators of observable collections - #7086
joaoaracheski wants to merge 10 commits into
Conversation
Fixes flet-dev#6293. ObservableList now handles += and *=, and ObservableDict handles |=; before, these mutated the collection without notifying, and the following attribute assignment of the same object was treated as no change.
|
Thanks! Please add changelog entry. |
|
Done, added the changelog entry under 1.1.3. Thanks again :) |
FeodorFitsner
left a comment
There was a problem hiding this comment.
Thanks for fixing #6293. The Python unit suite passed locally with 513 passed and 9 skipped. I left inline suggestions for preserving collection behavior and notifying once for nested operations, plus a documentation suggestion for synchronous subscribers.
| one copy of the list's items. | ||
| """ | ||
|
|
||
| super().extend([self._wrap(v) for v in it]) |
There was a problem hiding this comment.
Could we keep extend() incremental for ordinary iterables and special-case direct self-extension? Use super().extend(self) when it is self, and keep super().extend(self._wrap(value) for value in it) otherwise. The built-in self-extension preserves the references of existing entries, including plain nested lists from the field initializer, and += can keep delegating to extend(). Please cover a generator that reads items[-1] between yields and a self-extension test that checks the duplicated nested item with is.
| """ | ||
|
|
||
| super().update(*a, **{kk: self._wrap(vv) for kk, vv in k.items()}) | ||
| super().update({kk: self._wrap(vv) for kk, vv in dict(*a, **k).items()}) |
There was a problem hiding this comment.
Could we wrap each incoming value as it is inserted? The temporary dict fully consumes the input before changing the destination. For example, a generator that yields two new keys with len(mapping) as their value sees the same length twice with this implementation. Keep dict.update()'s argument handling by consuming either a mapping's keys or an iterable of pairs incrementally, applying wrapped keyword values last, and notifying once after a successful merge. |= can keep delegating to update(). A generator that reads the destination between yields would make a good regression test.
|
|
||
| super().extend(self._wrap(v) for v in it) | ||
| super().__imul__(n) | ||
| self._touch() |
There was a problem hiding this comment.
A nested augmented assignment currently notifies twice. For example, with a wrapped list at app.mapping["items"], app.mapping["items"] *= 2 notifies from the inner list, then again when Python writes it back through the outer dict's __setitem__. One option is to treat writing the identical observable child back into the same slot as a no-op when both wrappers report to the same owner and field. Apply the list guard to single-index writes only. Please cover nested +=, *=, and |= and the consequent no-op for explicit same-object assignment.
| Extend list in place (`+=`) and notify observers. | ||
| """ | ||
|
|
||
| self.extend(other) |
There was a problem hiding this comment.
These in-place methods notify synchronously before Python writes the returned object back. I verified that a subscriber which replaces app.items with [99] during app.items += [2] has its replacement overwritten by the write-back. Please document this ordering and mention that direct mutator calls such as app.items.extend([2]) preserve a synchronous subscriber's replacement. Preserving the replacement through augmented assignment can be a follow-up since deferring notification changes callback timing.
…ment notification ordering - extend() appends one item at a time again; self-extension uses list.extend(self) to keep references. - update() inserts entries one at a time, wraps values as they are inserted and notifies once. - Writing the same observable child back into its slot (nested +=, *=, |=) no longer notifies twice. - Document that augmented assignments notify before Python writes the result back.
|
Thanks for the detailed review! Addressed all four points: extend() is incremental again with a special case for self-extension, update() inserts entries one at a time and notifies once, writing the same observable child back into its slot is now a no-op, and the notification ordering is documented on @ft.observable. Added tests for each case. I learned a lot from this review. Glad to contribute to Flet! |
…and merge the observable tests
ObservableList.__setitem__ checks whether the value being written is the item's own observable child, which Python writes back after an augmented assignment such as `xs[0] += [1]`. That check read the current item with the raw index, and the assignment then converted the index again. An index object whose `__index__()` has side effects was therefore called twice and could target a different item: with `[10, 20]`, `xs[index] = 99` produced `[10, 99]` where a plain list produces `[99, 20]` (found in a Codex review). The index is now converted once with `operator.index()` and that integer is used for both the check and the assignment. Indexes that aren't integers (e.g. a str or a float) still go straight to `list.__setitem__`, so they raise the same TypeError, with the same message, as a plain list; an out-of-range index raises the same IndexError. Tests: - `test_set_item_reads_custom_index_once`: `__index__()` is called once and the right item changes (fails before this change). - `test_update_reads_mapping_keys`: `update()` takes a mapping's entries from `keys()`, as `dict.update()` does, and wraps the inserted values. - `test_merge_wraps_nested_values` gains an `ior-pairs` case: `|=` with an iterable of key-value pairs.
ObservableList and ObservableDict each had identical copies of `_touch()` (notify the owner under the field name) and `_wrap()` (wrap a nested list or dict before storing it), and the same-child write-back check lived in a module-level `_is_written_back(container, current, value)` function that both classes called. They now inherit from a private `_ObservableCollection` base class that holds all three: - `_touch()` and `_wrap()` move there unchanged; - `_is_written_back(current, value)` becomes a regular method, since it compares the value against the container's own owner and field, so the `container` argument is gone; - `isinstance(value, _ObservableCollection)` replaces the check against the two concrete types. The base class declares empty `__slots__` (plus type hints for `_owner_ref` and `_field`), and each subclass keeps its own `__init__` and `__slots__`, so instances still have no `__dict__`, the constructors keep their signatures (`iterable=` / `mapping=`), and `isinstance(x, list)` / `isinstance(x, dict)` still hold. No behavior change: the existing observable tests (nested in-place operators, same-child write-backs, custom index, notifications) pass unchanged, and pyright and mypy report the same diagnostics on the file as before.
Description
Fixes #6293.
ObservableListoverrides the mutating methods (append,extend,insert...) to notify observers, but not the in-place operators.self.users += [x]calls the built-inlist.__iadd__, which mutates the list without notifying; Python then assigns the same object back to the attribute, andObservable.__setattr__sees no change (same object) and doesn't notify either. So the state changes but the UI isn't refreshed, whileself.users.append(x)works.The same happens with
*=on lists and|=on dicts. This PR adds:ObservableList.__iadd__(+=): delegates toextend(), so items are wrapped and observers notified;ObservableList.__imul__(*=): repeats in place and notifies;ObservableDict.__ior__(|=): delegates toupdate()and notifies.__iadd__takes a snapshot of the right-hand side first, soitems += itemsdoesn't iterate a list that is growing.Test code
Before:
[]After:
['users', 'users', 'tags']Added three tests to
test_observable.py:test_list_inplace_operators_notifyandtest_dict_inplace_union_notifiesfail without the fix and pass with it;test_list_iadd_with_itselfguards theitems += itemscase.Type of change
Checklist
website/sidebars.ymlfor breaking changes, removals, and deprecations, if applicable.Additional details
uv run --group test pytest packages/flet/tests: 509 passed, 8 skipped.ruff checkandruff format --checkare clean on the changed files.#6225 (
use_statewith an updater that mutates in place and returns the same object) is related but not changed here: it follows the same rule as React'suseState, so I left it out of this PR.Suggested changelog entry under Bug fixes, if you'd like me to add it:
Summary by Sourcery
Fix observable collection mutations so in-place list and dictionary operators consistently refresh observers and handle nested updates correctly.
Bug Fixes:
+=,*=, and|=.Enhancements:
Documentation:
Tests: