devices widget: open file manager on dispvm block attach#300
devices widget: open file manager on dispvm block attach#300Sahilll10 wants to merge 1 commit intoQubesOS:mainfrom
Conversation
|
Suggestion: If you intend for this pull request to resolve the associated issue and would like for it to be linked to the issue automatically, you can replace |
| if self.device.device_class == 'block': | ||
| try: | ||
| new_dispvm.run_service("qubes.StartApp+qubes-open-file-manager") | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
I linked to an example providing the exception: QubesOS/qubes-issues#10709 (comment)
There was a problem hiding this comment.
Please reread the link I pointed above. It is not broad Exception, it is a custom exception for Qubes. Follow the links and you will find it.
| if self.device.device_class == 'block': | ||
| try: | ||
| new_dispvm.run_service("qubes.StartApp+qubes-open-file-manager") | ||
| except Exception: # pylint: disable=broad-except |
There was a problem hiding this comment.
|
Also see CI, black is complaining. |
137e262 to
70c63f8
Compare
|
Hey @ben-grande, pushed an update addressing both your review points:
Also fixed the issue link to Fixes QubesOS/qubes-issues#10709 as @andrewdavidwong suggested. Please let me know is anything else needs adjustment. |
70c63f8 to
f7a000f
Compare
|
Hey @ben-grande Thanks for pointing that out - and sorry for missing it earlier. I've now updated both handlers to catch Hopefully this change now matches what you intended to be fixed. I’m still learning and improving with each iteration, so I really appreciate your guidance and feedback during the review process. Please let me know if any further adjustments are needed . |
| ) | ||
| except exc.QubesException: | ||
| logger.exception( | ||
| "Failed to open file manager in %s", |
There was a problem hiding this comment.
Please read the example above: QubesOS/qubes-issues#10709 (comment)
The code needs to log the exception also, to let the user know what happened.
There was a problem hiding this comment.
Also, you already have a logger in new_dispvm.log.
| ) | ||
| except exc.QubesException: | ||
| logger.exception( | ||
| "Failed to open file manager in %s", |
There was a problem hiding this comment.
|
Hi @ben-grande Updated both handlers to use new_dispvm.log.exception() and capture as ex so the full exception detail is logged. Thanks for the guidance. Please let me know if further adjustments are required. |
|
Latest commit looks fine: 94a89fe. Please squash the commits. Do you have a Qubes machine to test it? |
94a89fe to
cfbdc62
Compare
|
Hi @ben-grande I have squashed into a single commit and cleaned up the message. Sir I don't currently have a dedicated Qubes machine. I've been working from the codebase and tracing patterns in the existing code. I'm working on a standard Linux machine for coding currently . I understand that's a real gap and I'm working on setting up a proper Qubes environment. Happy to make any further adjustments |
| from . import backend | ||
| import time | ||
|
|
||
| logger = logging.getLogger(__name__) |
| import asyncio | ||
| import functools | ||
| import pathlib | ||
| import logging |
There is no need to "Sir" me. "Ben" is fine. |
cfbdc62 to
b8fbc63
Compare
|
Hi @ben-grande I’ve removed the extraneous import logging and logger = logging.getLogger(name) lines and amended the existing commit to keep the history clean. Please let me know if any further adjustments are needed. |
Black complains. Please wait for the CI to pass or to be broken beyond your fixing (something unrelated to this PR), to request a review. |
|
Please test with the MockQubesComplete. Although no file manager will open, it will log the call. Let me know how far this setup without QubesOS allows you to test GUI things out. |
b8fbc63 to
81787aa
Compare
|
Marek, CI is failing due to unexpected call to |
|
I think the code looks fine. Added openqa label. |
The Details
|
|
I have confirmed that the test_policy_exceptions_handler.py failures are pre-existing on main and completely unrelated to this PR. Identical results on both: On this PR (commit 81787aa): exact same 9 failures, no new failures introduced. Regarding MockQubesComplete testing — the mock initialized successfully. No block devices exist in the mock by default so run_service is never triggered, but the device_class == "block" code path is confirmed correct. |
Git bisect on core-admin-client pointed to QubesOS/qubes-core-admin-client@84edd42. For git bisect run, I used: I did not expect that for the |
|
Created another issue for the broken tests: QubesOS/qubes-issues#10770. |
|
Hi @ben-grande I have submitted a fix for the broken tests in The fix registers the per-VM admin.vm.List call inside _add_to_vm_list so all MockQube instances automatically get it without per-test duplication. Please let me know if further adjustments are required. |
Hi @ben-grande PR QubesOS/qubes-core-admin-client#450 addresses this. The fix registers the per-VM admin.vm.List call inside MockQube._add_to_vm_list, so all mock VMs automatically get it without per-test duplication — which is what you hoped for in the issue. WITHOUT fix: 9 failed, 2 passed All test_policy_exceptions_handler tests now pass cleanly |
|
PipelineRetryFailed |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
==========================================
- Coverage 92.99% 91.29% -1.70%
==========================================
Files 64 66 +2
Lines 13312 13837 +525
==========================================
+ Hits 12379 12632 +253
- Misses 933 1205 +272 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixing my phrase above: Although no file manager will NOT open, it will log the call. Hi. Can you test this before I send to OpenQA? https://doc.qubes-os.org/en/r4.3/developer/general/developing-gui-applications.htm |
81787aa to
ac7c01f
Compare
|
Hi @ben-grande I have tested with MockQubesComplete. Added tests in qui/devices/tests/test_actionable_widgets.py: test_block_device_opens_file_manager — verifies run_service("qubes.StartApp+qubes-open-file-manager", wait=False) is called when a block device is attached to a dispvm All 5 tests pass locally: As expected, no actual file manager opens since there is no real Qubes machine — but the run_service call is confirmed correct via mock assertions. Please let me know if further changes are needed. |
Please wait for CI before asking for a review: |
e343d25 to
1ad3a9d
Compare
|
Hi @ben-grande The remaining checks:tests failure is test_global_config_init — a GTK abort deep inside rule_list_widgets.py and gtk_widgets.py. This PR does not touch either of those files. The three files changed in this PR are: qui/devices/actionable_widgets.py The crash traceback points to gtk_widgets.py:450-rule_list_widgets.py:115 — both untouched by this PR. The test also passes locally with the same codebase, suggesting this is an Xvfb/GTK environment instability in CI rather than a regression introduced here. The pylint and black checks are now both passing cleanly. |
|
PipelineRetryFailed |
|
Hi @ben-grande Apologies for being off topic. I had previously reached out on the Qubes-devel mailing list regarding my interest in the System Health Monitor project for the upcoming GSoC . Since the proposal submission phase has now started, I wanted to follow up and see if it might be possible to briefly discuss the direction of the project idea and any technical considerations that I should keep in mind while preparing my proposal. My goal is to make sure that the proposal aligns well with the project's expectations and architecture. Ben where can we discuss it? |
I think that the |
|
Changes looks good. I haven't tested yet. Can you address disabling this change if the user wants to: QubesOS/qubes-issues#10709 (comment). Should it be enabled by default? |
marmarta
left a comment
There was a problem hiding this comment.
This is just a little bit of nitpicking about the tests. I like the PR in general.
| @pytest.fixture | ||
| def mock_qapp(): | ||
| app = MockQubesComplete() | ||
| app.update_vm_calls() |
There was a problem hiding this comment.
That shouldn't be needed? The Mock object updates its call on creation. This is needed only when you're adding something to the object (e.g. a new qube) or changing some properties.
| return device | ||
|
|
||
|
|
||
| def make_mock_vm(qubes_app): |
There was a problem hiding this comment.
So, I don't think this is 100% wrong - it definitely works - but a more future-resilient way is using the existing mocks and overwriting only the method you want to verify. E.g. here you can use any of the existing mock vms in the MockQubesComplete object and later when you want to check that run_service was called, just patch the run_service object. That approach should be more resilient to later changes in the code, while your approach means that potentially if some part of the code changes and e.g. asks for more VM properties, this mock has to be re-done.
1ad3a9d to
2cb88ac
Compare
When a block device is attached to a new disposable via the devices widget, automatically open the file manager inside the dispvm using the qubes.StartApp+qubes-open-file-manager qrexec service. Only triggers for block device class. Uses new_dispvm.log for logging, consistent with existing patterns in the codebase. Fixes QubesOS/qubes-issues#10709
2cb88ac to
9fee2cf
Compare
|
Addressed both review comments: Feature flag : Added FEATURE_OPEN_FILE_MANAGER = "device-open-file-manager-on-attach" constant to backend.py. Enabled by default — user can opt out by setting the feature to an empty string on the appvm. Both AttachDisposableWidget and DetachAndAttachDisposableWidget check self.vm.vm_object.features.get(backend.FEATURE_OPEN_FILE_MANAGER, "1") before calling run_service. Test refactor : Removed redundant app.update_vm_calls(). Updated make_mock_vm to use qubes_app._qubes["test-vm"] as vm_object directly — uses the real MockQube instance including its features dict. The checks:pylint failure is a CI infrastructure issue unrelated to this PR. The job fails at pip3 install -r ci/requirements.txt while building pygobject — gobject-introspection-1.0 is missing from the runner environment. Could you trigger a pipeline retry? |
Opens the file manager automatically in the DispVM when a block
device is attached via the devices widget.
Uses the qubes.StartApp+qubes-open-file-manager qrexec service,
consistent with OpenFileManagerItem in qui/tray/domains.py.
Only triggers for block device class.
Fixes: QubesOS/qubes-issues#10709