1021175 - Incorrect version of wallpaper shows the first time you use dialer

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

Flame, while running:

Gaia   d2cfef555dabab415085e548ed44c48a99be5c32
SourceStamp 51b428be6213
BuildID 20140605040202
Version 32.0a1
Base image: 10G

STR:
1. Flash a Flame device. 
2. Change the default homescreen to another picture, either from Gallery or from Wallpapers
3. Make a call from Dialer.

Observe the attached screenshots. The first time you make a call using dialer the old wallpaper still shows. In the sequence the green wallpaper was initially selected and changed to a different one- the strawberry.

Comment 1

[Marcia Knous [:marcia]]

Attachment: What the dialer should like the first time I launch it

Comment 2

[Marcia Knous [:marcia]]

Attachment: First launch of dialer

Comment 3

[Jason Smith [:jsmith]]

A video of the bug here could help clarify the impact a bit better.

Comment 4

[Montoya Clemmons (MClemmons)]

(In reply to Jason Smith [:jsmith] from comment #3)
> A video of the bug here could help clarify the impact a bit better.

Flame 2.0 Video example = https://www.youtube.com/watch?v=VSt28YcLelc

Comment 5

[Jason Smith [:jsmith]]

(In reply to mclemmons from comment #4)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > A video of the bug here could help clarify the impact a bit better.
> 
> Flame 2.0 Video example = https://www.youtube.com/watch?v=VSt28YcLelc

We need a video here that focuses on the bug here & keeps out irrelevant information that's not part of the bug.

Comment 7

[Anthony Ricaud (:rik)]

Let's not make a video, we have enough information with the STRs. Could it be a regression of bug 1009596?

Comment 8

[Jason Smith [:jsmith]]

(In reply to Anthony Ricaud (:rik) from comment #7)
> Let's not make a video, we have enough information with the STRs. Could it
> be a regression of bug 1009596?

We need a video because I can't make a triage call off of the given information.

Comment 9

[Doug Sherk (:drs) (inactive)]

Here's the new video: http://youtu.be/VSt28YcLelc?t=52s

I'll look into this.

Comment 10

[Doug Sherk (:drs) (inactive)]

I backed out bug 1009596 locally and it's still broken.

Comment 11

[Jason Smith [:jsmith]]

UX - If the wallpaper is wrong when you first dial a phone call, then is this a blocker?

Note - this is a regression.

Comment 12

[Stephany Wilkes]

Defer to Victoria but my inclination is to say: not a blocker (up to Vicky though), but believe it is a regression.

Comment 13

[Anthony Ricaud (:rik)]

Even if it's not a blocker, I'd like to keep it as a feature-b2g: 2.0 since such regressions are not acceptable imho.

Comment 14

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: regression

Comment 15

[Victoria Gerchinhoren [:vicky]]

(In reply to Stephany Wilkes from comment #12)
> Defer to Victoria but my inclination is to say: not a blocker (up to Vicky
> though), but believe it is a regression.

It looks like a decision has already been made around the priority. I agree. This looks really odd and if it's a regression, we should fix it.

Thanks.

Comment 16

[Doug Sherk (:drs) (inactive)]

I'd like a regression range for this.

Comment 17

[Doug Sherk (:drs) (inactive)]

I think I used the wrong tag for comment 16.

Comment 19

[Jason Smith [:jsmith]]

Switching to qawanted to first branch check this on 1.4.

Comment 20

[Maria Angeles Oteo (:oteo)]

Not able to reproduce it in latest 1.4 version on a buri device

Build info
------------
hamachi-v1.4
B-101
Gecko-15e6ea6
Gaia-3419a1f
BuildID:20140622204229
Platform version:30.0

Comment 21

[Doug Sherk (:drs) (inactive)]

I believe this is a regression from the landing of the callscreen (thanks Etienne). I have a patch for this, but I've discovered a somewhat bad problem with Blobs which I need to hack a workaround for. I have described this further in bug 1029064.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Doug Sherk (:drs) from comment #21)
> I believe this is a regression from the landing of the callscreen (thanks
> Etienne). I have a patch for this, but I've discovered a somewhat bad
> problem with Blobs which I need to hack a workaround for. I have described
> this further in bug 1029064.

So that means this is a regression from bug 990003, right?

Comment 23

[Doug Sherk (:drs) (inactive)]

(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Doug Sherk (:drs) from comment #21)
> > I believe this is a regression from the landing of the callscreen (thanks
> > Etienne). I have a patch for this, but I've discovered a somewhat bad
> > problem with Blobs which I need to hack a workaround for. I have described
> > this further in bug 1029064.
> 
> So that means this is a regression from bug 990003, right?

I believe so, but I didn't bisect it or confirm that, I just jumped straight to fixing it. Based on my fix, I think it's safe to say so.

Comment 24

[Doug Sherk (:drs) (inactive)]

Attachment: Fix incorrect wallpaper shown if the user changes it.

PR: https://github.com/mozilla-b2g/gaia/pull/20942

I was wrong about the fix required in bug 1029064. Unfortunately I implemented a really complicated blob comparison in attachment 8445567 [details] [diff] [review], and then when I was done, I realized I could just listen for the wallpaper.image setting. Fortunately, it's really simple now.

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 8445619 [details] [diff] [review]
Fix incorrect wallpaper shown if the user changes it.

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

2 questions:
- What's you plan for the emergency wallpaper? (it won't be handle by a settings change)
- Isn't SettingsListener.observe triggering the callback a first time with |null| and thus calling _onWallpaperReady too early?

Comment 26

[Doug Sherk (:drs) (inactive)]

(In reply to Etienne Segonzac (:etienne) from comment #25)
> Comment on attachment 8445619 [details] [diff] [review]
> Fix incorrect wallpaper shown if the user changes it.
> 
> Review of attachment 8445619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 2 questions:
> - What's you plan for the emergency wallpaper? (it won't be handle by a
> settings change)

I figured we'd cross that bridge when we come to it. I don't think there's any value in leaving a stub function that does nothing.

Roughly, I think we could store the wallpaper whenever it's set, and when an emergency call ends, restore that old wallpaper.

> - Isn't SettingsListener.observe triggering the callback a first time with
> |null| and thus calling _onWallpaperReady too early?

I just checked and it's always called with a blob passed in if the pref is set. I think the default value is only used if the pref doesn't exist.

Comment 27

[Etienne Segonzac (:etienne)]

Comment on attachment 8445619 [details] [diff] [review]
Fix incorrect wallpaper shown if the user changes it.

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

Cool. Thanks for the answers.

Let's needinfo German since he's the one asking for the setEmergencyWallpaper to be kept in.
It's not a big deal, but I think it's fair to remove it if we know the assets won't come in time for this sprint or the next one.

Comment 28

[Etienne Segonzac (:etienne)]

See below :)

Comment 29

[Doug Sherk (:drs) (inactive)]

I thought about this some more, and I came up with a better strategy. Now that we have better contact photos code, we handle the emergency call wallpaper there. The contact photos code can store the emergency wallpaper blob, and when it detects that a call is an emergency call, it returns that as the contact photo for that call to CallScreen. This allows us to leverage the wallpaper/photo teardown logic of the contact photos code.

I'll still wait for Germán to answer before I push this, though.

Comment 30

[Germán Toro del Valle (:gtorodelvalle)]

Ups, I mislaid this need-info :O Sorry!

Totally fine with removing the mentioned non-really-functional functions ;-) Thanks for asking!

Comment 31

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/c5645f2d4f196b92f4d9b09b0aa580d05fd0f37a

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Eric Chang [:ericcc] [:echang]]

v2.1 on the left, v2.0 on the right(build 2014/07/07)
https://www.youtube.com/watch?v=udOaXTScZjo
Hi Vicky, Just want to make sure this is the expected behavior, thanks.

Comment 34

[Victoria Gerchinhoren [:vicky]]

(In reply to Eric Chang [:ericcc] [:echang] from comment #33)
> v2.1 on the left, v2.0 on the right(build 2014/07/07)
> https://www.youtube.com/watch?v=udOaXTScZjo
> Hi Vicky, Just want to make sure this is the expected behavior, thanks.

Sorry, what should I look at? If you have a picture assigned to a contact, that's what you should see. if there's no picture associated to a contact, the current wallpaper would be shown. I am asking because I just see examples of contacts with pictures here.

Comment 35

[Eric Chang [:ericcc] [:echang]]

Wallpaper shown at first, then it gets replaced by Contact photos, so we see both, one follows the other, just not sure if that is what we expected or intended to implement for transition like that.

Comment 36

[Eric Chang [:ericcc] [:echang]]

Hi Vicky, so the transition is okay, thanks..

Works okay with these 2 builds.
Flame Aurora v2.0
Gaia      1dd043857399c713e3b509c0ed31bdf20326f08b
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b
BuildID   20140707160206
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220

Flame Master v2.1
Gaia      e935f4ff190b76c70d9b2af8856c542a6e4a7546
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d7a3a0b7b
BuildID   20140707160206
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220