939174 - [User Story] Edge Gesture Tutorial Outside of FTE

Status: RESOLVED FIXED

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

Description

[Peter Dolanjski [:pdol]]

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.

Comment 1

[Peter Dolanjski [:pdol]]

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.

Comment 2

[Francis Djabri [:djabber]]

Attachment: WhatsNewTutorial_v0.2.pdf

Wireframes for "What's New" tutorial attached. This is shown after the user successfully completes a system update.

Comment 3

[Gregor Wagner [:gwagner]]

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?

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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?).

Comment 5

[Sam Joch [:samjoch]]

Attachment: Branch (WIP)

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?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8358161 [details]
Branch (WIP)

Can you provide a diff instead of a whole branch ? :)

Comment 7

[Sam Joch [:samjoch]]

Comment on attachment 8358161 [details]
Branch (WIP)

>https://github.com/samjoch/gaia/tree/bug-939174-UserStoryEdgeGestureTutorialOutsideOfFTE

Comment 8

[Sam Joch [:samjoch]]

Attachment: Diff Master Branch (wip)

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Any progress happening on this? I need to use the FTE on upgrade portion soon.

Comment 10

[Sam Joch [:samjoch]]

I am finishing the gecko & the gaia patches.
Kyle, will flag you when it will be attached.

Comment 11

[Gregor Wagner [:gwagner]]

Do we still need visuals here?

Comment 12

[Sam Joch [:samjoch]]

yep, we need all the assets of the steps.

Comment 13

[Gregor Wagner [:gwagner]]

Eric, do you have an ETA here?

Comment 14

[Sam Joch [:samjoch]]

Attachment: patch gecko

Comment 15

[Sam Joch [:samjoch]]

Attachment: gaia pr

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Just curious, why are you using asyncStorage instead of settings for ftu.enabled?

Comment 19

[Sam Joch [:samjoch]]

Attachment: patch gaia

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 21

[Eric Pang [:epang] UX]

redirecting to Peter La since he is working on FTU visuals.  Peter please let me know if you need my help with this :).  Thanks!

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8362122 [details] [review]
patch gaia

I'm so glad that you volunteered! :)

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v1) - WIP: Integration Tests for FTU bringup on Upgrade

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.

Comment 25

[Sam Joch [:samjoch]]

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?

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 28

[Gregor Wagner [:gwagner]]

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?

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 30

[Eric Pang [:epang] UX]

Attachment: FTU-Screens.zip

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!

Comment 32

[Sam Joch [:samjoch]]

Attachment: 0001-Bug-939174-User-Story-Edge-Gesture-Tutorial-Outside-.patch

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 34

[Sam Joch [:samjoch]]

Attachment: Gecko patch

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8372163 [details] [diff] [review]
Gecko patch

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

Very nice!

Comment 36

[Jason Smith [:jsmith]]

Clearing blocking flag - this is a targeted feature, not a committed feature. So this isn't a blocker.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 38

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8362122 [details] [review]
patch gaia

Stealing per Gregor request in IRC

Comment 39

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f646b79cf1e0

Comment 42

[Jason Smith [:jsmith]]

Not a blocker. Drivers decided that any features other than the listed QC features are not part of this release.

Comment 43

[Gregor Wagner [:gwagner]]

Sam, can you disable/backout the gaia pieces? We don't ship edge gestures in 1.4.

Comment 44

[Sam Joch [:samjoch]]

It not merged yet, i had issue with test.

Comment 45

[Michael Henretty [:mikehenrty][:mhenretty]]

Sam, are we ready to land this?

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8362122 [details] [review]
patch gaia

Same comment than last month. Should be pretty straightforward to fix.

Comment 47

[Gregor Wagner [:gwagner]]

(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?

Comment 48

[Gregor Wagner [:gwagner]]

Sam is back :)
Can we get this landed?

Comment 49

[Sam Joch [:samjoch]]

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?

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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...

Comment 51

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 52

[Francisco Jordano [:arcturus] [:francisco]]

Happy to review if flagged for :)

Comment 53

[Sam Joch [:samjoch]]

Attachment: Updated branch

Update the pr with the one that we discuss on irc.
r+ by :21

Comment 54

[Francisco Jordano [:arcturus] [:francisco]]

Hei Sam, could you rebase the patch?

Comment 55

[Jason Smith [:jsmith]]

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.

Comment 56

[Guillaume Marty [:gmarty]]

I'm taking over this bug. Anyone to point in the right direction of how to test it?

Comment 57

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 58

[Guillaume Marty [:gmarty]]

Attachment: Rebased PR on Github

Comment 59

[Guillaume Marty [: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.

Comment 60

[Guillaume Marty [:gmarty]]

Also, I need these assets for tablets (*-large.png at 822x515 px).

Comment 61

[Gregor Wagner [:gwagner]]

review ping

Comment 62

[Borja Salguero [:borjasalguero]]

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

Comment 63

[Guillaume Marty [:gmarty]]

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.

Comment 64

[Guillaume Marty [:gmarty]]

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!

Comment 65

[Borja Salguero [:borjasalguero]]

(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

Comment 66

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 67

[Eric Pang [:epang] UX]

Attachment: 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 :)

Comment 68

[Eric Pang [:epang] UX]

(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

Comment 69

[Sam Foster [:sfoster] (he/him)]

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.

Comment 70

[Sam Foster [:sfoster] (he/him)]

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

Comment 71

[Sam Foster [:sfoster] (he/him)]

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

Comment 72

[Sam Foster [:sfoster] (he/him)]

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.

Comment 73

[Sam Foster [:sfoster] (he/him)]

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?

Comment 74

[Borja Salguero [:borjasalguero]]

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!

Comment 75

[Sam Foster [:sfoster] (he/him)]

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.

Comment 76

[Sam Foster [:sfoster] (he/him)]

Attachment: (WIP) PR: Detect upgrade and launch tutorial

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.

Comment 77

[Borja Salguero [:borjasalguero]]

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!

Comment 78

[Sam Foster [:sfoster] (he/him)]

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?

Comment 79

[Sam Foster [:sfoster] (he/him)]

btw I have the wrong bug number if my branch and commit msg currently, I'll fix it before landing.

Comment 80

[Sam Foster [:sfoster] (he/him)]

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.

Comment 81

[Sam Foster [:sfoster] (he/him)]

Attachment: PR: Detect upgrade and launch tutorial

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?

Comment 82

[Sam Foster [:sfoster] (he/him)]

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

Comment 83

[Sam Foster [:sfoster] (he/him)]

One UI test failure showed up on travis: https://travis-ci.org/mozilla-b2g/gaia/jobs/26871439 - I think this is unrelated?

Comment 84

[Sam Foster [:sfoster] (he/him)]

removing leave-open flag. If we can land attachment 8435085 [details] [review] we're done here.

Comment 85

[Sam Foster [:sfoster] (he/him)]

Travis is shiny and green this time: https://travis-ci.org/mozilla-b2g/gaia/builds/26871435

Comment 86

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 87

[Sam Foster [:sfoster] (he/him)]

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.

Comment 88

[Sam Foster [:sfoster] (he/him)]

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 89

[Borja Salguero [:borjasalguero]]

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! :)

Comment 91

[Sam Foster [:sfoster] (he/him)]

Opened bug 1022055 to follow up with the refactor to move classList.show stuff out of AppManager

Comment 92

[Sam Foster [:sfoster] (he/him)]

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.

Comment 93

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.