1024779 - [ringtones] edge gestures doesn't work

Status: VERIFIED FIXED

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

Description

[Tiffanie Shakespeare [:tif]]

STR
1. Go to Settings->Sounds
2. Tap on Manage Ringtones
3. Try edge gesture

Expected
it works

Actual
it does not work

Comment 1

[Rob MacDonald [:robmac]]

Expected behaviour is that the edge gesture swipe from left would go to the last used application. NI me if you have any questions.

- Rob

Comment 2

[Rob MacDonald [:robmac]]

NI'ing Etienne and Alive as an FYI.

Comment 3

[Rob MacDonald [:robmac]]

Etienne and Alive, Tif and I just tried it with Settings > Keyboard > Built-in Keyboard and we're getting similar results. The Built in Keyboard is likely built as a separate app but pretending to be part of the settings app. Although the edge gesture is active, it's Built in Keyboard is swiping back to settings instead of another app. Further Built in Keyboard and Settings are showing as separate apps in task switcher.

If this is a common issue, we need a way to mitigate this.

Adding Jenny and Omega.

Comment 4

[Stephany Wilkes]

Adding Hema and Gregor to cc: and noting this as a blocker.

Comment 5

[Omega Feng [:Omega] [:馮於懋]]

Add Rudy and Arthur.
Please see Rob's comment in comment 3.
Edge gesture should change apps rather than changing pages within Settings app even it might be implemented as separately apps.

Comment 6

[Jenny Lee]

Agree with Omega. We should choose one direction for edge gesture, either use edge gesture to switch pages within an app or switch between apps. It will be quite confusing if we mix them both!

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

The case of keyboard settings is slightly different from the ringtone manager as the settings page of keyboard is not launched via inline activities.

As for the case of inline activity, like ringtone manager, I believe it should behave as other apps where users are allowed to switching apps using edge gesture.

Comment 8

[Jim Porter (:squib)]

The ringtones manager isn't an inline activity; it's launched in exactly the same way as the keyboard settings. We did this since the manager can launch activities as well, and didn't want to have multiple activities running at the same time.

Comment 9

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

From above I try to clarify what's UX want:
1. Settings and the app it launched to pretend to be part of settings should not be 2 sheets in task manager.
2. App launched by Settings should be swipe-able.

#2 is a bug so I will try to investigate why ringtone doesn't work for edge. But #1 is technically impossible because system app won't know why app A launches app B and app B is part of app A. Unless we hardcode the app origin in system app but you cannot hardcode all 3rd party keyboard in system app...

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to Jim Porter (:squib) from comment #8)
> The ringtones manager isn't an inline activity; it's launched in exactly the
> same way as the keyboard settings. We did this since the manager can launch
> activities as well, and didn't want to have multiple activities running at
> the same time.

It should be fine.
I think our best bet is to use inline activities here (for both the ringtones manager and the keyboard settings).

Comment 11

[Jim Porter (:squib)]

David: What are your opinions on making the ringtones manager be an activity? As I recall, you had originally advised against this since you were worried about an activity that could launch other activities. Based on comment 10, this doesn't seem to be a problem, and might simplify things.

(That said, I'm not sure if activities that neither send nor receive data really need to be *activities*.)

Comment 12

[David Flanagan [:djf]]

Jim,

I think my concern was with an earlier design where the ringtone management function was integrated with the ringtone picker so that you could end up with one pick activity launching another pick activity. That didn't seem like it would work well.

If using an activity to launch the ringtone manager works in this case that's fine with me. In comment 10, Etienne suggests an inline activity. But that seems to me to have the wrong semantics and the wrong app transition animation.  If you can fix this with a dispostion:window activity, I'd say go for it.  The problem, of course, is that other apps could implement activity handlers that mess with this.

But perhaps a dispositon:window activity just launches the other app and gives you the same results as now. In that case, I'd say it might be better to try to convince UX that this is a WONTFIX especially if the plan is to eventually go to a sheets based model where edge gestures allow navigation within apps as well as between apps.

Comment 13

[David Flanagan [:djf]]

(In reply to Rob MacDonald [:robmac] from comment #1)
> Expected behaviour is that the edge gesture swipe from left would go to the
> last used application. NI me if you have any questions.
> 
> - Rob

And technically that is exactly what is happening, right? The settings app launches the ringtones app and then the edge gesture goes back to the settings app. I understand, though, that we'd like to maintain the fiction that the ringtones management app is just part of the settings app.  But I fear that it may be hard to do that satisfactorily and think that we may need to compromise on the UX here.

Maybe using an activity will work, but then there may be an inappropriate animation when opening the ringtones management app (Jim will have to investigate to see about this).  And with an activity, what happens when you edge gesture back to the previous app and then forward again? I suspect you'd end up at the settings app not at the ringtone manager, so again the fiction of treating the ringtone manager as if it is just a panel of the settings app is compromised.

I suspect that making this work they way you want would involve a lot of potentially destabilizing work in the system app.  Alive would know for sure. It seems to me that if there is work required in the system app it is feature work and not bug fixes, so realistically we're probably talking about 2.2 before it could be done.

Rob: when are we going to switch to allowing edge gestures to navigate within an app?  If that is coming soon, I think that we should just consider the ringtones app to be ahead of its time and live with this minor inconsistency for now.

Jim: would you try using an activity here and report what happens with both inline and window disposition activities? Maybe this is actually easy to fix and I'm getting ahead of myself worrying about it.

Comment 14

[Jim Porter (:squib)]

Actually, a disposition:inline activity seems to work *better* than what we have now. I still can't edge-swipe back to the settings app*, but it does seem to fix bug 1017416 for free. disposition:window seems to have all the same issues that we have now, which I expect are caused in part by the app having a "system" role.

* I'm probably not the best person to test this, since I have, at best, a vague idea of when edge gestures are supposed to work.

Comment 15

[Jim Porter (:squib)]

Actually, it looks like for disposition:inline, we can edge-swipe to whatever app is "behind" the settings app. So that's good, I guess.

Comment 16

[Jim Porter (:squib)]

Attachment: Make the ringtone manager an inline activity

Comment 17

[Rob MacDonald [:robmac]]

(In reply to David Flanagan [:djf] from comment #13)
> 
> Rob: when are we going to switch to allowing edge gestures to navigate
> within an app?  If that is coming soon, I think that we should just consider
> the ringtones app to be ahead of its time and live with this minor
> inconsistency for now.

We anticipate this taking a minimum of two releases or more, which means we could be living with app to app edge gestures for an extended period. That's why I'm hoping Etienne's proposal will pan out. Although the transition will be off, we'll at least (hopefully) maintain the illusion of the ringtone manager as a view within the settings app.
 
Also, I'll review this patch tonight as I'm quite anxious to see it work in practice and compare it against the edge gesture spec.

Thanks!
Rob

Comment 18

[Rob MacDonald [:robmac]]

I'm not able to get this patch to work but will follow up with Jim and Tif tomorrow.

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8441770 [details] [review]
Make the ringtone manager an inline activity

Looking good, I think this is a better user experience.
A few clarifications:

- nested inline activities work, the window manager already supports them

- with this patch edge swiping to another app will work, and when swiping back to the settings app you'll see the inline activity(ies) still open

- we might want to change the back/close icon of the ringtone chooser/track selector since the transition is now reflecting that the activities are stacked on top of one another, maybe it should point down instead of left

PS: not sure but it looks like when you select a track another window.open is happening, can we change this one too?

Comment 20

[Jim Porter (:squib)]

(In reply to Etienne Segonzac (:etienne) from comment #19)
> PS: not sure but it looks like when you select a track another window.open
> is happening, can we change this one too?

Do you mean when you open the manager, click the +, and pick a song? If so, what I'm trying to do there is to open a "fake" ringtone share activity that takes the results of the just-completed pick activity. I'm not sure of a better way to do this. Ideas?

Comment 21

[David Flanagan [:djf]]

Comment on attachment 8441770 [details] [review]
Make the ringtone manager an inline activity

The code looks fine to me.  My main questions are about naming of the activity, and there is one unused string to remove from the locale file (or an error dialog to restore).  See github.

In the past did you have code in the app somewhere to call window.close when the app lost visibility?  Maybe I'm misremembering. But if it is still there somewhere you presumably need to rethink it as part of this patch.

Comment 22

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #21)
> In the past did you have code in the app somewhere to call window.close when
> the app lost visibility?  Maybe I'm misremembering. But if it is still there
> somewhere you presumably need to rethink it as part of this patch.

I did, but I deleted it before I landed the original patch.

Comment 23

[David Flanagan [:djf]]

Ignore that last bit. I see the window.close() code being removed right there in the patch.

Comment 24

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #23)
> Ignore that last bit. I see the window.close() code being removed right
> there in the patch.

Yeah, that's when you hit "back", but there was a brief period where I tried to close the window *whenever* it lost visibility, which at the time, required a gross hack for the "create a new ringtone" flow.

Comment 25

[David Flanagan [:djf]]

Part of what I didn't get about this bug is that I didn't realize how much window management had changed. In 1.4, pressing the home button during an inline activity cancelled the activity. That doesn't happen anymore on master. 

Rob: I'm guessing the reason you couldn't get Jim's patch to work is that you need to do a make reset-gaia since it modifies the app manifest. Be sure to test it in 2.0 and on master. I'm not sure when the necessary system app changes landed.

Comment 26

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #25)
> Part of what I didn't get about this bug is that I didn't realize how much
> window management had changed. In 1.4, pressing the home button during an
> inline activity cancelled the activity. That doesn't happen anymore on
> master. 

Argh. This just means the ringtone manager's list can get out of sync with the DB, since you could add a ringtone from the music app while the manager is open in the background. We can't listen for indexedDB changes (why not?!), so the manager won't know about the new ringtone until it's force-quit.

Comment 27

[Tiffanie Shakespeare [:tif]]

Working with Rob over IRC to help get the patch on his phone and also took a video for him in the meantime.

Comment 28

[Tiffanie Shakespeare [:tif]]

Took a look at the patch with Rob. The Manage page looks good - the pick activity seems to have solved some issues we were seeing.

We checked out the Music activity (I know this isn't our area but...) and that all looks good and works as expected.

We did still see some issues with the actual ringtone create screen. The edge gestures are still disabled and doing a long press on the home button dismisses the create screen. Fixing the edge gestures on this screen is a must. 

Maintaining state is not as high a priority for this release, but when crop/edit comes into play it will be an issue that needs to be solved. IDK if it would be easier to fix it now or later...

Comment 29

[Tiffanie Shakespeare [:tif]]

oops forgot to clear Rob's flag - see above comment.

Comment 30

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/ccd70903544486bea04e85d8a4aacf63f1de2a72

Comment 31

[Jim Porter (:squib)]

Comment on attachment 8441770 [details] [review]
Make the ringtone manager an inline activity

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 960329
[User impact] if declined: Edge gestures won't work from the ringtone manager
[Testing completed]: Manually tested and basic functionality backed by automated tests
[Risk to taking this patch] (and alternatives if risky): Low-risk
[String changes made]: None

Comment 32

[Tiffanie Shakespeare [:tif]]

Hey Jim - I was checking out Master just now and encountered many issues with the ringtone feature. I don't know if it was this patch or something else - can you double check?

Here's what I've been seeing...
- making your way to Mange Ringtones screen. press/hold home button. The preview thumbnail is just black
- doing an edge gesture from Manage Ringtones (to the left) just shows a white screen
- going through the new activity the first time, after you select the track, the flow just returns to the Mange Ringtones page
- subsequent attempts to create a ringtone go to the create page, but there is a double header. press/hold on the home button shows a thumbnail without the second header, but appears to be cut off on the right. tapping to return show the 2nd header again
- same holds true for edge gesture, swiping away and back removes the 2nd header but then it reappears

These above I've consistently seen. I've seen other issues not consistently...
- have an action more menu open for a custom ringtone, press/hold home button, the action menu switches to show only share
- have an action menu open and press/hold home button, the thumbnail shows manage ringtone. selecting the settings app returns and reopens the action menu

There were a couple other things that happened that I can't recall at the moment. Let me know how you want to handle it. Thanks Jim!

Comment 33

[Tiffanie Shakespeare [:tif]]

Ah! I recall another...

When having the phone in landscape it seems like sometimes the create screen refuses to rotate and when I can get it to rotate is show a split between the create screen and the Sound setting page

Comment 34

[Jim Porter (:squib)]

(In reply to Tiffanie Shakespeare from comment #32)
> Hey Jim - I was checking out Master just now and encountered many issues
> with the ringtone feature. I don't know if it was this patch or something
> else - can you double check?
> 
> Here's what I've been seeing...
> - making your way to Mange Ringtones screen. press/hold home button. The
> preview thumbnail is just black

Can you file a bug against the system app for this? This happens with all activities.

> - doing an edge gesture from Manage Ringtones (to the left) just shows a
> white screen

This works fine for me. It shows whatever app was behind the settings app (or the system wallpaper if there's no other apps open).

> - going through the new activity the first time, after you select the track,
> the flow just returns to the Mange Ringtones page

This also works fine for me.

> - subsequent attempts to create a ringtone go to the create page, but there
> is a double header. press/hold on the home button shows a thumbnail without
> the second header, but appears to be cut off on the right. tapping to return
> show the 2nd header again

I haven't seen this at all.

> - same holds true for edge gesture, swiping away and back removes the 2nd
> header but then it reappears

Ditto.

> - have an action more menu open for a custom ringtone, press/hold home
> button, the action menu switches to show only share

I can't reproduce this either.

> - have an action menu open and press/hold home button, the thumbnail shows
> manage ringtone. selecting the settings app returns and reopens the action
> menu

This is probably the same thing as the first issue you mentioned.

(In reply to Tiffanie Shakespeare from comment #33)
> When having the phone in landscape it seems like sometimes the create screen
> refuses to rotate and when I can get it to rotate is show a split between
> the create screen and the Sound setting page

I don't see this, but I *do* see that the Create Ringtone screen rotates, and it shouldn't. I'm explicitly locking it to portrait mode.

Comment 35

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #34)
> (In reply to Tiffanie Shakespeare from comment #32)
> > - going through the new activity the first time, after you select the track,
> > the flow just returns to the Mange Ringtones page
> 
> This also works fine for me.

I updated my tree and now I see this and most of the other create-ringtone issues that I couldn't see before. I've filed bug 1027991 to track that.

Comment 36

[bhavana bajaj [:bajaj]]

:squib, can you confirm this is safe to land on 2.0 given the issues pointed out by Tiffanie are being tracked in 1027991(which blocks 1021658 ) and patches here seem safe  for uplift?

Comment 37

[Jim Porter (:squib)]

It should be safe, but it might make sense to wait until the other bugs are landed to uplift this.

Comment 38

[Jim Porter (:squib)]

This should be safe to uplift now.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/a9cdb84c8f6d774410b3e9cc3b56ed310c2701b4

Comment 40

[Elie]

Attachment: VIDEO0076_Compress.MP4

This issue has been successfully verified on Flame 2.0:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141127000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2


This issue has been successfully verified on Flame 2.1:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2