USB Portal #210

hfiguiere · 2024-05-20T01:48:45Z

This is a draft, needs a lot of cleanup. Also the portal hasn't landed yet.

But I'm using this to exercise / test the USB portal.

hfiguiere · 2024-05-20T02:34:04Z

Currently includes a revert for #208

hfiguiere · 2024-05-20T03:31:40Z

The build failure is because it needs a flatpak version that isn't available.

src/desktop/usb.rs

ashpd-demo/src/portals/desktop/usb.rs

bilelmoussaoui · 2024-10-25T16:33:30Z

ashpd-demo/src/portals/desktop/usb.rs

+                            #[strong]
+                            page,
+                            async move {
+                                let usb = UsbProxy::new().await.unwrap();


Keep the proxy around

ashpd-demo/src/portals/desktop/usb.rs

bilelmoussaoui · 2024-10-25T16:34:19Z

ashpd-demo/src/portals/desktop/usb.rs

+
+        pub(super) async fn stop_session(&self) -> anyhow::Result<()> {
+            if let Some(session) = self.session.lock().await.take() {
+                session.close().await?;


should switch the buttons state

this is done by the non-imp version which is the one called from the UI.

Signed-off-by: Hubert Figuière <[email protected]>

This allow running the release build. Signed-off-by: Hubert Figuière <[email protected]>

A6GibKm · 2024-12-07T16:06:08Z

ashpd-demo/build-aux/com.belmoussaoui.ashpd.demo.Devel.json

@@ -13,6 +13,8 @@
        "--share=ipc",
        "--socket=wayland",
        "--device=dri",
+        "--own-name=com.belmoussaoui.ashpd.demo",


why is this needed? Isn't this only to own names for app ids which don't match the app id?

I use this build using fenv, and that allow to build and run the non "Devel" version. It's not required for this PR per see (hint it's still in draft).

ashpd-demo/build-aux/com.belmoussaoui.ashpd.demo.Devel.json

A6GibKm · 2024-12-07T16:07:35Z

ashpd-demo/data/resources/ui/usb_device_row.ui

+        <property name="margin-top">6</property>
+        <property name="margin-bottom">6</property>
+        <style>
+          <class name="linked" />


In think the consensus is that we want to avoid linked buttons whenever possible.

link (to the documentation) ?

There is none, ask in the #design:gnome.org room if you would like more context.

ashpd-demo/src/portals/desktop/usb.rs

A6GibKm · 2024-12-07T16:15:10Z

src/desktop/usb.rs

+pub struct UsbDevice {
+    parent: Option<String>,
+    readable: Option<bool>,
+    /// Device is writable. Default is false.


This is an option, please document None. Ditto for readable.

"Default is false". I thought this was clear.

documented readable. (cf the D-Bus xml)

What does None mean? How is it different from false? Why would you get one over the other?

because it's the a{sv} from D-Bus. As per the D-Bus interface it can be omitted so it has to be omittable here.

Signed-off-by: Hubert Figuière <[email protected]>

- Added UI for USB - Added rusb to the crates. - Added libusb to the manifest. Signed-off-by: Hubert Figuière <[email protected]>

bilelmoussaoui · 2024-12-22T09:44:42Z

ashpd-demo/build-aux/com.belmoussaoui.ashpd.demo.Devel.json

@@ -50,14 +62,15 @@
        {
            "name": "ashpd-demo",


I rather have a change that puts the manifest in ./ and see if that helps? :)

bilelmoussaoui · 2024-12-22T09:45:21Z

ashpd-demo/src/portals/desktop/usb.rs

+    }
+}
+
+mod imp {


This doesn't follow the current code style that puts mod imp at the top of the file

bilelmoussaoui · 2024-12-22T09:46:06Z

ashpd-demo/src/portals/desktop/usb.rs

+    impl ActionRowImpl for UsbDeviceRow {}
+
+    #[derive(Debug, gtk::CompositeTemplate, Default)]
+    #[template(resource = "/com/belmoussaoui/ashpd/demo/usb.ui")]


and split different types in a separate mod, see https://github.com/bilelmoussaoui/ashpd/blob/master/ashpd-demo/src/portals/desktop/notification.rs#L161

bilelmoussaoui · 2024-12-22T09:46:56Z

src/desktop/request.rs

@@ -268,7 +268,8 @@ where
        self.0.call("Close", &()).await
    }

-    pub(crate) fn path(&self) -> &ObjectPath<'_> {
+    /// Return the object path of the request.
+    pub fn path(&self) -> &ObjectPath<'_> {


Why was this made public?

bilelmoussaoui · 2024-12-22T09:48:06Z

src/desktop/usb.rs

+pub struct UsbDevice {
+    parent: Option<String>,
+    /// Device can be opened for reading. Default is false.
+    readable: Option<bool>,
+    /// Device can be opened for writing. Default is false.
+    pub writable: Option<bool>,
+    /// The device node for the USB.
+    #[zvariant(rename = "device-file")]
+    pub device_file: Option<String>,
+    /// Device properties
+    pub properties: Option<HashMap<String, OwnedValue>>,
+}


The way we deal with structs in general is that no field is public, only add getters or setters that would be public. This allow us to change the way we deserialize/serialize things without impacting the public API.

bilelmoussaoui · 2024-12-22T09:49:35Z

src/desktop/usb.rs

+#[zvariant(signature = "dict")]
+struct AcquiredDevice {
+    success: bool,
+    fd: Option<OwnedFd>,


So if success = true, do we always receive an fd or not? or if the success = true and fd = None, there are no more fds to acquire?

bilelmoussaoui · 2024-12-22T09:51:00Z

src/desktop/usb.rs

+    #[doc(alias = "FinishAcquireDevices")]
+    async fn finish_acquire_devices(
+        &self,
+        handle_token: &ObjectPath<'_>,


Why is this param called handle_token? is it really one or a request path?

bilelmoussaoui · 2024-12-22T09:52:03Z

src/desktop/usb.rs

+        Ok(Self(proxy))
+    }
+
+    /// Create Session


Please follow the doc style for proxies that are public.

bilelmoussaoui · 2024-12-22T09:53:39Z

src/desktop/usb.rs

+                let mut result = self.finish_acquire_devices(path).await?;
+                let finished = result.1;
+                devices.append(&mut result.0);


Suggested change

let mut result = self.finish_acquire_devices(path).await?;

let finished = result.1;

devices.append(&mut result.0);

let (new_devices, finished) = self.finish_acquire_devices(path).await?;

devices.append(&new_devices);

?

bilelmoussaoui · 2024-12-22T09:55:51Z

src/desktop/usb.rs

+}
+
+/// Device to acquire
+pub struct Device(pub String /* ID */, pub bool /* writable */);


This needs a constructor/getters

bilelmoussaoui · 2024-12-22T09:56:17Z

src/desktop/usb.rs

+
+/// USB error for acquiring device.
+#[derive(Debug)]
+pub struct UsbError(pub Option<String>);


Needs to implement std::error::Error

hfiguiere force-pushed the usb-portal branch 3 times, most recently from fced97d to ecb8189 Compare May 20, 2024 02:33

hfiguiere force-pushed the usb-portal branch 3 times, most recently from 4d81103 to de86287 Compare May 20, 2024 03:10

hfiguiere force-pushed the usb-portal branch from 239d19c to 1f7eec3 Compare September 22, 2024 21:36

bilelmoussaoui reviewed Oct 25, 2024

View reviewed changes

hfiguiere force-pushed the usb-portal branch 3 times, most recently from 3aee673 to 9146631 Compare November 23, 2024 07:33

hfiguiere force-pushed the usb-portal branch 2 times, most recently from 7eadba8 to 7697720 Compare December 7, 2024 03:06

hfiguiere added 2 commits December 7, 2024 08:08

Use the crate from the repo

217383b

Signed-off-by: Hubert Figuière <[email protected]>

flatpak: Allow the base bus name in flatpak

1dc8063

This allow running the release build. Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the usb-portal branch from 7697720 to f7c76db Compare December 7, 2024 15:26

A6GibKm reviewed Dec 7, 2024

View reviewed changes

hfiguiere force-pushed the usb-portal branch 2 times, most recently from 5d2cff3 to 85d3611 Compare December 7, 2024 16:46

hfiguiere added 2 commits December 7, 2024 12:54

desktop: Added usb portal

c31a08f

Signed-off-by: Hubert Figuière <[email protected]>

demo: Added USB portal support

15d92c9

- Added UI for USB - Added rusb to the crates. - Added libusb to the manifest. Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the usb-portal branch from 85d3611 to 15d92c9 Compare December 7, 2024 17:54

bilelmoussaoui reviewed Dec 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USB Portal #210

USB Portal #210

hfiguiere commented May 20, 2024

hfiguiere commented May 20, 2024

hfiguiere commented May 20, 2024

bilelmoussaoui Oct 25, 2024

bilelmoussaoui Oct 25, 2024

hfiguiere Nov 23, 2024

A6GibKm Dec 7, 2024

hfiguiere Dec 7, 2024

A6GibKm Dec 7, 2024

hfiguiere Dec 7, 2024

A6GibKm Dec 7, 2024

A6GibKm Dec 7, 2024

hfiguiere Dec 7, 2024

hfiguiere Dec 7, 2024

A6GibKm Dec 7, 2024

hfiguiere Dec 7, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

bilelmoussaoui Dec 22, 2024

USB Portal #210

Are you sure you want to change the base?

USB Portal #210

Conversation

hfiguiere commented May 20, 2024

hfiguiere commented May 20, 2024

hfiguiere commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment