Closed Bug 964601 Opened 6 years ago Closed 5 years ago

[Devices][MTP][Gaia] Add USB Storage panel with MTP/UMS selection


(Firefox OS Graveyard :: Gaia::Settings, defect)

Gonk (Firefox OS)
Not set



2.1 S3 (29aug)
feature-b2g 2.1


(Reporter: iliu, Assigned: gasolin)



(Whiteboard: [ft:devices][p=5])


(2 files)

According to the user story in Bug 922927, we have to implement following items for reaching the user story.

1. If USB storage is disabled, grey out "Share using USB" toggle button.(This is an improving item for interaction.)
2. Remove "USB storage toggle button" from  Settings::Media storage.
3. Let the MTP is always enabled.(Gaia is able to control the flag while platform is ready to support.)
For the item 3, it will need MTP supporting via platform. Then, it's able to toggle MTP enabled/disabled via settings key.
Depends on: 748350
feature-b2g: --- → 2.1
Assignee: nobody → iliu
Whiteboard: [ft:Devices]
Whiteboard: [ft:Devices] → [ft:devices]
Target Milestone: --- → 2.1 S3 (29aug)
Summary: [Devices][Gaia] Media Transfer Protocol → [Devices][MTP][Gaia] Media Transfer Protocol
Duplicate of this bug: 1052885
Component: General → Gaia::Settings
No longer depends on: 748350
In transfer protocol page, will set

ums.mode : 1 -> UMS
ums.mode : 3 -> MTP

based on bug 1043264 comment 6
Ian will handle origin media storage related change in bug 943825. I'd help make new usb_storage panel

The new spec is:

1. add new link left of USB storage switch
2. add a protocol selection panel contains (mtp / ums) (set mtp by default)
3. change mode based on protocol selection result
Assignee: iliu → gasolin
Summary: [Devices][MTP][Gaia] Media Transfer Protocol → [Devices][MTP][Gaia] Add USB Storage panel with MTP/UMS selection
WIP for gecko test, need refactor and unit test
I would have thought that UMS should be set by default, since that's what we've been using in the past.

Making MTP be the default seems a bit premature right now, especially since its new, and we know it doesn't work anywhere near as reliably as UMS.
ni jenny(UX) to clarify Dave's comment 6 about why set MTP by default.

From tech perspective, ni alphan, if Dave and you feel UMS is better fit for current release, we should do that and fire followup bug to change default to MTP for future release.
Flags: needinfo?(jelee)
Flags: needinfo?(alchen)
In my opinion, if there is UMS support, it is good to set UMS as default.
UMS is more stable and faster than MTP.
As Dave said, UMS can work well on anywhere but MTP can't.
Flags: needinfo?(alchen)
If there's technical concerns, I'm ok with using UMS as default. However, when MTP is stabilized in the future, we should switch back to MTP. Thank you!
Flags: needinfo?(jelee)
Comment on attachment 8475742 [details] [review]
pull request redirect to github

set feedback flag to get some feedback before unit tests are done.
Attachment #8475742 - Flags: feedback?(ejchen)
Comment on attachment 8475742 [details] [review]
pull request redirect to github

F+ for settings part. Please check my comments on Github ! Thanks ;)
Attachment #8475742 - Flags: feedback?(ejchen) → feedback+
Comment on attachment 8475742 [details] [review]
pull request redirect to github

Thanks EJ for feedback. Unit test added. Set review for system part.

will set settings review after add the split menu UI.
Attachment #8475742 - Flags: review?(alive)
Comment on attachment 8475742 [details] [review]
pull request redirect to github

split menu added, @ej please help review it. Thanks!
Attachment #8475742 - Flags: review?(ejchen)
Attached image screenshot
set UI review
Attachment #8478152 - Flags: ui-review?(hhuang)
Comment on attachment 8478152 [details]

Hi Fred, it looks good! Thank you!
Attachment #8478152 - Flags: ui-review?(hhuang) → ui-review+
Comment on attachment 8475742 [details] [review]
pull request redirect to github

r+ with nits
Attachment #8475742 - Flags: review?(alive) → review+
Comment on attachment 8475742 [details] [review]
pull request redirect to github

Thanks Fred, r+ with nits on Github.

Attachment #8475742 - Flags: review?(ejchen) → review+
Thanks for review. I've addressed your comments, will fix TBPL test issue before land.
Whiteboard: [ft:devices] → [ft:devices][p=5]
Confirmed with EM/EPM, and this can be landed before FL.

Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1059135
Blocks: 1061090
Depends on: 1070137
Depends on: 1073433
Depends on: 1169821
You need to log in before you can comment on or make changes to this bug.