[Flatfish]Need script to detect device type for reuse in shared folder

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gduan, Assigned: gduan)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+)

Details

Attachments

(1 attachment)

Need a script that can detect device type for other apps to use.

Updated

5 years ago
Summary: [Flatfish] Need script to detect device type in shared folder → [Flatfish] Need script to detect device type for reuse in shared folder
Assignee: nobody → gduan
Summary: [Flatfish] Need script to detect device type for reuse in shared folder → [Flatfish] Need script to detect device type in shared folder
Summary: [Flatfish] Need script to detect device type in shared folder → [Flatfish]Need script to detect device type for reuse in shared folder
Created attachment 788816 [details]
PR to master


This patch is to provide a simple script that can tell apps what kinda device we're using.
Attachment #788816 - Flags: review?(timdream)
Comment on attachment 788816 [details]
PR to master

r+, Let's not land this patch to master until we have it settled on the product side.
Attachment #788816 - Flags: review?(timdream) → review+
Hi Tim,

I update my patch for below reasons.
1. if screen is 1280x800 and app is opened in portrait mode, then my original isLarge may return us wrong value.
2. add isPortrait and isLandscape api
3. add orientationObserver
4. function name and file name may cause some confusion, so I update the name as screen_layout

Could you kindly check again? thanks.
Flags: needinfo?(timdream)

Comment 4

5 years ago
Hi all,

Bug 788975 tried to use accelerometer sensor to detect screen rotation because 
  1. some low end device didn't have orientation sensor.
  2. Orientation sensor consumed more power then accelerometer sensor.

So if you use deviceorientation to calculate the screen orientation then it will not work on some low end device. (but auto rotation is workable.)

Updated

5 years ago
blocking-b2g: --- → koi+

Comment 5

5 years ago
HI George & Tim,

Please take comment 4 into consideration. Thanks.
Flags: needinfo?(gduan)
Hi Macro,

Thanks for your comment 4. I will listen mozorientationchange instead of deviceorientation.
Flags: needinfo?(gduan)
Comment on attachment 788816 [details]
PR to master

Hi Tim,

I think I should remove isPortrait, isLandscape, and orientationObserver, since mozorientationchange event already do a great job on it and also window.screen.orientation which tell us portrait/landscape with primary/secondary.

However, I still made some change on this patch. Considering app is opened in portrait mode under 1280x800 screen, the original api will not tell correct result, so I choose maximum value of height and width to identify device type.

Could you kindly review again? Thanks.
Attachment #788816 - Flags: review+ → review?(timdream)

Comment 8

5 years ago
In last week offline discussion, George and I decide to not use maxWidth (get max value from width/height) here because current break point (768px from bootstrap) is sufficient to distinguish for most bigger device(ex 1080x800 or 1200x800). Either 1200, 1080, or 800 exceed 768px, thus generally tablet will always be detected as 'medium' device.

Comment 9

5 years ago
Comment on attachment 788816 [details]
PR to master

looks good to me, except need to fix a typo shown in github comment
Attachment #788816 - Flags: feedback+

Updated

5 years ago
Blocks: 903915
Discussed offline. Revised patch to come later today.
Flags: needinfo?(timdream)
Comment on attachment 788816 [details]
PR to master

Hi Tim,

This patch has added getCurrentLayout method and rename addQueryWatcher/removeQueryWatcher as watch/unwatch and also update some comment. Please kindly review it again. Thanks.
Attachment #788816 - Flags: review?(timdream)
Comment on attachment 788816 [details]
PR to master

Thank you! Let's start landing large screen patches :)
Attachment #788816 - Flags: review?(timdream) → review+
Thanks Tim.

Merge into master
https://github.com/mozilla-b2g/gaia/commit/7cfd4679d681a2a231a01ebe7c867b3b562c095c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Backed out for breaking the build, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27111057&tree=B2g-Inbound
(and yeah logs are pretty useless lol)

https://github.com/mozilla-b2g/gaia/commit/e343634ff8077cce1343294dc5d6c5fe586c5668

Please test locally before pushing! :-)
This might have been false blame by the gaia commit robot cooincidentally pushing at around the same time (turns out we don't use gaia.json properly all the time), and the bustage here might have been caused by bug 904068.

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi Ed,

I don't think it's my patch's problem. Should I merge into master again?
Flags: needinfo?(emorley)
Yup :-)
Flags: needinfo?(emorley)
Merge into master,
https://github.com/mozilla-b2g/gaia/commit/c8ee5fc7d7247093113eb8f89465e1121c866f3c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.