Skip to content

Conversation

@bwikbs
Copy link
Member

@bwikbs bwikbs commented Feb 1, 2021

Initial release.

@bwikbs bwikbs requested a review from a team February 1, 2021 02:29
Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

You may also add these lines to https://github.com/flutter-tizen/plugins/blob/master/README.md (after testing the plugin on watch/watch emulator/TV/TV emulator):

  • Under "List of packages" (insert after "battery_tizen")

    | [**connectivity_tizen**](packages/connectivity) | [connectivity](https://github.com/flutter/plugins/tree/master/packages/connectivity) (1st-party) | [![pub package](https://img.shields.io/pub/v/connectivity_tizen.svg)](https://pub.dev/packages/connectivity_tizen) | No |
  • Under "Device limitations"

    | [**connectivity_tizen**](packages/connectivity) | ✔️ | ✔️ | ✔️ | ✔️ |
    

Looks good otherwise!

Comment on lines 13 to 14
connectivity: ^0.4.9
connectivity_platform_interface: ^1.0.3
Copy link
Member

Choose a reason for hiding this comment

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

The latest connectivity plugin version is 2.0.2 and the platform interface version is 1.0.6. Would it be better to update the dependencies before connectivity_tizen is published on pub.dev for the first time?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. It was updated with latest version.

@@ -0,0 +1,18 @@
// You have generated a new plugin project without
Copy link
Member

Choose a reason for hiding this comment

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

I guess this file was added accidentally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.


This plugin is supported on these types of devices:

- Galaxy Watch (running Tizen 4.0 or later)
Copy link
Member

Choose a reason for hiding this comment

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

Please specify if any privilege is required to use this plugin in README. There's an example in the checklist or https://github.com/flutter-tizen/plugins/tree/master/packages/path_provider#required-privileges.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Just a nitpick but it would be better to remove the bullet (-) and indentation since they are redundant.

@bwikbs bwikbs marked this pull request as draft February 2, 2021 05:06
@bwikbs bwikbs force-pushed the connectivity branch 2 times, most recently from 19c81fc to 66d528d Compare February 2, 2021 05:25
@bwikbs
Copy link
Member Author

bwikbs commented Feb 2, 2021

You may also add these lines to https://github.com/flutter-tizen/plugins/blob/master/README.md (after testing the plugin on watch/watch emulator/TV/TV emulator):

  • Under "List of packages" (insert after "battery_tizen")
    | [**connectivity_tizen**](packages/connectivity) | [connectivity](https://github.com/flutter/plugins/tree/master/packages/connectivity) (1st-party) | [![pub package](https://img.shields.io/pub/v/connectivity_tizen.svg)](https://pub.dev/packages/connectivity_tizen) | No |
  • Under "Device limitations"
    | [**connectivity_tizen**](packages/connectivity) | ✔️ | ✔️ | ✔️ | ✔️ |
    

Looks good otherwise!

My TV environment has some problem, so I couldn't check at TV. I'll find a way and it will be updated soon.

@bwikbs bwikbs marked this pull request as ready for review February 2, 2021 05:30
@swift-kim
Copy link
Member

The example app worked as expected on TV emulator, but on mobile and wearable emulators, the status always showed "wifi" (even when they were in flight mode). Maybe it's a defect in the system library or SDK.

@bwikbs
Copy link
Member Author

bwikbs commented Feb 4, 2021

The example app worked as expected on TV emulator, but on mobile and wearable emulators, the status always showed "wifi" (even when they were in flight mode). Maybe it's a defect in the system library or SDK.

It's pretty OK at TM1, TW3. I think it's a bug of emulator.

@swift-kim
Copy link
Member

Then we may mark this plugin as ❌ or ⚠️ in the device limitations section ("Watch emulator" column) in README.md. You can revert your changes to README.md if we are going to update it later at once.

void registerObsever(std::unique_ptr<flutter::EventSink<flutter::EncodableValue>> &&events)
{
ensureConnectionHandle();
if (connection_set_type_changed_cb(m_connection, connetionTypeChangedCB, this) != CONNECTION_ERROR_NONE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure but did you set clang_format_style as Google?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@bwikbs bwikbs force-pushed the connectivity branch 2 times, most recently from ceb16a4 to 58e82dd Compare February 9, 2021 00:12
@swift-kim swift-kim merged commit ade86c9 into flutter-tizen:master Feb 9, 2021
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.

3 participants