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

CB-12667 android: Added logic for searching sensors from Samsung vendor #34

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

matrosov-nikita
Copy link
Contributor

Platforms affected

Android

What does this PR do?

This PR adds opportunity for searching orientation sensors which have specific orientation type (it's relevant for Samsung devices).

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@cordova-qa
Copy link

Cordova CI Build has completed successfully.

Commit - Link
Dashboard - Link

66 tests run, 9 skipped, 0 failed.

Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM. Anything else needed to merge this in?

@infil00p
Copy link
Member

@matrosov-nikita Which Samsung devices are affected? All of them? Only the Samsung Galaxy S3/4/5? If we could get more info on that, we can test for this and try and make there there's no regressions.

@matrosov-nikita
Copy link
Contributor Author

@infil00p, sorry for late reply, I've tested a plugin with two Samsung devices:

  1. Samsung Galaxy Core Prime VE Duos SM-G361H (Android 5.1)
    samsung galaxy core prime ve duos sm-g361h
    It has only 'Screen Orientation Sensor' with orientation type 65558
  2. Samsung Galaxy S5 (Android 5.1)
    samsung
    It has two orientation sensors - 'Screen Orientation Sensor'(type=65558) and 'Orientation Sensor'(type=3)

For Samsung Galaxy S5 plugin works perfect. So, not all devices are affected. It depends on what orientation sensors the device has.

@filmaj
Copy link
Contributor

filmaj commented May 1, 2017

A quick Google search reveals that the Note 2 and Note 4 also have this sensor type:

http://www.mattcurry.com/projects-2/the-n-a-o-m-i-project/note4-sensor-output/
https://github.com/hbibel/TapToUnlock/wiki/Sensor-Dumps-of-Different-Devices

Interestingly, the only device I was able to dig up that has the Samsung-specific orientation sensor but not the "normal" orientation sensor is the SM-G361H model @matrosov-nikita points out.

@matrosov-nikita are you able to confirm that this device's Samsung Orientation Sensor works and provides the same kind of data as the regular android Orientation sensor?

@matrosov-nikita
Copy link
Contributor Author

@filmaj, I can confirm that auto and manual tests work w/ my fix.

@petrot
Copy link

petrot commented May 16, 2017

This commit will fix the compass problem on samsung devices? e.g. Samsung Galaxy J5 (2016)

@matrosov-nikita
Copy link
Contributor Author

matrosov-nikita commented May 17, 2017

@petrot, what do you mean 'compass problem'. Do you have any particular cases which don't work for you? I could test them with this fix.

@petrot
Copy link

petrot commented May 17, 2017

One of my user has a Galaxy J5, but as I recognized yesterday, this phone does not have a sensor. So my problem has been solved.. :)

@matrosov-nikita
Copy link
Contributor Author

matrosov-nikita commented Jun 7, 2017

@filmaj, @infil00p, do I need to do something else on this PR?

@filmaj
Copy link
Contributor

filmaj commented Jun 7, 2017

@matrosov-nikita as long as you tested this on one of the real devices affected by the issue, and confirmed the fix, I am good to have this merged.

@filmaj
Copy link
Contributor

filmaj commented Jun 7, 2017

Just realized you don't have committer rights - I will merge this in.

@filmaj
Copy link
Contributor

filmaj commented Jun 7, 2017

@matrosov-nikita can you rebase with latest master first, please?

@matrosov-nikita
Copy link
Contributor Author

@filmaj, done

@asfgit asfgit merged commit a77b4fa into apache:master Jun 7, 2017
@cordova-qa
Copy link

Cordova CI Build has completed successfully.

Commit - Link
Dashboard - Link

66 tests run, 9 skipped, 0 failed.

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.

6 participants