pyapp-kit / app-model Goto Github PK

          Oh while you're here, I noticed that you can't build a `QModelMenu` for submenus because during teardown error results:

see: https://github.com/napari/napari/pull/4865/files#r1288031823

Originally posted by @lucyleeow in #142 (comment)

@lucyleeow, can you make a quick example of this bug? I see the traceback you posted, but it's hard to determine exactly what you ran to get it.

Description

Does QModelMenu have a method similar to _destroy in NapariMenu:

    def _destroy(self):
        """Clean up action data to avoid widget leaks."""
        for ax in self.actions():
            ax.setData(None)

            with contextlib.suppress(AttributeError):
                ax._destroy()
        if self in self._INSTANCES:
            self._INSTANCES.remove(self)

If not, would it be suitable to add it? I think (?) ideally it would perform the setData(None) and the QAction._destroy() methods. (I think QModelMenu save a list of instances in _INSTANCES so I don't think that part would be needed).

My specific use case is for use during Window._teardown() in tests.

cc @DragaDoncila

some more details ... ultimately, I think this bug is due to a failure in the disconnection strategy i mentioned above in #147 (comment).

If you add self.sample_menu.destroyed.connect(lambda: print("DESTROY")) somewhere, you'll see that the destroyed
event IS indeed emitted, but the intended _disconnect callback is never getting called (and so self._app.menus.menus_changed.disconnect(self._on_registry_changed) is never getting called either). I think this probably means that the strategy of having a QObject connect its own method to its destroyed signal isn't going to work (presumably that callback method itself is cleaned up and no longer called).

If you try to outsmart it by using a locally defined function closing on the QObject:

        @self.destroyed.connect
        def _disconnect() -> None:
            self._app.menus.menus_changed.disconnect(self._on_registry_changed)

then the _disconnect function does get called, but then psygnal has an issue actually executing the disconnection cause it can no longer find the callback to disconnect it:

Traceback (most recent call last):
  File "/Users/talley/dev/self/app-model/src/app_model/backends/qt/_qmenu.py", line 58, in _disconnect
    self._app.menus.menus_changed.disconnect(self._on_registry_changed)
  File "src/psygnal/_signal.py", line 827, in disconnect
  File "src/psygnal/_signal.py", line 799, in _slot_index
  File "src/psygnal/_weak_callback.py", line 130, in weak_callback
  File "src/psygnal/_weak_callback.py", line 363, in __init__
  File "src/psygnal/_weak_callback.py", line 190, in __init__
  File "src/psygnal/_weak_callback.py", line 258, in object_key
RuntimeError: wrapped C/C++ object of type QModelSubmenu has been deleted

It's possible we could catch and special case that in psygnal itself (to handle the case where essentially all attribute access is no longer permitted on the previously connected callback)... but I need to look into that.

Originally posted by @tlambert03 in #147 (comment)

A test failed. Please review the workflow logs for details.

We create app using QApplication but then use my_app which is meant to be a app_model.Application

The --pre Test workflow failed on 2023-04-06 12:14 UTC

The most recent failing test was on macos-latest py
with commit: d217ab3

Full run: https://github.com/pyapp-kit/app-model/actions/runs/4628707290

(This post will be updated if another test fails, as long as this issue remains open.)

Add example of using ContextNamespace, ContextKey and create_context to create a class of related context keys.

The --pre Test workflow failed on 2022-11-09 19:44 UTC

The most recent failing test was on windows-latest py
with commit: 4c20668

Full run: https://github.com/napari/app-model/actions/runs/3427958821

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2022-06-29 12:02 UTC

The most recent failing test was on windows-latest py
with commit: 7233dcf

Full run: https://github.com/[object Object]/actions/runs/2582938412

(This post will be updated if another test fails, as long as this issue remains open.)

Should we add the ability store args/kwargs (like a partial) in an Action?

    action1 = Action(id="cmd_id", title="title1", callback=cb)
    action2 = Action(id="cmd_id2", title="title2", callback=cb, kwargs={"a": 1})

or should the callback just be the partial. Need to think about pros cons.
first thought: the "pro" of just using callback=partial(cb, a=1) is that it's familiar regular python. The "con" is that it (currently) would require us to create a new command id for every partial variant. Maybe that's not a big deal

The --pre Test workflow failed on 2023-08-17 12:10 UTC

The most recent failing test was on ubuntu-latest py
with commit: ee121c6

Full run: https://github.com/pyapp-kit/app-model/actions/runs/5890676462

(This post will be updated if another test fails, as long as this issue remains open.)

https://flet.dev/

How to use it with flet?

The --pre Test workflow failed on 2022-06-29 12:05 UTC

The most recent failing test was on windows-latest py
with commit: 7233dcf

Full run: https://github.com/[object Object]/actions/runs/2582938412

(This post will be updated if another test fails, as long as this issue remains open.)

• app-model version: 08d0fbd (Jul 5)
• Python version: 3.10
• Operating System: Mac OS

Description

Order of modifiers can be inconsistant, I think it would be good to always have shortcuts in an order that is consistant, an likely the one the current platform uses.

>>> str(KeyBinding.from_str('Meta+Alt'))
'Meta+Alt'

>>> str(KeyBinding.from_str('Meta+Alt+Shift'))
'Alt+Meta+Shift'

I'm workin on napari UI shortcut editor, and while editing a shortcut if I have Alt-Meta pressed, and pressed shift, it is weird to see Alt and Meta swap places.

In general there seem to be some reordering:

In [2]: str(KeyBinding.from_str('Meta+Alt'))
Out[2]: 'Meta+Alt'  # order unchanged from input

In [3]: str(KeyBinding.from_str('Alt+Meta'))
Out[3]: 'Alt+Meta'  # order unchanged from input

In [4]: str(KeyBinding.from_str('Alt+Meta+Shift'))
Out[4]: 'Alt+Meta+Shift' # order unchanged from input

In [5]: str(KeyBinding.from_str('Meta+Alt+Shift'))
Out[5]: 'Alt+Meta+Shift' # order changed ! from input

So another possibility is to keep the order that was passed in, but I think that would be more complicated.

It's unclear to me why this is happening as the __str__s are pretty straitforward and should keep the order:

    def __str__(self) -> str:
        return " ".join(str(part) for part in self.parts)

    def __str__(self) -> str:
        out = ""
        if self.ctrl:
            out += "Ctrl+"
        if self.shift:
            out += "Shift+"
        if self.alt:
            out += "Alt+"
        if self.meta:
            out += "Meta+"
        if self.key:
            out += str(self.key)
        return out

So Im pretty sure there is a right in my face that I don't see, and it's likely related to having modifiers only shortcuts while starting to type a chord that I'm not seeing.

There is some great info in the src/app_model/expressions/_expressions.py module docstring, which is unfortunately not included in the documentation. Would you consider including this / is there a way to easily do this in the current documentation system?

Also would a list of supported comparisons / ops be useful?

I really like where we are going in terms of making GUI declarative, along the way there are some quite notable things you did leading us here. (magic GUI, manifest, etc.).

I would want to throw this idea out to move this one step forward. Why not declare the GUI as config files instead of python code? Coming from the java world, I liked the spring framework of managing objects (which we are also doing the injection part lightweight via in-n-out, thanks!) And I would say there are additional benefits if we switch to file declaration from code declaration. (and I am in no way trying to say we should follow spring to use XML as the format)

• Developers can then run schema checks without additional tooling
• versioning can be more explicit by marking them out, potentially having better multi-version support
• simplify some verbosity with coding

Take the example app as an example. actions.py can be rewritten to actions.yml. We can probably drop the total amount of text by 50%+

and by having the declaration via a config file, the downstream application can declare a set of things supported instead of being injected directly. For example, the app can also declare in the configuration that "Ctrl is reserved for system use and no one can override," etc.

The --pre Test workflow failed on 2022-06-29 00:05 UTC

The most recent failing test was on macos-latest py
with commit: 7777627

Full run: https://github.com/[object Object]/actions/runs/2579863021

(This post will be updated if another test fails, as long as this issue remains open.)

• app-model version: main (0.1.2)
• Python version: 3.10.10
• Operating System: macOS 13.3

Description

There is a problem instantiating the QModelMenu with PySide 6.5.0. It looks like multiple inheritance gets a bit trickier here as now QWidget is effectively calling super().__init__? This feels like a pretty big change so perhaps it should be reported on the Qt bug tracker.

Try this demo script to see what I mean:

# qwidget_demo.py
from PySide6.QtCore import QObject
from PySide6.QtWidgets import QApplication, QWidget

app = QApplication([])


class A(QObject):
    def __init__(self, *args, **kwargs):
        print("A.__init__", args, kwargs)


class B(QWidget, A):
    def __init__(self, *args, **kwargs):
        QWidget.__init__(self, *args, **kwargs)
        print("B.__init__", args, kwargs)


print("instantiating A")
A()

print("instantiating B")
B()

output with PySide 6.4.2

❯ python qwidget_demo.py 
instantiating A
A.__init__ () {}
instantiating B
B.__init__ () {}

output with PySide 6.5.0

❯ python qwidget_demo.py 
instantiating A
A.__init__ () {}
instantiating B
A.__init__ () {}
B.__init__ () {}

What I Did

Create a virtual environment with the newly released PySide6 v6.5.0:

% python -m venv venv
% source venv/bin/activate
% pip install pyside6==6.5.0
% pip install -e '.[qt,test,test-qt]'
% pytest

Tests are failing for QMenu with this rather confusing output:

self = <[RuntimeError("'__init__' method of object's base class (QModelMenu) not called.") raised in repr()] QModelMenu object at 0x12e46ae00>, menu_id = 'help'
app = Application('complete_test_app'), title = None, parent = None

    def __init__(
        self,
        menu_id: str,
        app: Union[str, Application],
        title: Optional[str] = None,
        parent: Optional[QWidget] = None,
    ):
>       QMenu.__init__(self, parent)
E       TypeError: _MenuMixin.__init__() missing 1 required positional argument: 'app'

src/app_model/backends/qt/_qmenu.py:144: TypeError

Possible solution?

I'm not sure what the best solution is here - I don't have a ton of experience with multiple inheritance or mixins. One thing that seems to work is to give _MenuMixin a dummy __init__ like this, but it doesn't necessarily feel like the right way to do it:

--- a/src/app_model/backends/qt/_qmenu.py
+++ b/src/app_model/backends/qt/_qmenu.py
@@ -44,7 +44,10 @@ class _MenuMixin(QObject):
     _app: Application
     _menu_id: str
 
-    def __init__(
+    def __init__(self, *args, **kwargs):
+        pass
+
+    def _init_mixin(
         self,
         menu_id: str,
         app: Union[str, Application],
@@ -141,8 +144,9 @@ class QModelMenu(QMenu, _MenuMixin):
         title: Optional[str] = None,
         parent: Optional[QWidget] = None,
     ):
+        print(QModelMenu.__mro__)
         QMenu.__init__(self, parent)
-        _MenuMixin.__init__(self, menu_id, app)
+        self._init_mixin(menu_id, app)
         if title is not None:
             self.setTitle(title)
         self.aboutToShow.connect(self._on_about_to_show)
@@ -217,7 +221,7 @@ class QModelToolBar(QToolBar, _MenuMixin):
     ) -> None:
         self._exclude = exclude
         QToolBar.__init__(self, parent)
-        _MenuMixin.__init__(self, menu_id, app)
+        self._init_mixin(menu_id, app)
         if title is not None:
             self.setWindowTitle(title)

I think app-model does not currently support in-n-out inject parameters such as:

• inject : on_unresolved_required_args, on_unannotated_required_args, guess_self
• inject_processors: first_processor_only, raise_exception

I have no current use of this. Just wanted to confirm and know what the future plans were, for some napari app-model documentation (that I will one day write 😬 ).

The --pre Test workflow failed on 2022-07-02 00:02 UTC

The most recent failing test was on ubuntu-latest py
with commit: 75e2e7d

Full run: https://github.com/[object Object]/actions/runs/2599721848

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2022-07-02 00:02 UTC

The most recent failing test was on ubuntu-latest py
with commit: 75e2e7d

Full run: https://github.com/[object Object]/actions/runs/2599721848

(This post will be updated if another test fails, as long as this issue remains open.)

clever implementation

however, nitpick, reversing this does not result in a KeyCombo:

In [1]: from app_model.types import KeyCode, KeyMod

In [2]: KeyMod.CtrlCmd | KeyCode.KeyM
Out[2]: <KeyCombo.CtrlCmd|KeyM: 2078>

In [3]: KeyCode.KeyM | KeyMod.CtrlCmd
Out[3]: 2078

Originally posted by @kne42 in napari/napari#4784 (comment)

The --pre Test workflow failed on 2022-07-03 12:03 UTC

The most recent failing test was on windows-latest py
with commit: 75e2e7d

Full run: https://github.com/[object Object]/actions/runs/2604980680

(This post will be updated if another test fails, as long as this issue remains open.)

• app-model version: 0.2.4
• Python version: 3.8, 3.11
• Operating System: Linux

Description

Autogenerate PR for bump test constraints fails on the test because of the bump to app-model 0.2.4

napari/napari#6478

https://github.com/napari/napari/actions/runs/7320326962/job/19939301830?pr=6478

Test are passing on 0.2.3

A short investigation shows that 0.2.4 introduces the breaking change in #146. Could I ask to yank this release from Pypi and revert this change (or release it as 0.3.0)

• app-model version: 0.1.4
• Python version: 3.10
• Operating System: Mac

Description

I see what you are doing with Keybinding, and trying to make it magically work with strings, but I think it's a footgun in the long run.

What I Did

In [1]: from app_model.types import KeyBinding

In [2]: X = [KeyBinding.from_str('A')]

In [3]: [isinstance(s, str) for s in X]
Out[3]: [False]

In [4]: 'A' in X
Out[4]: True

For me Out[3] logically implies that In [4] Cannot be True. And equal but hash is different is a footgun as well:

In [7]: d = {}

In [8]: d[X[0]] = 1

In [9]: d['A'] # must be 1 as X[0] == 'A' ?

No KeyError.

I think this is make worse by the fact that __str__ return the keybinding in the string form as now code in napari liberally use the object directly in places where it's implicitely str()'d.

A test failed. Please review the workflow logs for details.

• app-model version: latest

Description

I was surprised by the following:

In [1]: str(KeyBinding.from_str('Meta+Shift+Return'))
Out[1]: 'Shift+Meta+'

A realized that Return is not a handled key, should we have a warning/error on invalid values ?

The --pre Test workflow failed on 2024-01-28 00:32 UTC

The most recent failing test was on ubuntu-latest py
with commit: 7f853e6

Full run: https://github.com/pyapp-kit/app-model/actions/runs/7682149944

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2022-07-02 00:04 UTC

The most recent failing test was on windows-latest py
with commit: 75e2e7d

Full run: https://github.com/[object Object]/actions/runs/2599721848

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2023-05-30 12:11 UTC

The most recent failing test was on macos-latest py
with commit: db7aaa9

Full run: https://github.com/pyapp-kit/app-model/actions/runs/5121290967

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2022-07-06 12:02 UTC

The most recent failing test was on windows-latest py
with commit: a0e7a21

Full run: https://github.com/tlambert03/app-model/actions/runs/2622619166

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2023-05-01 12:13 UTC

The most recent failing test was on windows-latest py
with commit: 332d59b

Full run: https://github.com/pyapp-kit/app-model/actions/runs/4851006197

(This post will be updated if another test fails, as long as this issue remains open.)

The --pre Test workflow failed on 2023-02-06 12:13 UTC

The most recent failing test was on ubuntu-latest py
with commit: d87f77a

Full run: https://github.com/pyapp-kit/app-model/actions/runs/4103480073

(This post will be updated if another test fails, as long as this issue remains open.)

in napari, we use a system to render shortcuts with special characters (e.g. the Mac Command symbol for Cmd). I’m on my phone right now so it’s hard for me to show what I mean but I hope you understand the gist

I was thinking that this sort of almost “text rendering” should belong in app-model since I imagine it has a pretty common use case

what do you think @tlambert03? I’d like to work on this but wanted to check first if people like the idea

Currently, Action is frozen and does not allow editing keybinding.
So either, next to default keybinding there should be an option to provide overwrite of keybinding, or builders (like QModelMenu) should accept dict with overwrite of keybindings (action id to keybind like in napari settings).

The third option is to store key bind overwrite in Application instance, as it is already passed and used.

The --pre Test workflow failed on 2023-12-04 00:32 UTC

The most recent failing test was on windows-latest py
with commit: f92ce8d

Full run: https://github.com/pyapp-kit/app-model/actions/runs/7080401696

(This post will be updated if another test fails, as long as this issue remains open.)

might be nice to add a example about QModelMenu ?

Originally posted by @lucyleeow in #142 (comment)

The --pre Test workflow failed on 2022-07-13 00:03 UTC

The most recent failing test was on ubuntu-latest py
with commit: 169f637

Full run: https://github.com/tlambert03/app-model/actions/runs/2660132876

(This post will be updated if another test fails, as long as this issue remains open.)

To avoid depending on injection resolving on menu clicking (see napari/napari#6466) it will be nice to allow providing mapping (like {Viever: viever_instance}) to QModelMenu. As QModelMenu is often glued to a given window, it will not provide any risk, but reduce error probability.

The items passed in dict should be stored as weakref internally and unwrapped on trigger fire.

          Might feel redundant but a processor example below could be nice.

Originally posted by @lucyleeow in #142 (comment)

The --pre Test workflow failed on 2023-06-06 00:39 UTC

The most recent failing test was on windows-latest py
with commit: 5bc0300

Full run: https://github.com/pyapp-kit/app-model/actions/runs/5183267569

(This post will be updated if another test fails, as long as this issue remains open.)

after napari/napari#4784 ... add a CI step to run napari tests whenever app-model is modified

The --pre Test workflow failed on 2022-07-05 12:02 UTC

The most recent failing test was on windows-latest py
with commit: 0a456cd

Full run: https://github.com/[object Object]/actions/runs/2615989973

(This post will be updated if another test fails, as long as this issue remains open.)

There is no findAction method for QModelSubmenu. Should we be using QModelMenu to build submenus? I don't think you findAction on a parent QModelMenu will work for finding submenu actions.

Thanks

The --pre Test workflow failed on 2022-07-07 00:02 UTC

The most recent failing test was on ubuntu-latest py
with commit: f086639

Full run: https://github.com/tlambert03/app-model/actions/runs/2626344898

(This post will be updated if another test fails, as long as this issue remains open.)

• app-model version: Running app_model version 0.1.4
• Python version: 3.11.3
• Operating System: Windos 64

Description

Running the example file results in the following window:

The Menu items are wrongly named.

It looks like app-model doesn't support running in an event loop? For instance, commands can only be executed asynchronously through threads.
Do you think it could support async GUI backends? I'm thinking about Textual in particular.

• app-model version: 0.1.1

• Python version: 3.9

• Operating System: macOS

Description

I am trying to unbind the keybinding from previously binding action, but seems no avail

What I Did

unbinds = []
for keybinding in self._app.keybindings._keybindings:
  if keybinding.command_id == current_action:
  unbinds.append(keybinding)

  print(f"Before {len(self._app.keybindings._keybindings)}")
  print(f'len {len(unbinds)}')
  for unbind in unbinds:
    self._app.keybindings._keybindings.remove(unbind)
  print(f"After {len(self._app.keybindings._keybindings)}")

the print statement correctly reported 4 before, 1 to be dropped, and 3 after. however the old keybinding still seem to be in effect.

looking at the dispose method it seems also doing similar things. is this expected?

just a note here to remind us that napari napari/napari#4784 pins app-model to <0.3.0. The intention is that bumping past 0.3.0 indicates dropping support for napari v0.4.17