Added adapter.rs #32

NivedithaShankar · 2019-03-30T20:39:56Z

@Sruthi-Kannan @Harsha1908

This is the first step in converting bluetoothAdapter from enum to trait.

We converted enum bluetoothAdapter (in bluetooth.rs) to trait using a structure in the new adapter.rs file. We wanted to make sure we are doing the right thing before proceeding to the other steps. We are yet to integrate this change in the servo crate.

Let us know if we have to make changes to this

This change is

jdm · 2019-03-31T01:10:59Z

If you add pub mod adapter; to lib.rs, you will see a number of new compile errors that need to be addressed.

jdm

Three significant changes that should eliminate a bunch of the current errors being reported.

jdm · 2019-04-02T02:04:07Z

src/adapter.rs

+use bluetooth::BluetoothDevice;
+
+pub trait BluetoothAdapter {
+    fn new()-> Result<Box<BluetoothAdapter>, Box<Error>>;


This new method cannot be part of the trait, since it causes a compile error. Instead use the solution I described in a previous email.

From my understanding of the email, I changed the init() functions to new() as your solution had the following,

impl BluetoothAdapter {
#[cfg(all(target_os = "macos", feature = "bluetooth"))]
pub fn new() -> Result<Box, Box> {
// ...
}
This will allow callers to use BluetoothAdapter::new() to create a new
instance.

The code snippet I wrote in the email is different than what you have here. The snippet implements a bunch of per-platform static methods named new on all BluetoothAdapter trait objects. The code in this PR adds a static new method to the BluetoothAdapter trait, which then must be part of each concrete implementation of the trait, and the compiler does not like that.

Hi Josh,
We modified the code as per your suggestion and we're still facing some issues.
One of the errors that we are getting is, 'no associated item named Empty found for type dyn adapter::BluetoothAdapter in the current scope'. (Compiling on Windows).

impl BluetoothAdapter {
#[cfg(not(any(all(target_os = "linux", feature = "bluetooth"),
all(target_os = "android", feature = "bluetooth"),
all(target_os = "macos", feature = "bluetooth"))))]
pub fn new() -> Result<Box, Box> {
let adapter = try!(BluetoothAdapterEmpty::init());
Ok(BluetoothAdapter::Empty(Arc::new(adapter)))
}
}
Could you give suggestions to fix this issue?

You need to use Ok(Box::new(adapter)) to return the type that is required.

jdm · 2019-04-02T02:04:46Z

src/adapter.rs

+    fn get_device_id(&self) -> Result<u32, Box<Error>>;
+    fn get_modalias(&self) -> Result<(String, u32, u32, u32), Box<Error>>;  
+}
+pub struct Bluez(Arc<BluetoothAdapterBluez>);


Each of the per-platform implementations should not be public, and will need a #[cfg(...)] block to ensure they are only visible to the compiler on appropriate platforms.

May I also add a suggestion? I see that there's a lot of methods on this struct which basically delegates back to self.0.method(), so is it possible to instead implement Deref<Target = BluetoothAdapterBluez> instead of delegating methods?

That exposes the per-platform types to the caller crate. I prefer to have the trait as a layer of indirection to prevent that, and expose any cross-platform API issues.

jdm · 2019-04-02T02:06:52Z

src/adapter.rs

+
+
+    fn create_discovery_session(&self) -> Result<BluetoothDiscoverySession, Box<Error>> {
+        BluetoothDiscoverySession::create_session(self.clone())


Every API like this that current takes a BluetoothAdapter argument will need to be modified. We will need to inline the android branch of BluetoothDiscoverySession::create_session in this method instead (ie. create a `BluetoothDiscoverySessionBluez object directly here).

jdm · 2019-04-08T16:35:32Z

src/adapter.rs

+    }
+
+    #[cfg(feature = "bluetooth-test")]
+    pub fn new() -> Result<Box<BluetoothAdapter>, Box<Error>> {


Since we can have both the bluetooth and the bluetooth-test feature enabled simultaneously, let's call this method new_mock instead.

Thank you for the feedback!
We have not added a structure for Mock(Arc yet. Bluetooth-test has additional methods such as set_id, set_address and so on.
We wanted to confirm if those additional methods can be added in impl BluetoothAdapter{ } instead and have the common methods such as get_id(), get_address().. in impl BluetoothAdapter for Mock{ }.

Mmm, that's interesting. I didn't notice that before. Here is what we should do - add those methods to trait BluetoothAdapter and add default implementations that return an Err value. Then for the implementation for the mock adapter, override the defaults with the real implementations. Does that make sense?

Yes! Thank you for the clarification. I think we got it. We will make the changes and let you know.

src/lib.rs

src/adapter.rs

jdm

This looks very close!

jdm · 2019-04-22T23:27:49Z

src/adapter.rs

+
+    #[cfg(feature = "bluetooth-test")]
+    pub fn new_mock() -> Result<Box<FakeBluetoothAdapter>, Box<Error>> {
+        Ok(Box::new_empty())


This line isn't right. It should be calling Box::new(...) with an instance of FakeBluetoothAdapter, and the return type should be Box<BluetoothAdapter>.

When I tried to fix this by making the changes you suggested, it says trait bluetoothadapter is not implemented for blurmac : : FakebluetoothAdapter (But I cannot make changes to that file right?)

The changes we made for this method in adapter.rs was,
pub fn new_mock() -> Result<Box<BluetoothAdapter, Box> {
let fake = FakeBluetoothAdapter::new_empty();
Ok(Box::new(fake))
}

Or are we approaching it the wrong way?

Oh, I see what's happening. Like the struct Mac, struct Android, and struct Bluez, you need a struct that wraps the FakeBluetoothAdapter so you can implement the BluetoothAdapter trait for it, and return that.

My mistake; there already is a struct Mock. That's what you should be returning inside of the Box.

Okay so if I do that, then I have to pass an instance of Mock on Box::new(...) right?
Because it now expects struct 'adapter : : Mock'

src/adapter.rs

jdm · 2019-04-22T23:30:12Z

src/adapter.rs

+
+    fn create_discovery_session(&self) -> Result<BluetoothDiscoverySession, Box<Error>> {
+        //BluetoothDiscoverySession::create_session(self.clone())
+        //let test_session = FakeBluetoothDiscoverySession{};


Remove these commented out lines.

jdm · 2019-04-22T23:31:04Z

src/bluetooth.rs

-            },
-        }
-    }
-
    #[cfg(feature = "bluetooth-test")]
    pub fn create_mock_device(adapter: BluetoothAdapter, device: String) -> Result<BluetoothDevice, Box<Error>> {


This method will complain if you run cargo build --features bluetooth-test.

I was not aware of where create_mock_device method is being called. I was thinking if I should remove it?

https://github.com/servo/servo/blob/f1dd31f70440fa9c7a40525bd1e03eede568f74d/components/bluetooth/test.rs#L153

Isn't this another API that uses 'adapter: BluetoothAdapater' as a parameter? So it expects a trait but, tries to match with enum variants. We faced a similar issue for BluetoothDevice and BluetoothDiscoverySession, and as you suggested, we inlined the functions in adapter.rs itself. So, should I write this function in adapter.rs itself and remove it from bluetooth.rs?

jdm · 2019-04-22T23:32:27Z

Don't forget to check out the logs for TravisCI - there are some compile errors you still need to fix.

jdm · 2019-04-25T16:02:16Z

src/empty.rs


-    pub fn get_device_list(&self) -> Result<Vec<String>, Box<Error>> {
-        Err(Box::from(NOT_SUPPORTED_ERROR))
+impl BluetoothAdapter for EmptyAdapter {


Given

devices/src/adapter.rs

Lines 454 to 459 in 4aee4f1

struct Empty(Arc<BluetoothAdapterEmpty>);

#[cfg(not(any(all(target_os = "linux", feature = "bluetooth"),

all(target_os = "android", feature = "bluetooth"),

all(target_os = "macos", feature = "bluetooth"))))]

impl BluetoothAdapter for Empty{

I think we can probably revert these changes. Sorry for the confusion :( It's hard to keep track of what has been changed and what has not at this point.

Previous comment used to contain a double negative: "I don't think we can". It has been updated.

Oh! Do you want me to bring empty.rs to its previous implementation?
=> Change the structs name alone from BluetoothAdapter to EmptyAdapter
=> Keep the rest as it was(without the block impl BluetoothAdapter for EmptyAdapter)
Have one whole block called impl EmptyAdapter with all methods

Yes, I think it should work fine without any of those changes. There are enough moving parts in this pull request that we should reduce any unnecessary changes.

src/adapter.rs

jdm · 2019-04-25T17:45:26Z

Assuming no more compile errors, I think these changes will be ready to merge. For the purposes of your project, the only remaining work that is required is making changes to Servo to use these new changes.

Sruthi-Kannan · 2019-04-25T22:33:16Z

Assuming no more compile errors, I think these changes will be ready to merge. For the purposes of your project, the only remaining work that is required is making changes to Servo to use these new changes.

Thank you for the reply Josh.
Is this PR ready to be merged? If yes, should I do something with respect to the commits we made?

jdm · 2019-04-26T01:15:09Z

Please squash them into one commit.

adapter added Squashing commits Update adapter.rs Fixed minor errors Fixed compiler errors for trait BluetoothAdapter Fixed errors in Mac struct of BluetoothAdapter Added bluetooth-test functionality Added bluetooth test as you suggested. Could you check if it is fine? Removed enum type BluetoothAdapter Removed create_session and create_device functions Fixed issues in bluetooth-test Also made changes to APIs that take adapter as a parameter Implemented trait for EmptyAdapter Renamed struct from BluetoothAdapter to EmptyAdapter and implemented trait BluetoothAdapter for the same. This was done to make fn new() work in adapter.rs(for Empty) Fixed errors in few methods Made changes to fn new(), fn new_mock() and methods that use self.0.clone() Moved create_mock_device to adapter.rs Reverted changes in empty.rs Modified fn new() for Empty Modified fn new() for Mac, Android and Bluez Squashed 2 commits Squashed 2 more commits Squashed 7 commits Squashed 14 commits Squashed 16 commits Squashed Squashed commits Final Commit Final squashed commit

NivedithaShankar · 2019-04-26T03:17:08Z

Before you merge this PR, I was wondering if these changes would affect the main servo repository. There is code in the main repo that requires BluetoothAdapter as an enum. Would merging these changes lead to errors there?

jdm · 2019-04-26T03:54:04Z

No. The version of this repository that is used is controlled by this line. It requires running cargo update -p device to change the revision that is in use.

jdm · 2019-04-26T03:56:20Z

@bors-servo r+

bors-servo · 2019-04-26T03:56:21Z

📌 Commit 0fd6566 has been approved by jdm

bors-servo · 2019-04-26T03:56:25Z

⌛ Testing commit 0fd6566 with merge 328014f...

@Sruthi-Kannan

Added adapter.rs @Sruthi-Kannan @Harsha1908 This is the first step in converting bluetoothAdapter from enum to trait. We converted enum bluetoothAdapter (in bluetooth.rs) to trait using a structure in the new adapter.rs file. We wanted to make sure we are doing the right thing before proceeding to the other steps. We are yet to integrate this change in the servo crate. Let us know if we have to make changes to this  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/devices/32)

bors-servo · 2019-04-26T03:58:54Z

☀️ Test successful - checks-travis
Approved by: jdm
Pushing 328014f to master...

jdm suggested changes Apr 2, 2019

View reviewed changes

jdm suggested changes Apr 8, 2019

View reviewed changes

jdm suggested changes Apr 13, 2019

View reviewed changes

src/adapter.rs Outdated Show resolved Hide resolved

src/adapter.rs Outdated Show resolved Hide resolved

src/adapter.rs Outdated Show resolved Hide resolved

src/adapter.rs Outdated Show resolved Hide resolved

src/adapter.rs Outdated Show resolved Hide resolved

jdm suggested changes Apr 22, 2019

View reviewed changes

jdm reviewed Apr 25, 2019

View reviewed changes

src/adapter.rs Outdated Show resolved Hide resolved

src/adapter.rs Outdated Show resolved Hide resolved

NivedithaShankar force-pushed the master branch 2 times, most recently from 980ee53 to 92f652f Compare April 26, 2019 03:12

NivedithaShankar force-pushed the master branch from 92f652f to 0fd6566 Compare April 26, 2019 03:14

bors-servo merged commit 0fd6566 into servo:master Apr 26, 2019



		fn create_discovery_session(&self) -> Result<BluetoothDiscoverySession, Box<Error>> {
		BluetoothDiscoverySession::create_session(self.clone())

	struct Empty(Arc<BluetoothAdapterEmpty>);

	#[cfg(not(any(all(target_os = "linux", feature = "bluetooth"),
	all(target_os = "android", feature = "bluetooth"),
	all(target_os = "macos", feature = "bluetooth"))))]
	impl BluetoothAdapter for Empty{

Added adapter.rs #32

Added adapter.rs #32

Conversation

NivedithaShankar commented Mar 30, 2019 • edited by larsbergstrom

jdm commented Mar 31, 2019

jdm left a 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

jdm Apr 2, 2019 • edited

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

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NivedithaShankar Apr 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm Apr 25, 2019 • edited

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

jdm commented Apr 22, 2019

jdm Apr 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NivedithaShankar Apr 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Apr 25, 2019

Sruthi-Kannan commented Apr 25, 2019

jdm commented Apr 26, 2019

NivedithaShankar commented Apr 26, 2019

jdm commented Apr 26, 2019 • edited

jdm commented Apr 26, 2019

bors-servo commented Apr 26, 2019

bors-servo commented Apr 26, 2019

bors-servo commented Apr 26, 2019

NivedithaShankar commented Mar 30, 2019 •

edited by larsbergstrom

jdm Apr 2, 2019 •

edited

NivedithaShankar Apr 24, 2019 •

edited

jdm Apr 25, 2019 •

edited

jdm Apr 25, 2019 •

edited

NivedithaShankar Apr 25, 2019 •

edited

jdm commented Apr 26, 2019 •

edited