Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix name collisions between dataclass fields and guiclass container attributes #688

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ classifiers = [
dynamic = ["version"]
dependencies = [
"docstring_parser>=0.7",
"psygnal>=0.6.1",
"psygnal>=0.8.0",
"qtpy>=1.7.0",
"superqt[iconify]>=0.6.1",
"typing_extensions>=4.6",
Expand Down
4 changes: 2 additions & 2 deletions src/magicgui/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
removed in future releases. Use at your own risk.
"""

from .schema._guiclass import button, guiclass, is_guiclass
from .schema._guiclass import GuiBuilder, button, guiclass, is_guiclass

Check warning on line 7 in src/magicgui/experimental.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/experimental.py#L7

Added line #L7 was not covered by tests

__all__ = ["button", "guiclass", "is_guiclass"]
__all__ = ["GuiBuilder", "button", "guiclass", "is_guiclass"]

Check warning on line 9 in src/magicgui/experimental.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/experimental.py#L9

Added line #L9 was not covered by tests
59 changes: 49 additions & 10 deletions src/magicgui/schema/_guiclass.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from dataclasses import Field, dataclass, field, is_dataclass
from typing import TYPE_CHECKING, Any, Callable, ClassVar, TypeVar, overload

from psygnal import SignalGroup, SignalInstance, evented
from psygnal import SignalGroup, SignalInstance, evented, is_evented
from psygnal import __version__ as psygnal_version

from magicgui.schema._ui_field import build_widget
Expand Down Expand Up @@ -149,13 +149,15 @@
"https://github.com/pyapp-kit/magicgui/issues."
)

setattr(cls, gui_name, GuiBuilder(gui_name, follow_changes=follow_changes))

builder = GuiBuilder(
gui_name,
follow_changes=follow_changes,
events_namespace=events_namespace,
)
if not is_dataclass(cls):
cls = dataclass(cls, **dataclass_kwargs) # type: ignore
cls = evented(cls, events_namespace=events_namespace)

setattr(cls, _GUICLASS_FLAG, True)
setattr(cls, gui_name, builder)
builder.__set_name__(cls, gui_name)
return cls

return _deco(cls) if cls is not None else _deco
Expand Down Expand Up @@ -195,19 +197,56 @@


class GuiBuilder:
"""Descriptor that builds a widget for a dataclass or instance."""
"""Descriptor that builds a widget for a dataclass or instance.

Parameters
----------
name : str, optional
The name of the property that will return a `magicgui` widget.
When used as a descriptor, the name of the class-attribute to which this
descriptor is applied will be used.
follow_changes : bool, optional
If `True` (default), changes to the dataclass instance will be reflected in the
gui, and changes to the gui will be reflected in the dataclass instance.
events_namespace : str
If the owner of this descriptor is not already evented when __set_name__ is
called, it will be be made evented and the events namespace will be set to this
value. By default, this is "events". (see `psygnal.SignalGroupDescriptor`
for details.)
"""

def __init__(self, name: str = "", follow_changes: bool = True):
def __init__(
self,
name: str = "",
follow_changes: bool = True,
events_namespace: str = "events",
):
self._name = name
self._follow_changes = follow_changes
self._owner: type | None = None
self._events_namespace = events_namespace

def __set_name__(self, owner: type, name: str) -> None:
self._name = name
self._owner = owner
if not is_evented(owner):
evented(owner, events_namespace=self._events_namespace)
setattr(owner, _GUICLASS_FLAG, True)

def widget(self) -> ContainerWidget:
"""Return a widget for the dataclass or instance."""
if self._owner is None:
raise TypeError(

Check warning on line 239 in src/magicgui/schema/_guiclass.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/schema/_guiclass.py#L238-L239

Added lines #L238 - L239 were not covered by tests
"Cannot call `widget` on an unbound `GuiBuilder` descriptor."
)
return build_widget(self._owner)

Check warning on line 242 in src/magicgui/schema/_guiclass.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/schema/_guiclass.py#L242

Added line #L242 was not covered by tests

def __get__(
self, instance: object | None, owner: type
) -> ContainerWidget[BaseValueWidget]:
wdg = build_widget(owner if instance is None else instance)
) -> ContainerWidget[BaseValueWidget] | GuiBuilder:
if instance is None:
return self
wdg = build_widget(instance)

# look for @button-decorated methods
# TODO: move inside build_widget?
Expand Down
3 changes: 2 additions & 1 deletion src/magicgui/schema/_ui_field.py
Original file line number Diff line number Diff line change
Expand Up @@ -852,4 +852,5 @@ def _get_values(obj: Any) -> dict | None:
def build_widget(cls_or_instance: Any) -> ContainerWidget[BaseValueWidget]:
"""Build a magicgui widget from a dataclass, attrs, pydantic, or function."""
values = None if isinstance(cls_or_instance, type) else _get_values(cls_or_instance)
return _uifields_to_container(get_ui_fields(cls_or_instance), values=values)
fields = get_ui_fields(cls_or_instance)
return _uifields_to_container(fields, values=values)
27 changes: 15 additions & 12 deletions src/magicgui/widgets/bases/_container_widget.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import contextlib
from collections.abc import Iterable, Mapping, MutableSequence, Sequence
from itertools import chain
from typing import (
TYPE_CHECKING,
Any,
Expand All @@ -22,6 +23,7 @@
from magicgui.widgets.bases._mixins import _OrientationMixin

from ._button_widget import ButtonWidget
from ._named_list import NamedList
from ._value_widget import BaseValueWidget
from ._widget import Widget

Expand Down Expand Up @@ -80,7 +82,7 @@ class BaseContainerWidget(Widget, _OrientationMixin, Sequence[WidgetVar]):
_initialized = False
# this is janky ... it's here to allow connections during __init__ by
# avoiding a recursion error in __getattr__
_list: list[WidgetVar] = [] # noqa: RUF012
_list: NamedList[WidgetVar] = NamedList()

def __init__(
self,
Expand All @@ -91,7 +93,7 @@ def __init__(
labels: bool = True,
**base_widget_kwargs: Unpack[WidgetKwargs],
) -> None:
self._list: list[WidgetVar] = []
self._list: NamedList[WidgetVar] = NamedList()
self._labels = labels
self._layout = layout
self._scrollable = scrollable
Expand All @@ -110,11 +112,14 @@ def __len__(self) -> int:

def __getattr__(self, name: str) -> WidgetVar:
"""Return attribute ``name``. Will return a widget if present."""
for widget in self._list:
if name == widget.name:
return widget
if (wdg := self._list.get_by_name(name)) is not None:
return wdg
return object.__getattribute__(self, name) # type: ignore

def get_widget(self, name: str) -> WidgetVar | None:
"""Return widget with name `name`, or None if one doesn't exist."""
return self._list.get_by_name(name)

@overload
def __getitem__(self, key: int | str) -> WidgetVar: ...

Expand Down Expand Up @@ -429,17 +434,15 @@ def asdict(self) -> dict[str, Any]:

def update(
self,
mapping: Mapping | Iterable[tuple[str, Any]] | None = None,
mapping: Mapping | Iterable[tuple[str, Any]] = (),
**kwargs: Any,
) -> None:
"""Update the parameters in the widget from a mapping, iterable, or kwargs."""
with self.changed.blocked():
if mapping:
items = mapping.items() if isinstance(mapping, Mapping) else mapping
for key, value in items:
getattr(self, key).value = value
for key, value in kwargs.items():
getattr(self, key).value = value
items = mapping.items() if isinstance(mapping, Mapping) else mapping
for key, value in chain(items, kwargs.items()):
if isinstance(wdg := self._list.get_by_name(key), BaseValueWidget):
wdg.value = value
self.changed.emit(self)

@debounce
Expand Down
80 changes: 80 additions & 0 deletions src/magicgui/widgets/bases/_named_list.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
from __future__ import annotations

from typing import TYPE_CHECKING, Generic, TypeVar, overload

if TYPE_CHECKING:
from collections.abc import Iterator, MutableSequence
from typing import Protocol

class Named(Protocol):
name: str


T = TypeVar("T", bound="Named")


class NamedList(Generic[T]):
"""Container that maintains a list of objects with a 'name' attribute,.

Supports list-like insertion and O(1) lookup by name.
"""

def __init__(self) -> None:
self._list: list[T] = []
self._dict: dict[str, T] = {}

def insert(self, index: int, item: T) -> None:
"""Inserts an item at the specified index.

Raises ValueError if an item with the same name already exists.
"""
self._list.insert(index, item)
# NB!
# we don't actually assert name uniqueness here, because it ruins
# the true list-like quality. So, when retrieving by name, you will
# simply get the first item that has been inserted with that name.
if item.name not in self._dict:
self._dict[item.name] = item

def append(self, item: T) -> None:
"""Appends an item at the end of the list."""
self.insert(len(self._list), item)

Check warning on line 41 in src/magicgui/widgets/bases/_named_list.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/widgets/bases/_named_list.py#L41

Added line #L41 was not covered by tests

def get_by_name(self, name: str) -> T | None:
"""Returns the item with the given name, or None if not found."""
return self._dict.get(name)

def remove_by_name(self, name: str) -> None:
"""Removes the item with the given name."""
item = self._dict.pop(name)
self._list.remove(item)

Check warning on line 50 in src/magicgui/widgets/bases/_named_list.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/widgets/bases/_named_list.py#L49-L50

Added lines #L49 - L50 were not covered by tests

@overload
def __getitem__(self, key: int) -> T: ...
@overload
def __getitem__(self, key: slice) -> MutableSequence[T]: ...
def __getitem__(self, index: int | slice) -> T | MutableSequence[T]:
return self._list[index]

@overload
def __delitem__(self, index: slice) -> None: ...
@overload
def __delitem__(self, index: int) -> None: ...
def __delitem__(self, index: int | slice) -> None:
if isinstance(index, slice):
for item in self._list[index]:
self._dict.pop(item.name, None)
del self._list[index]
else:
item = self._list[index]
del self._list[index]
self._dict.pop(item.name, None)

def __len__(self) -> int:
return len(self._list)

def __iter__(self) -> Iterator[T]:
return iter(self._list)

def __repr__(self) -> str:
return repr(self._list)

Check warning on line 80 in src/magicgui/widgets/bases/_named_list.py

View check run for this annotation

Codecov / codecov/patch

src/magicgui/widgets/bases/_named_list.py#L80

Added line #L80 was not covered by tests
Loading