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)

defect
Not set
normal

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.
Assignee: nobody → david.garciaparedes
Blocks: 1039386
Depends on: 1073501
Attached file patch v1 (obsolete) —
It still needs tests
Attachment #8498806 - Flags: review?(etienne)
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+
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)
Blocks: 1101425
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)
Assignee: david.garciaparedes → fernando.campo
Attached file patch v2
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 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 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+
Comment on attachment 8551364 [details] [review]
patch v2

so flagging etienne for super after gmarty's comment
Attachment #8551364 - Flags: superreview?(etienne)
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)
Oh and we should attach some screenshots to the bug and ask (Eric?) for ui-review. Thanks!
Attached image DefaultLaunch-icon-unselected (obsolete) —
Assuming that the Eric that Etienne mentions is EChang, sorry if wrong person.
Attachment #8572705 - Flags: ui-review?(echang)
Attached image DefaultLaunch-noIcon-selected (obsolete) —
second screenshot, more options, selected checkbox (copied from bug 1039386 attachment 8498163 [details])
Attachment #8572707 - Flags: ui-review?(echang)
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)
Attachment #8572707 - Flags: ui-review?(echang) → ui-review?(epang)
Attached image screen.jpg
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)
Attached image Updated screenshot (obsolete) —
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)
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)
(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 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+
Attached image updated-font-300
updated to 300, please check again
Attachment #8573413 - Attachment is obsolete: true
Attachment #8573413 - Flags: ui-review?(epang)
Attachment #8574467 - Flags: ui-review?(epang)
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)
Made a last comment :)
Flags: needinfo?(etienne)
Comment on attachment 8574467 [details]
updated-font-300

Looks good, thanks for making the change Fernando!
Attachment #8574467 - Flags: ui-review?(epang) → ui-review+
Last comment addressed, r+'ed and all tests green. Not a bad start for monday :)
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
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)
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)
Blocks: 1141479
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)
(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)
(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)
Depends on: 1155701
Depends on: 1202592
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: