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

[HapticFeedback] Implement vibrate() #75

Conversation

pwasowski2
Copy link

@pwasowski2 pwasowski2 commented Apr 26, 2021

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:

  HapticFeedback.vibrate();

Signed-off-by: Pawel Wasowski [email protected]

HapticFeedback.vibrate() was one of the features to implement on this list #53 (comment)

Before building with this change, be sure to use tizen_tools with this commit applied: https://github.com/flutter-tizen/tizen_tools/pull/10

The change will only work on devices that support it. I have tested it on a Samsung Z3 (aka TM1) smartphone with Tizen 6.5.

@pwasowski2 pwasowski2 marked this pull request as ready for review April 26, 2021 15:27
@pwasowski2 pwasowski2 mentioned this pull request Apr 26, 2021
12 tasks
shell/platform/tizen/BUILD.gn Outdated Show resolved Hide resolved
shell/platform/tizen/channels/platform_channel.cc Outdated Show resolved Hide resolved
[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

Signed-off-by: Pawel Wasowski <[email protected]>
HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

Signed-off-by: Pawel Wasowski <[email protected]>
@pwasowski2
Copy link
Author

As this is my first change in flutter-tizen/engine, I'm not sure about the design of my patch. I've tried to follow the contributor's guidelines from the original flutter/engine repo, but I'd like to know your opinion about some things :)

  1. I've put FeedbackManager class implementation in platform_channel.cc. Honestly, I consider it to be a little bit of a code smell, and would rather put this class in separate files (standard .h and .cc combination for declaration and definitions), but I didn't know where to put these files in the repo's source tree. Do you think, I can put them (i.e. feedback_manager.h and feedback_manager.cc) directly in src/flutter/shell/platform/tizen/channels?
  2. From what I've seen, Flutter has no strict rules regarding formulation of error messages. I've tried to make my messages meaningful - have I got it right?

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.

I've put FeedbackManager class implementation in platform_channel.cc. Honestly, I consider it to be a little bit of a code smell,

I personally don't mind this.

Do you think, I can put them (i.e. feedback_manager.h and feedback_manager.cc) directly in src/flutter/shell/platform/tizen/channels?

Yes, you can. You can also refer to other platform implementations and find the best way you think.

From what I've seen, Flutter has no strict rules regarding formulation of error messages. I've tried to make my messages meaningful - have I got it right?

Printing meaningful error messages is always a good thing and I think you're doing right. You should use FT_LOGE rather than FT_LOGD to print your error to the tool's log console. The flutter-tizen tool only prints log messages with the log level INFO or above. There's no other rule for formatting log messages AFAIK.

shell/platform/tizen/BUILD.gn Outdated Show resolved Hide resolved
shell/platform/tizen/channels/platform_channel.cc Outdated Show resolved Hide resolved
- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>
shell/platform/tizen/BUILD.gn Outdated Show resolved Hide resolved
shell/platform/tizen/channels/platform_channel.cc Outdated Show resolved Hide resolved
Copy link

@pkosko pkosko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@bbrto21 bbrto21 left a comment

Choose a reason for hiding this comment

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

LGTM

@bbrto21 bbrto21 merged commit 81ff054 into flutter-tizen:flutter-2.0.1-tizen May 11, 2021
swift-kim pushed a commit that referenced this pull request Jun 7, 2021
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

Signed-off-by: Pawel Wasowski <[email protected]>

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

Signed-off-by: Pawel Wasowski <[email protected]>

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Refactor FeedbackManager

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Use FT_LOGE for all error logs
@swift-kim swift-kim mentioned this pull request Jul 14, 2021
swift-kim pushed a commit that referenced this pull request Sep 27, 2021
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

Signed-off-by: Pawel Wasowski <[email protected]>

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

Signed-off-by: Pawel Wasowski <[email protected]>

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Refactor FeedbackManager

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Use FT_LOGE for all error logs
swift-kim pushed a commit that referenced this pull request Nov 14, 2021
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

Signed-off-by: Pawel Wasowski <[email protected]>

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

Signed-off-by: Pawel Wasowski <[email protected]>

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Refactor FeedbackManager

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Use FT_LOGE for all error logs
swift-kim pushed a commit that referenced this pull request Dec 9, 2021
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

Signed-off-by: Pawel Wasowski <[email protected]>

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

Signed-off-by: Pawel Wasowski <[email protected]>

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Refactor FeedbackManager

Signed-off-by: Pawel Wasowski <[email protected]>

* [HapticFeedback] Use FT_LOGE for all error logs
swift-kim pushed a commit that referenced this pull request Dec 17, 2021
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

* [HapticFeedback] Refactor FeedbackManager

* [HapticFeedback] Use FT_LOGE for all error logs

Signed-off-by: Pawel Wasowski <[email protected]>
swift-kim pushed a commit that referenced this pull request Feb 7, 2022
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

* [HapticFeedback] Refactor FeedbackManager

* [HapticFeedback] Use FT_LOGE for all error logs

Signed-off-by: Pawel Wasowski <[email protected]>
swift-kim pushed a commit that referenced this pull request Feb 11, 2022
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

* [HapticFeedback] Refactor FeedbackManager

* [HapticFeedback] Use FT_LOGE for all error logs

Signed-off-by: Pawel Wasowski <[email protected]>
swift-kim pushed a commit that referenced this pull request May 12, 2022
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

* [HapticFeedback] Refactor FeedbackManager

* [HapticFeedback] Use FT_LOGE for all error logs

Signed-off-by: Pawel Wasowski <[email protected]>
swift-kim pushed a commit that referenced this pull request Aug 5, 2022
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

* [HapticFeedback] Refactor FeedbackManager

* [HapticFeedback] Use FT_LOGE for all error logs

Signed-off-by: Pawel Wasowski <[email protected]>
swift-kim pushed a commit that referenced this pull request Sep 1, 2022
* [HapticFeedback] Implement vibrate()

[Before] HapticFeedback.vibrate() was not implemented

[After] The code below will cause a short vibration:
      HapticFeedback.vibrate()

* Make HapticFeedback.vibrate's dependencies dependent on profile

HapticFeedback.vibrate will only work on mobile and wearable profiles so
its dependencies should only be included in builds for these devices.

* Make FeedbackManager a singleton

* [HapticFeedback.vibrate] Minfor fixes

- use the proper "MOBILE_PROFILE" and "WEARABLE_PROFILE" names instead of
  "MOBILE" and "WEARABLE"
- reorder lines in BUILD.gn

* Refactor implementation of HapticFeedback.vibrate() and BUILD.gn

* [HapticFeedback] Refactor FeedbackManager

* [HapticFeedback] Use FT_LOGE for all error logs

Signed-off-by: Pawel Wasowski <[email protected]>
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.

4 participants