Closed
Bug 1073495
Opened 10 years ago
Closed 9 years ago
Modify activity selection menu, to include a checkbox to remember selection
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Firefox OS Graveyard
Gaia::System
Tracking
(b2g-master fixed)
RESOLVED
FIXED
2.2 S8 (20mar)
Tracking | Status | |
---|---|---|
b2g-master | --- | fixed |
People
(Reporter: davidg, Assigned: fcampo)
References
Details
Attachments
(3 files, 4 obsolete files)
In order to implement feature in bug 1039386: - A 'Use from now on' checkbox needs to be added to the activity selection menu. - When the user checks it, his selection is remembered and he will not be asked again for the same type of activity.
Comment 1•10 years ago
|
||
WIP on: https://github.com/willyaranda/gaia/commit/4dab0dd327822a094a38cdaf90f1491452cfc60d
Reporter | ||
Comment 2•10 years ago
|
||
It still needs tests
Reporter | ||
Updated•10 years ago
|
Attachment #8498806 -
Flags: review?(etienne)
Comment 3•10 years ago
|
||
Comment on attachment 8498806 [details] [review] patch v1 This looks really cool! I just discovered the feature/the spec so maybe I missed some stuff in this first review round but I think we're already in good shape. For new features like this we're pushing strongly for integration tests. Don't know if you're familiar with them [1] yet, but there's always a helping hand available on IRC (:kgrandon, :albertopq and myself to pick three). We should add the following test (in apps/system/test/marionette): * trigger a pick activity (maybe longpress on the homescreen) * check the "remember" option * choose the "Wallpaper" option (we run on b2g-desktop so the camera won't work and the gallery will be empty) * -> check that the wallpaper app is displayed * repeat * -> check that the activity choice list wasn't displayed but we have the wallpaper app directly [1] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_integration_tests
Attachment #8498806 -
Flags: review?(etienne) → feedback+
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8498806 [details] [review] patch v1 Hi Etienne, Thanks for the review. I addressed the comments on github and also added the integration test you suggested. It is my first integration test, so probably I made some mistakes. Could you review it again? Thanks a lot.
Attachment #8498806 -
Flags: review?(etienne)
Comment 5•10 years ago
|
||
Comment on attachment 8498806 [details] [review] patch v1 Hey David, very promising I think we're close. I wont go into details (and I don't know all of them) but the marionette-js framework does a lot of generator magic enabling us to write *much* more linear and readable tests. Have a look around at other tests files to see what I mean, you don't need to nest everything the framework will properly wait before sending the next action. Also feel free to ping :kgrandon or myself on IRC if you have questions while writing the integration tests, but apps/system/test/marionette is usually a great source of inspiration even for tricky cases :)
Attachment #8498806 -
Flags: review?(etienne)
Updated•9 years ago
|
Assignee: david.garciaparedes → fernando.campo
Assignee | ||
Comment 6•9 years ago
|
||
Hey Etienne, I'm taking over David's work and modified the tests you commented on his pull request. I'm assuming you are still the proper reviewer? Please take a look when you have a spare moment Cheers!
Attachment #8498806 -
Attachment is obsolete: true
Attachment #8551364 -
Flags: review?(etienne)
Comment 7•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 Redirecting to Guillaume to get a fresh pair of eyes on it. But feel free to flag me at any time for support!
Attachment #8551364 -
Flags: review?(etienne) → review?(gmarty)
Comment 8•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 Looks all good to me. I'm not overly familiar with this part of code, so you still may want to have another go by someone else. But cool feature!
Attachment #8551364 -
Flags: review?(gmarty) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 so flagging etienne for super after gmarty's comment
Attachment #8551364 -
Flags: superreview?(etienne)
Comment 10•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 There's actually an additional review menu for case like this one (addl. review at the bottom of the list). Anyway, made a few comments on the PR and looks like there is some Gu failures too. I'll make sure the next review round goes quickly :)
Attachment #8551364 -
Flags: superreview?(etienne)
Comment 11•9 years ago
|
||
Oh and we should attach some screenshots to the bug and ask (Eric?) for ui-review. Thanks!
Assignee | ||
Comment 12•9 years ago
|
||
Assuming that the Eric that Etienne mentions is EChang, sorry if wrong person.
Attachment #8572705 -
Flags: ui-review?(echang)
Assignee | ||
Comment 13•9 years ago
|
||
second screenshot, more options, selected checkbox (copied from bug 1039386 attachment 8498163 [details])
Attachment #8572707 -
Flags: ui-review?(echang)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8572705 [details]
DefaultLaunch-icon-unselected
probably better to ask for review to someone from ux instead of QA :D
sorry for the noise!
Attachment #8572705 -
Flags: ui-review?(echang) → ui-review?(epang)
Assignee | ||
Updated•9 years ago
|
Attachment #8572707 -
Flags: ui-review?(echang) → ui-review?(epang)
Comment 15•9 years ago
|
||
Hi Fernando, thanks for flagging me on this, but can you make a few visual updates? 1. Vertically center in space 2. Horizontally center the check box with icons above 3. Left align text with list text above 4. Name 'Use from now on' light italic When ready can you reflag me for review? Thanks!
Flags: needinfo?(fernando.campo)
Assignee | ||
Comment 16•9 years ago
|
||
I went for font-weight: 200 as light, but maybe you prefer to use 300? (400 is the normal value)
Attachment #8572705 -
Attachment is obsolete: true
Attachment #8572707 -
Attachment is obsolete: true
Attachment #8572705 -
Flags: ui-review?(epang)
Attachment #8572707 -
Flags: ui-review?(epang)
Flags: needinfo?(fernando.campo)
Attachment #8573413 -
Flags: ui-review?(epang)
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 soooooooo, failing tests fixed, new added (unit and integration) for the 'reset on app installed' case, ui updated and everyone will be [hopefully] happy :)
Attachment #8551364 -
Flags: review?(etienne)
Comment 18•9 years ago
|
||
(In reply to Fernando Campo (:fcampo) from comment #16) > Created attachment 8573413 [details] > Updated screenshot > > I went for font-weight: 200 as light, but maybe you prefer to use 300? (400 > is the normal value) Thanks Fernando! Let's increase to 300 - it looks a little thin at the moment.
Comment 19•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 Made a lot of comments but only small ones :) So r=me with those addressed, feel free to ping me if anything's unclear.
Attachment #8551364 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 20•9 years ago
|
||
updated to 300, please check again
Attachment #8573413 -
Attachment is obsolete: true
Attachment #8573413 -
Flags: ui-review?(epang)
Attachment #8574467 -
Flags: ui-review?(epang)
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8551364 [details] [review] patch v2 All comments addressed, but I'd rather another pass on it if you have the time, as I had to change a few more tests when going for 'applicationinstall' event instead of triggering the function directly (problems with multiple listeners attached to window). Basically waiting for it to get green, but another look is always welcome
Flags: needinfo?(etienne)
Comment 23•9 years ago
|
||
Comment on attachment 8574467 [details]
updated-font-300
Looks good, thanks for making the change Fernando!
Attachment #8574467 -
Flags: ui-review?(epang) → ui-review+
Assignee | ||
Comment 24•9 years ago
|
||
Last comment addressed, r+'ed and all tests green. Not a bad start for monday :)
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Comment 25•9 years ago
|
||
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/dcb2595f74448f22748ca86e22865f0556d858dd
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
status-b2g-master:
--- → fixed
Target Milestone: --- → 2.2 S8 (20mar)
Comment 26•9 years ago
|
||
Can someone explain what's the difference between "Take picture" and "Take photo"? The first one doesn't seem to match the key name either (pickimage).
Flags: needinfo?(fernando.campo)
Assignee | ||
Comment 27•9 years ago
|
||
My mistake, it was added for debugging (as it's not on the original list) and I forgot to delete it. Also, opencontact is duplicated, and string ids should be different for some of them. Complete list of new strings on bug 1039386 attachment 8522777 [details] page 10 This is what happens when I don't add l10n people to the review process :( Opening a new bug for quick patch, would you mind taking a look :flod?
Flags: needinfo?(fernando.campo)
Comment 28•9 years ago
|
||
IMO we should NOT implement this in system app but in gecko mozApps API or new API for that; definitely not mozSettings to store the manifestURL. In the whole activity implementation, system app does not join the request part but only showing the user the options. It makes no sense to store the selected app manifestURL before, between, and after the activity choices request. A proper implementation should be registering the preference in the mozApps object once gecko sees the application starts and the setMessageHandler is successfully called. Only gecko knows this. It's not guaranteed that an activity is launched means it could handle the request well. Also, the mapping table of activity type + mime type is not scalable and extendible if there are more and more new type activities and new MIME types. Please re-consider to implement this in backend. Jonas, what do you think?
Flags: needinfo?(jonas)
I don't really have a strong opinion on this. The main argument that I can see for putting the implementation of this in gecko is that we know more specifically there which filters and types are used for the given action, and so we have more freedom to know exactly how widely to apply the chosen default. But I don't know what the UX spec for this feature is. If the UX spec can be implemented in the system app then I'm fine with that. But please don't remember a given default-choice for *all* actions. At least it needs to be keyed on the webactivity type that was initiated. Beyond that, I think the choice of where to put the implementation is between Fabrice (who I think owns the Gecko code here) and Alive (who owns the Gaia code). You guys should fight it out: Two men enter... then two men leave after having come to an agreement.
Flags: needinfo?(jonas)
Comment 30•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #29) > But I don't know what the UX spec for this feature is. If the UX spec can be > implemented in the system app then I'm fine with that. This is the spec https://bug1039386.bugzilla.mozilla.org/attachment.cgi?id=8522777, it's included in bug 1039386. > > But please don't remember a given default-choice for *all* actions. At least > it needs to be keyed on the webactivity type that was initiated. I think this was the intention, the spec is aligned with that, but leaving Fernando to confirm it to be 100% sure.
Flags: needinfo?(fernando.campo)
Assignee | ||
Comment 31•9 years ago
|
||
(In reply to Maria Angeles Oteo (:oteo) from comment #30) > (In reply to Jonas Sicking (:sicking) from comment #29) > > But please don't remember a given default-choice for *all* actions. At least > > it needs to be keyed on the webactivity type that was initiated. > > I think this was the intention, the spec is aligned with that, but leaving > Fernando to confirm it to be 100% sure. There's not a option for defaulting an app for all actions, but there's an option to do it for all types on certain action. e.g. not possible to have an app for all the different 'open' activities. but it's possible to have it for all the 'open/image'
Flags: needinfo?(fernando.campo)
You need to log in
before you can comment on or make changes to this bug.
Description
•