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

feat: attach/detach bound devices in tray context menu #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JPHutchins
Copy link
Contributor

This PR adds a "nice to have" feature. Generally, I think that the GUI is great and it would be a mistake to put too much functionality in the system tray. Instead, users should be encourage dto open the main window where operations will be less ambiguous.

However, my day-to-day usage rarely involves inspecting unbound devices, binding devices, or setting up auto attach. I have 2 or 3 bound devices and I either want them attached or not.

And so, this PR adds the connected + bound devices in the context menu. A check mark is displayed next to devices that are forwarded. Clicking on a device either attaches or detaches depending on the state.

image

I am new to Rust and this was hard for me to do. 😢 I am concerned about memory leaks. I have already resolved a stack overflow caused by the event handler closure not being unbound. It was getting added to the stack on every context menu open:

if let Some(handler) = self.menu_tray_event_handler.borrow().as_ref() {
    nwg::unbind_event_handler(handler);
}

I've tested the current implementation quite a bit now and it seems OK, but it could be totally wrong and unsafe - I simply don't have enough experience with Rust to know! Nevertheless, I think the feature is a good one.

Unaddressed issues:

  • No handling of devices that are auto attach. Ideally they would be greyed out since attaching/detaching is pointless.
  • Windows annoying lag during USB enumeration. I've seen this before, it's not a Rust problem. During attach, the context menu will become unresponsive, presumably because the thread is "busy" doing the attach. Would be nice to let the user know what's happening somehow but IMO it's not too bad.

Copy link
Owner

@nickbeth nickbeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change so I'll accept it. However there's some changes needed before I merge this.

src/gui/usbipd_gui.rs Outdated Show resolved Hide resolved
src/gui/connected_tab/mod.rs Outdated Show resolved Hide resolved
src/gui/usbipd_gui.rs Outdated Show resolved Hide resolved
src/gui/usbipd_gui.rs Outdated Show resolved Hide resolved
src/gui/usbipd_gui.rs Outdated Show resolved Hide resolved
src/gui/usbipd_gui.rs Show resolved Hide resolved
src/gui/usbipd_gui.rs Outdated Show resolved Hide resolved
src/gui/usbipd_gui.rs Show resolved Hide resolved
src/usbipd.rs Outdated Show resolved Hide resolved
src/gui/usbipd_gui.rs Outdated Show resolved Hide resolved
@JPHutchins JPHutchins force-pushed the feature/device-list-in-tray branch from fb73bb0 to b4eae05 Compare November 10, 2024 22:03
@JPHutchins JPHutchins requested a review from nickbeth November 10, 2024 22:06
Copy link
Owner

@nickbeth nickbeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've actually gone and tested this PR, it's a really neat feature!

There's one more issue though: errors while attaching are not reported to the user. We should really spawn a dialog box with the error message, see how it's done in other parts of the attaching code.

Additionally, I've noticed that since attaching takes quite some time, once you click on a device in the menu for attaching it, the menu disappears and it won't open up until the attaching is completed. Now, I'm not even sure how to easily fix this, as every way I come up with requires complex state tracking, so maybe it's fine?

src/gui/usbipd_gui.rs Show resolved Hide resolved
@JPHutchins
Copy link
Contributor Author

There's one more issue though: errors while attaching are not reported to the user. We should really spawn a dialog box with the error message, see how it's done in other parts of the attaching code.

Agreed! I think that I can cause an error by opening the menu, unplugging the device, then attempting to forward it. And trying to forward with WSL shutdown.

@JPHutchins
Copy link
Contributor Author

Additionally, I've noticed that since attaching takes quite some time, once you click on a device in the menu for attaching it, the menu disappears and it won't open up until the attaching is completed. Now, I'm not even sure how to easily fix this, as every way I come up with requires complex state tracking, so maybe it's fine?

Yeah, it's the same for the main window. I hate threads and state, so my vote is to keep it simple. I see this whenever interacting with Windows USB. Their APIs must block for 200ms - 2s during attach/detach events.

@JPHutchins JPHutchins force-pushed the feature/device-list-in-tray branch from b4eae05 to a3ae37a Compare November 11, 2024 22:40
@JPHutchins JPHutchins requested a review from nickbeth November 11, 2024 22:52
@JPHutchins
Copy link
Contributor Author

I've been running this branch from morning to evening everyday at work for the last month - haven't noticed any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants