Closed Bug 939174 Opened 6 years ago Closed 6 years ago

[User Story] Edge Gesture Tutorial Outside of FTE

Categories

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

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: pdol, Assigned: sfoster)

References

Details

(Whiteboard: [ucid:System110, ucid:browser104, ft:systems-fe, 2.0][systemsfe][p=13])

Attachments

(4 files, 11 obsolete files)

1.09 MB, application/pdf
Details
2.15 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
214.97 KB, application/x-zip-compressed
Details
46 bytes, text/x-github-pull-request
alive
: review+
borjasalguero
: review+
Details | Review
User Story:
As a user who has updated my OS, I want a tutorial to inform me how to use edge gestures to switch between open apps so that the ability is more discoverable.

Acceptance Criteria:
1. After updating to the new OS version which includes app-to-app gesture switching, I am visually informed how to switch between open app using edge gestures.
Note that we need the ability to disable this if a decision is made not to ship the app-to-app switching feature in 1.3.
Wireframes for "What's New" tutorial attached. This is shown after the user successfully completes a system update.
Flags: in-moztrap?(jhammink)
Whiteboard: [ucid:System110, 1.3:P2, ft:systems-fe] → [ucid:System110, 1.3:P2, ft:systems-fe][systemsfe]
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Assignee: nobody → sjochimek
Vivien, do you have any preferences here? How should we implement this. Should we start FTU after an upgrade or should have create another Upgrade app?
Flags: needinfo?(21)
(In reply to Gregor Wagner [:gwagner] from comment #3)
> Vivien, do you have any preferences here? How should we implement this.
> Should we start FTU after an upgrade or should have create another Upgrade
> app?

I feel like having one app is less an overhead. Also it will let us arrange the screens that needs to be displayed in case the user goes from version 1.1 to 1.4 directly for example. (is that even possible?).
Flags: needinfo?(21)
No longer blocks: 1.3-systems-fe
Whiteboard: [ucid:System110, 1.3:P2, ft:systems-fe][systemsfe] → [ucid:System110, 1.4:P2, ft:systems-fe][systemsfe]
Flags: in-moztrap?(jhammink)
Attached file Branch (WIP) (obsolete) —
I came up with this solution to handle FTU Update.

So the idea, is to have an empty previous_os initially and to keep
the FTU logic and then just select the right TutorialSteps to show
in the FTU app.

At first, the 'initial' steps will be printed.
On update, os_previous..os key will be printed (without language, wifi, etc).

To test it, you should set previous_os to '1.3.0.0-prerelease' in common-settings.js.

For now now i set previous_os settings when FTU is done, but its needed to be set right after the update.

What do you think?
Attachment #8358161 - Flags: feedback?(21)
Comment on attachment 8358161 [details]
Branch (WIP)

Can you provide a diff instead of a whole branch ? :)
Attachment #8358161 - Flags: feedback?(21)
Flags: needinfo?(sjochimek)
Attached file Diff Master Branch (wip) (obsolete) —
Attachment #8358420 - Flags: feedback?(21)
Flags: needinfo?(sjochimek)
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 C2/1.4 S2(17jan)
Any progress happening on this? I need to use the FTE on upgrade portion soon.
I am finishing the gecko & the gaia patches.
Kyle, will flag you when it will be attached.
Do we still need visuals here?
yep, we need all the assets of the steps.
Eric, do you have an ETA here?
Flags: needinfo?(epang)
Attached patch patch gecko (obsolete) — Splinter Review
Attachment #8358420 - Attachment is obsolete: true
Attachment #8358420 - Flags: feedback?(21)
Attached file gaia pr (obsolete) —
Attachment #8361726 - Flags: feedback?(kyle)
Attachment #8361726 - Flags: feedback?(21)
Attachment #8361726 - Attachment description: github pr → gaia pr
Attachment #8361692 - Flags: feedback?(anygregor)
Whiteboard: [ucid:System110, 1.4:P2, ft:systems-fe][systemsfe] → [ucid:System110, ucid:browser104, 1.4:P2, ft:systems-fe][systemsfe]
Duplicate of this bug: 947497
Duplicate of this bug: 947493
Just curious, why are you using asyncStorage instead of settings for ftu.enabled?
Attached file patch gaia (obsolete) —
Attachment #8361726 - Attachment is obsolete: true
Attachment #8361726 - Flags: feedback?(kyle)
Attachment #8361726 - Flags: feedback?(21)
Attachment #8362122 - Flags: review?(21)
Hi folks,

I know that the title says to do this changes outside the FTU, but actually the changes are inside the FTU, so should a peer from FTU take a look to them?

Cheers,
F.
redirecting to Peter La since he is working on FTU visuals.  Peter please let me know if you need my help with this :).  Thanks!
Flags: needinfo?(epang) → needinfo?(pla)
Comment on attachment 8362122 [details] [review]
patch gaia

I'm so glad that you volunteered! :)
Attachment #8362122 - Flags: review?(21) → review?(francisco.jordano)
Attachment #8362122 - Flags: review?(francisco.jordano) → review?(borja.bugzilla)
So, a couple of things. First off, I noticed there was what I think is an error in the settings name (deviceinfo.previous versus deviceinfo.previous_os), made a comment about it in the code review.

I also noticed that the patch is still using asyncStorage for ftu.enabled. Unfortuantely, that makes it quite difficult to test this, because I have no way to set ftu.enabled from outside the app, and since this is in the system app, it comes up before I can figure out how to wedge something in.

This goes back to the question I asked before: Why is asyncStorage being used? I mean, I can see some reasons (cleans up settings, doesn't really NEED to be outside the scope of app, etc), but if this is going to become a common case then we need to look at adding a capability to create these values to the integration test system also, the same way we have for prefs and settings right now.
Adding patch for WIP commit of tests for bringing up FTU during update. Doesn't currently work since we're still figuring out what needs to be done around asyncStorage.
Kyle, thanks for the review, i hve updated the branch.

For asyncStorage, the patch doesn't use it, it was already used in the code.
Don't understand exactly what that change?
Oooooh, ok, sorry, that was my misunderstanding then. I'm used to setting FTU.manifestURL to null in custom-settings when I need builds to come up without the FTU. I was under the assumption that we were emulating the ftu launcher when doing that, and that the launcher set the manifestURL to null and used that as the state check, and that this patch changed that. Also, before last week we never had integration tests for the FTU in the first place, so this is new anyways. Since this is going to require more changes than I thought, I'll split the integration tests for FTU upgrade starting out to another bug.
Comment on attachment 8363252 [details]
Patch 4 (v1) - WIP: Integration Tests for FTU bringup on Upgrade

Marking integration test obsolete and moving it to another bug.
Attachment #8363252 - Attachment is obsolete: true
Comment on attachment 8361692 [details] [diff] [review]
patch gecko

Review of attachment 8361692 [details] [diff] [review]:
-----------------------------------------------------------------

We are using get and set in an async way here and call shutdown. I am not sure if this is safe. bent?
Attachment #8361692 - Flags: feedback?(anygregor) → feedback?(bent.mozilla)
Comment on attachment 8361692 [details] [diff] [review]
patch gecko

Review of attachment 8361692 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/UpdatePrompt.js
@@ +370,5 @@
> +    Services.settings.createLock().get("deviceinfo.os", {
> +      handle: function(name, value) {
> +        log("Set previous_os to: " + value);
> +        let lock = Services.settings.createLock();
> +        lock.set("deviceinfo.previous_os", value, null, null);

You'll need to wait for set to call its success callback before calling shutdown, otherwise you may or may not write this value into the settings database.
Attachment #8361692 - Flags: feedback?(bent.mozilla)
blocking-b2g: --- → 1.4+
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.3 C3/1.4 S3(31jan)
Attached file FTU-Screens.zip (obsolete) —
Hey Sam, sorry for the delay of the screens.  Peter has been swamped with work.  I've attached the screens.  Let me know if anything else is needed :).  Thanks!
Flags: needinfo?(pla) → needinfo?(sjochimek)
Duplicate of this bug: 947491
Attachment #8362122 - Flags: review?(borja.bugzilla)
Flags: needinfo?(sjochimek)
Target Milestone: 1.3 C3/1.4 S3(31jan) → 1.4 S1 (14feb)
Depends on: 968215
Flags: in-moztrap?(jhammink)
Attachment #8361692 - Attachment is obsolete: true
Attachment #8371512 - Flags: review?(bent.mozilla)
Comment on attachment 8371512 [details] [diff] [review]
0001-Bug-939174-User-Story-Edge-Gesture-Tutorial-Outside-.patch

Review of attachment 8371512 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! One small thing and one big thing need addressing below:

::: b2g/components/UpdatePrompt.js
@@ +366,4 @@
>    restartProcess: function UP_restartProcess() {
>      log("Update downloaded, restarting to apply it");
>  
> +    let callbackAfterSet = function UP_callbackAfterSet() {

Nit: I don't think naming the anonymous function is really useful anymore, so I would just do:

  let callbackAfterSet = function() {

@@ +381,5 @@
> +
> +    // Save current os version in deviceinfo.previous_os
> +    let lock = Services.settings.createLock({
> +      handle: callbackAfterSet
> +    });

You're not handling the abort case here, so if for some reason your settings transaction fails you won't actually restart the phone. It's unlikely but you should probably handle that case somehow.
Attachment #8371512 - Flags: review?(bent.mozilla)
Attached patch Gecko patchSplinter Review
Attachment #8371512 - Attachment is obsolete: true
Attachment #8372163 - Flags: review?(bent.mozilla)
Comment on attachment 8372163 [details] [diff] [review]
Gecko patch

Review of attachment 8372163 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice!
Attachment #8372163 - Flags: review?(bent.mozilla) → review+
Whiteboard: [ucid:System110, ucid:browser104, 1.4:P2, ft:systems-fe][systemsfe] → [ucid:System110, ucid:browser104, 1.4:P2, ft:systems-fe][systemsfe][p=13]
Attachment #8362122 - Flags: review?(francisco.jordano)
Clearing blocking flag - this is a targeted feature, not a committed feature. So this isn't a blocker.
blocking-b2g: 1.4+ → ---
Attachment #8362122 - Flags: review?(francisco.jordano) → review?(borja.bugzilla)
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/f646b79cf1e0

Does this need a [leave open] on the whiteboard for the Gaia patch still? Please add it if the answer is yes.
Keywords: checkin-needed
Whiteboard: [ucid:System110, ucid:browser104, 1.4:P2, ft:systems-fe][systemsfe][p=13] → [ucid:System110, ucid:browser104, 1.4:P2, ft:systems-fe][systemsfe][p=13][leave open]
Comment on attachment 8362122 [details] [review]
patch gaia

Stealing per Gregor request in IRC
Attachment #8362122 - Flags: review?(borja.bugzilla) → review?(francisco.jordano)
Comment on attachment 8362122 [details] [review]
patch gaia

Left some comments in github.

Impressive job Sam, left some ideas on how to do it more flexible, and other things I saw.

Also asking for a system peer for the system part, despite that I know that code, better that someone from system review it.
Attachment #8362122 - Flags: review?(francisco.jordano) → review?(21)
Comment on attachment 8362122 [details] [review]
patch gaia

I made some comments on the PR as well as on https://github.com/samjoch/gaia/commit/1af60d0581e18c70aef25856174a5a0aeba98dbd (my git skills fails).

I would like to see a new version before r+'ing.
Attachment #8362122 - Flags: review?(21)
blocking-b2g: --- → 1.4+
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Not a blocker. Drivers decided that any features other than the listed QC features are not part of this release.
blocking-b2g: 1.4+ → backlog
No longer blocks: 1.4-systems-fe
Flags: in-moztrap?(jhammink)
Attachment #8362122 - Flags: review?(21)
Target Milestone: 1.4 S2 (28feb) → 1.4 S3 (14mar)
Sam, can you disable/backout the gaia pieces? We don't ship edge gestures in 1.4.
Flags: needinfo?(sjochimek)
It not merged yet, i had issue with test.
Flags: needinfo?(sjochimek)
Target Milestone: 1.4 S3 (14mar) → 1.4 S4 (28mar)
Sam, are we ready to land this?
Flags: needinfo?(sjochimek)
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
blocking-b2g: backlog → 1.5+
Depends on: 991849
No longer depends on: edge-gestures
Attachment #8362122 - Flags: review?(21)
Flags: needinfo?(sjochimek)
Comment on attachment 8362122 [details] [review]
patch gaia

Same comment than last month. Should be pretty straightforward to fix.
Attachment #8362122 - Flags: review?(21)
Target Milestone: 1.4 S5 (11apr) → 1.4 S6 (25apr)
(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #46)
> Comment on attachment 8362122 [details] [review]
> patch gaia
> 
> Same comment than last month. Should be pretty straightforward to fix.

Any update here? Did you fix the issues?
Flags: needinfo?(sjochimek)
Flags: needinfo?(sjochimek)
Sam is back :)
Can we get this landed?
Flags: needinfo?(sjochimek)
Vivien, there is nothing to fix here.
THe only problem is that the changes you ask (cf. github) keep break the tests and i cant find the reason. all the variables used dont seems to be used else where in the code. (that must be the reason you ask those changes). but i cant find a solution for your needs here. Btw, this part of the code have nothing to do with the patch. 

i suggest that to land this patch and to file a new bug for that.

What do you think?
Flags: needinfo?(sjochimek) → needinfo?(21)
(In reply to Sam Joch [:samjoch] from comment #49)
> Vivien, there is nothing to fix here.
> THe only problem is that the changes you ask (cf. github) keep break the
> tests and i cant find the reason. all the variables used dont seems to be
> used else where in the code. (that must be the reason you ask those
> changes). but i cant find a solution for your needs here. Btw, this part of
> the code have nothing to do with the patch. 
> 
> i suggest that to land this patch and to file a new bug for that.
> 
> What do you think?

Can you post a log of the tests beeing broken ? I would really like to address this code to make it easier to maintain in the future...
Flags: needinfo?(21)
Comment on attachment 8362122 [details] [review]
patch gaia

We discussed that on IRC and it seems like Travis is green now even with the changes I asked. I made a small nit on the new PR.

Please note that I didn't review at all the communications/ftu part since it has been started by Francisco. You may need his r+ to land.
Attachment #8362122 - Flags: review+
Flags: needinfo?(francisco.jordano)
Happy to review if flagged for :)
Flags: needinfo?(francisco.jordano)
Attached file Updated branch (obsolete) —
Update the pr with the one that we discuss on irc.
r+ by :21
Attachment #8362122 - Attachment is obsolete: true
Attachment #8413726 - Flags: review?(francisco.jordano)
Hei Sam, could you rebase the patch?
Flags: needinfo?(sjochimek)
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.
blocking-b2g: 2.0+ → backlog
Whiteboard: [ucid:System110, ucid:browser104, 1.4:P2, ft:systems-fe][systemsfe][p=13][leave open] → [ucid:System110, ucid:browser104, ft:systems-fe, 2.0][systemsfe][p=13][leave open]
Assignee: sjochimek → gmarty
I'm taking over this bug. Anyone to point in the right direction of how to test it?
Guillaume, can you apply Sam's gaia patch, and do what's mentioned in comment #5? Seems like that should pop up this tutorial flow.
Flags: needinfo?(sjochimek) → needinfo?(gmarty)
Attached file Rebased PR on Github (obsolete) —
Attachment #8413726 - Attachment is obsolete: true
Attachment #8413726 - Flags: review?(francisco.jordano)
Attachment #8418071 - Flags: review?(borja.bugzilla)
Flags: needinfo?(gmarty)
I rebased samjoch's code and had to make many changes to accommodate the recent changes (See bug 976535).
I introduced Tutorial.config to set the tutorial configuration and to check whether to show the update tutorial after an OS update.
Also, I need these assets for tablets (*-large.png at 822x515 px).
Flags: needinfo?(epang)
feature-b2g: --- → 2.0
review ping
Flags: needinfo?(borja.bugzilla)
Comment on attachment 8418071 [details] [review]
Rebased PR on Github

Hi Guillaume! After taking a look to the patch, it seems that this 'Tutorial from an update' has enough value to have a separate Logic & Markup, instead of modifying (again) the Tutorial section. 

Following the WF given [1] is a separate section (despite of the UI is the same as the Tutorial one, but this is good because you can use the same CSS styles!) when upgrading from one version to another.
This could be so useful not only for the case of v1.3 > v1.4, so I would create a separate section called 'upgrade-tutorial' (with it on JS), which taking into account the version previously installed and the new one, will show the right steps to the user (we could even have a different set per scenario of upgrade, and keep this consistency in the future, so will be so easy to add a new version here!). As a suggestion: probably we could have a 'generic' tutorial, and add the number of steps, images and l10n keys by a configuration file. But you could do this in a followup. Thanks!


[1] https://bug939174.bugzilla.mozilla.org/attachment.cgi?id=833230
Attachment #8418071 - Flags: review?(borja.bugzilla) → review-
Flags: needinfo?(borja.bugzilla)
Hi again! The code in this PR is generic enough to do what you describe. You can already use it to do all kind of upgrades as it is executed whenever the os version changes.

You just have to add an entry to the `config` object of tutorial.js then add the relevant assets to the images folder & l10n. The default tutorial that you call 'generic' is called 'default' there.

But I agree, this object should be placed in a separate configuration file to ease maintenance.
Hi Borja, I know you're away this week, but can you please see my reply above and if it's OK for you then finish the review. I'd love to complete this bug quickly. Many thanks!
Flags: needinfo?(borja.bugzilla)
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
(In reply to gmarty from comment #64)
> Hi Borja, I know you're away this week, but can you please see my reply
> above and if it's OK for you then finish the review. I'd love to complete
> this bug quickly. Many thanks!

Hi Guillaume! As you know we have refactored the Tutorial, trying to keep it as simple as it is (a linear flow showing images & text). So in this case I would go for the second idea that I suggested in [1] (probably is the easiest one to implement, because your code is aligned with it!): *Make the 'tutorial' even more generic*, using a configuration file. This would let us to load a generic 'tutorial' as many times as required, with a different set of images & text if needed, and you could get rid of the new "update-screen" panel, because at the end is a 'tutorial' (defining tutorial as a set of images & text in a linear flow, based on a basic navigation), with a different set of steps.
The configuration file should defined the number of steps in tiny & large layouts, and an array with the steps, defining l10nkey and image per each step (it's even enough to extract the number of steps from the array). That's all! 

I think this is a simple approach, easy to implement and reusable in the future, so I would go to this way. Let me know if you need help on this! 

Please ask me to review when ready, and some peer of System for the modifications there. Thanks!


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=939174#c62
Flags: needinfo?(borja.bugzilla)
Hei folks,

just did an intermediate solution:

https://github.com/arcturus/gaia/tree/gmarty-bug-939174

Getting the current code and separating the config to an external file. Also adding to that config not just then number of steps but the images location, so we remove conditionals cases.

I guess that we can get to an intermediate aggreement, land something like that, and do some follow ups.

Cheers,
F.
Attached file Updated-FTU-Screens
Hi Guillaume,  I've re attached the images with the tablet size.  I've also made an update to the app swiping screens so all sizes need to be replaced.  Thanks!  Let me know if anything else is needed :)
Attachment #8364317 - Attachment is obsolete: true
Flags: needinfo?(epang) → needinfo?(gmarty)
(In reply to Eric Pang [:epang] from comment #67)
> Created attachment 8426175 [details]
> Updated-FTU-Screens
> 
> Hi Guillaume,  I've re attached the images with the tablet size.  I've also
> made an update to the app swiping screens so all sizes need to be replaced. 
> Thanks!  Let me know if anything else is needed :)

forgot to mention they will also be here on box:
https://mozilla.box.com/s/oenjjc1bfbqewyki76jn
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Blocks: 990036
Blocks: 990035
Heads up that for the vertical homescreen tutorial (bug 990035) we are using video files. I've mocked up a naive/straw-man patch  to show how that might look currently: https://github.com/sfoster/gaia/compare/bug-990035-video-tutorial. I've also asked jsavory to clarify what content goes with what steps in which contexts, but my understanding is that the tutorial needs to support a sequence of either pngs or videos or both. 
Guillaume, I know you are out some of this week, is there anything I can do to move this forward meantime? as it would make more sense to base my patches on this work rather than whats on master currently. I'll pull the branch and have a go at that tommorow either way.
The proposed config file and imageFolder seems to introduce a lot of redundant images, and doesn't address the (new as of 2.0 and bug 990035) need for videos instead of images in some scenarios. We now have variable steps by device/layout type, and variable steps by tutorial context (first-run, update from x to y). Each might need different assets (image/video) and strings. I'd like to propose a per-upgrade config file (e.g. tutorial-1.3.0.0-prerelease..1.4.0.0-prerelease.json) and a config data structure like: 

{
  tiny: {
    steps: [
      { 
        asset: '/path/to/image',
        text: 'l10nid'
      },
      { 
        asset: '/path/to/movie',
        text: 'l10nid'
      },
      { 
        asset: '/path/to/image',
        text: 'l10nid'
      }
    ]  
  },
  'large': {
        asset: '/path/to/image',
        text: 'l10nid'
  }
}

The max-steps now comes from config[layout].steps.length. Each config can reuse images/video and strings as necessary, so we don't get that duplication in each folder. We can infer the presentation of the resource (image/video) from the extension, or add a assetType property to be explicit. 
If this sounds ok I think it makes sense to first land a configurable tutorial, then land assets and configs for the different upgrade scenarios. We'll need to rebase/start over as in the meantime Bug 1010782 happened and apps/communications/ftu moved to apps/ftu
Flags: needinfo?(francisco)
Correction, Tutorial can know layout immediately (when ScreenLayout module loads), but knowing if we are upgrading or not has to wait for navigator.mozL10n.ready -> getSetting('previous_os' -> getSetting('os'). I know ScreenLayout can notify of size/layout change, but as we only distinguish between phone-size and not (table-size) I think we can go ahead a split the config out by layout: 

config/tiny.json
----------------

{ 
  default: {
  	steps: [
      { 
        asset: '/path/to/image',
        text: 'l10nid'
      },
      { 
        asset: '/path/to/movie',
        text: 'l10nid'
      },
      { 
        asset: '/path/to/image',
        text: 'l10nid'
      }
    ]
  }, 
  "1.3.0.0-prerelease..1.4.0.0-prerelease": {
    steps: [
      { 
        asset: '/path/to/image',
        text: 'l10nid'
      }
      ....
    ]
  }
}

and similar for large.json. We can of course keep it all in one config file, but as we have different number of steps and different images it makes sense I think to split it this way. I'll work on a patch to implement this and we can discuss
Depends on: 1017314
I opened bug 1017314 and made it block this and the other 2.0 tutorial updates. ni'd franciso from that bug to approve this config structure and approach.
Flags: needinfo?(francisco)
No longer blocks: 990035
Here's where I'm at with this: https://github.com/sfoster/gaia/tree/bug-939174-upgrade-tutorial
This works for me, if you reset the profile and set deviceinfo.previous_os to 1.3 or 1.4 you should get the tutorial on startup, with just 2 steps. 

It builds on the patch from 990035 to add the upgrade logic and config data for the specific steps to show. Is there anything else you want to see in here before I put this in for review?
Flags: needinfo?(borja.bugzilla)
Hi Sam! I think that now (as we have everything configurable by a config file), this patch should be quite easy to test & review. Please create the PR and ask me to review this. Thanks!
Flags: needinfo?(borja.bugzilla)
We have everything we need at this point from UX. Placeholder assets for this bug are landing in bug 990035, final assets will land in bug 1019289. Note that we are not updating the tablet/large-layout experience for 2.0, it will get the existing images.
Assignee: gmarty → sfoster
Flags: needinfo?(gmarty)
I've created this PR to get a head start on your feedback/review. But I've become confused (possibly just the late hour) by how the device.previous_os setting is supposed to get set in the first place. And how to test this. Still, the rest of the patch should stand. There's a couple of console.logs in there which I'll clean up as I resolve these issues.
Attachment #8418071 - Attachment is obsolete: true
Attachment #8434017 - Flags: feedback?(borja.bugzilla)
Comment on attachment 8434017 [details] [review]
(WIP) PR: Detect upgrade and launch tutorial

My main concern is to add more stuff to the code of the 'app.js'. I would recommend to move all these code related with the version handling to a separate JS, and use promises for retrieving first the version, and then show the right tutorial taking into account the result.

For the System modifications, please ask to review :alive or any other peer of System.

Thanks!
Attachment #8434017 - Flags: feedback?(borja.bugzilla)
I've reworked the patch and updated the PR. It now includes a shared VersionHelper library with just a couple of methods to avoid duplicating the details and logic in both the FTU AppManager and FTULauncher. I'm running into a problem with how we've been detecting an upgrade, and how the FTU can know what to do. 

How this currently works is: we bootstrap and FtuLauncher checks ftu.enabled in asyncStorage. If that's false, it will also check if we are upgrading via VersionHelper and launch the FTU if necessary. The FTU does its own version check as it loads. If the version info indicates an update, it jumps past the FTU screen and just shows the tutorial. It will attempt to look for a tutorial config matching the version delta - e.g. 1.3..2.0 

When the FTU closes, we update deviceinfo.previous_os to the deviceinfo.os so the next isUpgrade() call will be false. The problem is that the previous_os setting is new as of this patch, so we have to infer an upgrade if that setting is empty or not found. That means that when you apply this patch or create a new profile we are *always* in an upgrade state. And as we dont currently have a way to pass parameters into the FTU, even the ftu.enabled flag is lost. 

I think ideally, the upgrade/"what's new" FTU state would be a different entry point. But I'm not sure we have time for that. I guess I could use a setting to pass info between FtuLauncher and FTU? Or, maybe there is a better way to know if we just upgraded?
Flags: needinfo?(borja.bugzilla)
Flags: needinfo?(alive)
btw I have the wrong bug number if my branch and commit msg currently, I'll fix it before landing.
I just updated the PR with https://github.com/sfoster/gaia/compare/bug-939175-upgrade-tutorial#diff-f24b244642f249eb139b7090b11337feR176, which updates the deviceinfo.previous_os setting before launching the FTU. That helps.
I got the upgrade/ftu.enabled problem worked up and this PR updates that logic and tests. 

There are new unit tests for FTULauncher (ftu_launcher_test.js) to verify and make explicit the expected behaviour. Also for the VersionHelper

To test this on-device, this is what works for me: 
# flash the patch and wipe the profile
make reset-gaia 
# the FTU should come up as normal, step through it. You can skip the tutorial
# Verify then update the deviceinfo.previous_os setting on the device: 
gcli getsetting deviceinfo.previous_os 
gcli setsetting deviceinfo.previous_os 1.3.0.blah
# now reboot
# the 'what's new' FTU screen should come up and present you with 2 steps demoing vertical scroll and sheets/swipe from edge

It would be nice to split this patch out into FTU and System bugs, but in the interest of time can I ask you to review as-is?
Attachment #8434017 - Attachment is obsolete: true
Attachment #8435085 - Flags: review?(borja.bugzilla)
Attachment #8435085 - Flags: review?(alive)
Flags: needinfo?(borja.bugzilla)
Flags: needinfo?(alive)
More about the gcli thing: http://blargon7.com/2013/10/command-line-interface-tool-for-gaia/
You'll need to 

$ adb forward tcp:2828 tcp:2828
One UI test failure showed up on travis: https://travis-ci.org/mozilla-b2g/gaia/jobs/26871439 - I think this is unrelated?
removing leave-open flag. If we can land attachment 8435085 [details] [review] we're done here.
Whiteboard: [ucid:System110, ucid:browser104, ft:systems-fe, 2.0][systemsfe][p=13][leave open] → [ucid:System110, ucid:browser104, ft:systems-fe, 2.0][systemsfe][p=13]
Travis is shiny and green this time: https://travis-ci.org/mozilla-b2g/gaia/builds/26871435
Comment on attachment 8435085 [details] [review]
PR: Detect upgrade and launch tutorial

Some questions in github.
I don't have strong opinion here since we are already storing version info in settings db and I think it won't be too harmful if previous os version is stored but if next time we could have some discussion before a nontrivial patch I will be happy.

Thanks for adding ftu launcher tests.
Attachment #8435085 - Flags: review?(alive) → review+
I've updated the PR. I swapped around the ftu.enabled and version checks, so that it launches the whats new tutorial even if ftu.enabled is false (i.e. a OTA update). Updated the unit tests to agree with this logic. I also change VersionHelper.isUpgrade's logic to treat a falsy deviceinfo.previous_os as /not/ an upgrade, as the UpdatePrompt sets the deviceinfo.previous_os setting, there should be no scenario where we need to launch the whats new tutorial without knowing what the previous version was.
Borja, I talked with Alive and got the launch/don't launch FTU logic worked out. Travis will hopefully bless the latest PR: https://travis-ci.org/mozilla-b2g/gaia/builds/26921283, so I just need your comments and ok. I'll try and catch you first thing my (PST) morning.
Comment on attachment 8435085 [details] [review]
PR: Detect upgrade and launch tutorial

Hi Sam!,
Some comments in Github to address before merging, but FTU part looks ok to me (some minor improvements, nothing blocking).

Wait to Travis and take into account the suggestions I did in GH, and we are done with this! Thanks! :)
Attachment #8435085 - Flags: review?(borja.bugzilla) → review+
Duplicate of this bug: 990036
Opened bug 1022055 to follow up with the refactor to move classList.show stuff out of AppManager
Landed: https://github.com/mozilla-b2g/gaia/commit/2a8207dff39314cca6535dd974b7ad9d70c1b0b3
https://github.com/sfoster/gaia/commit/c199c775ebf4e842d3f92d43ded4bb719cc0f112

travis came up green: https://travis-ci.org/sfoster/gaia/builds/26996553
.. but https://travis-ci.org/mozilla-b2g/gaia/jobs/26996440 shows a test_a11y_unlock_to_homescreen failure - afaict this is unrelated.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
Depends on: 1045345
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.