fix: Fix recursion check in Qt6

[hanjinliu]

The current implementation of the menu context updates for Qt6 (the "FIXME" part) will skip all the nested submenus. For example, the enablement expression of an action at File > XXX > Run YYY will be ignored (= always enabled).
This PR fixes this problem by using the _skip: QModelMenu argument to avoid running _update_from_context on the same menu. I checked that this works on my personal app, but I don't know if it is safe in other use cases.

I think whether submenus are updated properly should be included in the test, but I cannot fully understand the FullApp pytest fixture (such as when each action is supposed to be enabled). Could you please help me with adding this test if it's reasonable to be added?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a0d95b1) to head (b59383c).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #232   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        31           
  Lines         1916      1914    -2     
=========================================
- Hits          1916      1914    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tlambert03]

looking at my note again, it looks like I was working around this limitation:

QAction has lost the .menu() method, and it's a bit hard to find how to get to the parent menu now.

it looks like that was true in Qt 6.1 ... probably when I was adding support for this. But the .menu() method looks to have been returned in Qt6.2+

Can you check to see if simply removing that not QT6 clause now works?

    for action in actions:
        if isinstance(action, QMenuItemAction):
            action.update_from_context(ctx)
        elif isinstance(menu := action.menu(), QModelMenu):
            menu.update_from_context(ctx)

perhaps we don't need to be careful about recursion on the .parent() at all anymore

[hanjinliu]

I didn't know the menu() method returned! Yes, it worked as expected.
According to the tests, it looks PyQt6 6.2.3 does not support it yet. We have to explicitly drop the support of this version.

[tlambert03]

I think whether submenus are updated properly should be included in the test, but I cannot fully understand the FullApp pytest fixture (such as when each action is supposed to be enabled). Could you please help me with adding this test if it's reasonable to be added?

I agree that it should be tested, but if I understand your problem statement that " the enablement expression of an action at File > XXX > Run YYY will be ignored (= always enabled).", it appears to already be tested right here:

app-model/tests/test_qt/test_qmenu.py

Lines 89 to 103 in a0d95b1

	menu.update_from_context({"something_open": False, "friday": True})
	assert submenu.isVisible()
	assert submenu.isEnabled()
	menu.update_from_context({"something_open": False, "friday": False})
	assert submenu.isVisible()
	assert not submenu.isEnabled()
	menu.update_from_context({"something_open": True, "friday": False})
	# assert not submenu.isVisible()
	assert not submenu.isEnabled()
	menu.update_from_context({"something_open": True, "friday": True})
	# assert not submenu.isVisible()
	assert submenu.isEnabled()

those tests are asserting that the enablement of a submenu does change based on the context provided to the parent menu. can you help determine what might be different about the case you found that was broken>?

[hanjinliu]

it appears to already be tested right here

sorry if I didn't make it clear, but what I meant in File > XXX > Run YYY is that the QAction named "Run YYY", not a submenu. Those lines are testing the enablement of QMenus (I hope I understand it correctly). The problem I had in Qt6 is that all the QActions under the nested menus were always enabled.

[tlambert03]

ok, I added a test that I think tests that in b59383c (it checks the enablement of an action inside of submenu is still controlled by the context). However, it also passes on main, without this PR ... so I'm still not sure I'm hitting the case you found?

[hanjinliu]

Thank you for making the test!
Did you tested it locally? I copied the file content to my main branch (forked after the last commit) and my pytest fails exactly in the assert not submenu_action.isEnabled() line.

OS: Windows 11
Python 3.11.9
Qt versions:

pyqt6                         6.8.0
pyqt6-qt6                     6.8.1
pyqt6-sip                     13.9.1
pytest-qt                     4.4.0
qtpy                          2.4.2

[tlambert03]

Ok great! I must have made a mistake when I tried to test it on main. Thanks