appmenus: update menus when template_for_dispvms changes#74
appmenus: update menus when template_for_dispvms changes#74Jayant-kernel wants to merge 3 commits into
Conversation
|
@ben-grande @andrewdavidwong |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
==========================================
+ Coverage 62.07% 68.01% +5.93%
==========================================
Files 2 2
Lines 683 694 +11
==========================================
+ Hits 424 472 +48
+ Misses 259 222 -37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Looks good. I am unsure if events are enabled for this test, because even other tests are calling Scheduling for openqa. |
@marmarek Please add |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026060915-devel&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026050504-devel&flavor=update
Failed tests143 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/176874#dependencies 29 fixed
Unstable testsDetails
Performance TestsPerformance degradation:27 performance degradations
Remaining performance tests:83 tests
|
|
Passed CI and OpenQA. I tested the code, it is missing the appmenu handling the event and:
|
|
Jayant, are you using QubesOS? Do you need instructions how to test it locally? |
It needs some code on |
|
@ben-grande |
Yes. This PR doesn't fix the issue you mentioned. Please change the commit message from "Fixes" to "For", so it doesn't close the issue when this is merged. About what is missing, is some handling on desktop-linux-menu: #74 (comment), but this PR may be merged before. |
Jayant-kernel
force-pushed
the
fix-template-for-dispvms-menu-update
branch
from
9789d4f to
cedec3b
Compare
|
@ben-grande |
|
PipelineRetry |
|
@marmarek @ben-grande |
|
Changes looks fine. I think it is just a matter of time till Marek can accept it. |
|
@marmarek |
|
Now that you are running Qubes, would you like to give this another try? |
Setting or clearing the template_for_dispvms property did not trigger a menu refresh. This caused the "Disposable:" submenu to appear or disappear only after manually running qvm-appmenus --update. Add a property-set:template_for_dispvms handler in the extension, following the same pattern as the existing label and provides_network handlers. Also add regression tests verifying that appmenus_create correctly creates dispvm entries when the property is enabled, and removes them when it is cleared. For QubesOS/qubes-issues#9194
Jayant-kernel
force-pushed
the
fix-template-for-dispvms-menu-update
branch
from
cedec3b to
9f601d8
Compare
|
Tested locally, unit tests pass . |
|
I don't mean the unit tests, I mean testing the interactions with the
components inside a QubesOS system, and if it behaves as expected.
…On Sat, May 23, 2026, 8:44 AM Jayant ***@***.***> wrote:
*Jayant-kernel* left a comment (QubesOS/qubes-desktop-linux-common#74)
<#74 (comment)>
Tested locally, unit tests pass .
—
Reply to this email directly, view it on GitHub
<#74 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BCE2O4PS4HAARK6RFWLZO7D44FCDLAVCNFSM6AAAAACWKHH22WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKMRUGQ2TENRZGA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| asyncio.ensure_future(self.update_appmenus(vm))) | ||
|
|
||
| @qubes.ext.handler('property-set:template_for_dispvms') | ||
| def template_for_dispvms_setter(self, vm, event, **kwargs): |
There was a problem hiding this comment.
Disposable-related menu entries are supposed to be created only if appmenus-dispvm feature is also set. There is already handling for setting this feature, so if one first sets template_for_dispvms property and then appmenus-dispvm feature, it will regenerate correctly. But if one first sets the feature, and only then the property, indeed regenerating is missing.
But, with the current version of this PR, when the user sets both the feature and the property (as qubes-vm-settings application does), regenerating will happen twice, in parallel, making QubesOS/qubes-issues#10908 worse. This should ensure regenerating is done only when expected entries change. This means:
- here - only if
appmenus-dispvmfeature is already set - in the
appmenus-dispvmfeature handlers (both of them) - iftemplate_for_dispvmsproperty isTrue
|
Regeneration is now gated on both |
Jayant-kernel
force-pushed
the
fix-template-for-dispvms-menu-update
branch
from
46c9866 to
4fd1810
Compare
Jayant-kernel
force-pushed
the
fix-template-for-dispvms-menu-update
branch
from
4fd1810 to
b76065b
Compare
This error most likely is caused by this PR. But I'm not exactly sure where, maybe something in the mockup code that runs on import? |
| def assertUpdateScheduled(self, callback, should_schedule): | ||
| ext = qubesappmenusext.AppmenusExtension() | ||
| ext.collect_done_tasks = unittest.mock.Mock() | ||
| ext.update_appmenus = unittest.mock.Mock(return_value='update-task') |
There was a problem hiding this comment.
Found it - it's here. The AppmenusExtension() module (and other core-admin extensions) have a trap: they are singletons. The object you create (and modify) here, will survive across all tests. If you need to mockup some of its methods, use mock.patch.object, to undo the change after the test.
|
Hmm, you’re right. I was modifying the AppmenusExtension singleton directly in the test helper, so the mocked methods could leak into later tests. I’ve changed it to use scoped mock.patch.object() instead . |
4 participants
Setting or clearing the
template_for_dispvmsproperty on a VM does nottrigger a menu refresh. The "Disposable:" submenu fails to appear (or
disappear) until the user manually runs
qvm-appmenus --update.Add a
property-set:template_for_dispvmshandler inqubesappmenusext/__init__.py, following the same pattern as theexisting
labelandprovides_networkhandlers.Also add two regression tests:
test_008: dispvm entries appear aftertemplate_for_dispvmsis set Truetest_009: dispvm entries are removed aftertemplate_for_dispvmsis clearedFixes QubesOS/qubes-issues#9194