Closed
Bug 903913
Opened 11 years ago
Closed 11 years ago
[Flatfish]Need script to detect device type for reuse in shared folder
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(blocking-b2g:koi+)
RESOLVED
FIXED
blocking-b2g | koi+ |
People
(Reporter: gduan, Assigned: gduan)
References
Details
Attachments
(1 file)
Need a script that can detect device type for other apps to use.
Updated•11 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 | ||
Updated•11 years ago
|
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
Assignee | ||
Updated•11 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 | ||
Comment 1•11 years ago
|
||
This patch is to provide a simple script that can tell apps what kinda device we're using.
Attachment #788816 -
Flags: review?(timdream)
Comment 2•11 years ago
|
||
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+
Assignee | ||
Comment 3•11 years ago
|
||
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•11 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•11 years ago
|
Updated•11 years ago
|
blocking-b2g: --- → koi+
Comment 5•11 years ago
|
||
HI George & Tim,
Please take comment 4 into consideration. Thanks.
Flags: needinfo?(gduan)
Updated•11 years ago
|
Blocks: koi-devices
Assignee | ||
Comment 6•11 years ago
|
||
Hi Macro,
Thanks for your comment 4. I will listen mozorientationchange instead of deviceorientation.
Flags: needinfo?(gduan)
Assignee | ||
Comment 7•11 years ago
|
||
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•11 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•11 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•11 years ago
|
Attachment #788816 -
Flags: review?(timdream)
Comment 10•11 years ago
|
||
Discussed offline. Revised patch to come later today.
Flags: needinfo?(timdream)
Assignee | ||
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
Comment on attachment 788816 [details]
PR to master
Thank you! Let's start landing large screen patches :)
Attachment #788816 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Thanks Tim.
Merge into master
https://github.com/mozilla-b2g/gaia/commit/7cfd4679d681a2a231a01ebe7c867b3b562c095c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 14•11 years ago
|
||
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! :-)
Comment 15•11 years ago
|
||
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•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 16•11 years ago
|
||
Hi Ed,
I don't think it's my patch's problem. Should I merge into master again?
Flags: needinfo?(emorley)
Assignee | ||
Comment 18•11 years ago
|
||
Merge into master,
https://github.com/mozilla-b2g/gaia/commit/c8ee5fc7d7247093113eb8f89465e1121c866f3c
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•