1015853 - Changing theme should update wallpaper image

Status: VERIFIED FIXED

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

Description

[Olle Klang]

When themeing Gaia it should be possible to set a specific wallpaper for each theme. When the user changes theme the wallpaper should change accordingly.

Comment 1

[[:fabrice] Fabrice Desré]

I discussed this a bit with Vivien. What we think is the simplest solution so far is something along these lines:
- themes app would provide a ./wallpapers.json file listing wallpapers for this theme.
- when the user selects a theme, the settings app also let him chose a wallpaper if there's any included with the theme.

Comment 2

[Olle Klang]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> I discussed this a bit with Vivien. What we think is the simplest solution
> so far is something along these lines:
> - themes app would provide a ./wallpapers.json file listing wallpapers for
> this theme.
> - when the user selects a theme, the settings app also let him chose a
> wallpaper if there's any included with the theme.

So a theme could actually define several wallpapers? I like that and I think it's important from a UX point of view to treat the case when only one wallpaper is included as smooth as possible. What I suggest is that the wallpaper would then change without user interaction.

Comment 3

[Olle Klang]

(In reply to < away until June 17 > from comment #1)
> I discussed this a bit with Vivien. What we think is the simplest solution
> so far is something along these lines:
> - themes app would provide a ./wallpapers.json file listing wallpapers for
> this theme.
> - when the user selects a theme, the settings app also let him chose a
> wallpaper if there's any included with the theme.

This approach would also open up to support different wallpapers on lockscreen and homescreen as described in Bug 988374.

Comment 4

[Olle Klang]

Attachment: Update-wallpaper-on-theme-change-WIP-v1.patch

Comment 5

[Olle Klang]

Attachment: Update-wallpaper-on-theme-change-WIP-v2.patch

Uploaded two patches, both gets an error when loading a cross domain image. Can you have a look Fabrice?

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: theme-wallpaper.patch

Olle,

Here's a working patch based on your 2nd one - it still needs a bit of clean up.

Main changes:
- use {  mozSystem: true } when creating the xhr.
- loading the wallpaper from the actual selected theme, not from the overridden uri.

Comment 7

[Olle Klang]

(In reply to Fabrice Desré [:fabrice] from comment #6)

Thanks Fabrice, seems to work fine. I'll clean it up and post it on gitHub.

Comment 8

[Olle Klang]

Attachment: pull-request

Patch for review.

Comment 9

[Evelyn Hung]

Hi Olle,

I'm not intending to block your patch, but I'd like to confirm this feature is what we want to maintain for long term releases. Could you point me which UX spec you are referring or who is the UX contact? Thanks.

(also ni Mike who might know the answer.)

Comment 10

[Olle Klang]

Hi Evelyn,

I do not think there is a spec from Mozilla side as this is a feature requested from our side. However, any feedback would be appreciated.

Comment 11

[Evelyn Hung]

(In reply to Olle Klang from comment #10)
> Hi Evelyn,
> 
> I do not think there is a spec from Mozilla side as this is a feature
> requested from our side. However, any feedback would be appreciated.

Got it, thanks.
Then I'd like to hear from UX first. I'm fine to review this patch to see if it's technically working, but for long-term supporting this feature, I need UX/Product approval.

@Mike, could you have someone in UX team feedback on this feature? Is 'setting a specific wallpaper for each theme' a feature Mozilla wants to support? 

Thanks!

Comment 12

[GoodMike]

Hi Wilfred,
Can you help to check if Mozilla has committed to this feature ? And it needs to go through UX review with Rob.

Hi Rob,
Can you take a look at this feature ? I think we need to do UX spec review with our partner before anything lands into master.

Thank you.

Comment 13

[Evelyn Hung]

ni Wilfred per comment 12.

Comment 14

[[:fabrice] Fabrice Desré]

We need to move faster here. Please land, we'll figure out what to do if anyone has objections later.

Comment 15

[Kai-Chih Hu [:khu]]

I heard that this bug is required for Tako. Mark whiteboard and feature-b2g flag here. Please correct me if I am wrong.

Comment 16

[Evelyn Hung]

Comment on attachment 8486235 [details] [diff] [review]
pull-request

All look good to me except I didn't see the 'wallpaper.json' file in your PR. Is it from customization or some where?

Comment 17

[Evelyn Hung]

Comment on attachment 8486235 [details] [diff] [review]
pull-request

(In reply to Evelyn Hung [:evelyn] from comment #16)
> Comment on attachment 8486235 [details] [diff] [review]
> pull-request
> 
> All look good to me except I didn't see the 'wallpaper.json' file in your
> PR. Is it from customization or some where?

So after cross referring the patch in bug 1055104, I realized the wallpaper.json should be included in theme app.

Comment 18

[Evelyn Hung]

Comment on attachment 8486235 [details] [diff] [review]
pull-request

These theme bugs didn't show they're 2.1 blockers until the last minute. We need UX review here, and we need a clear spec that defines what's the "theme" scope. 
I'm sorry we've landed partial theme patches in bug 1055104 and other bugs. I also found the scenario doesn't work now because there is no certified theme app. We should pref off this feature on master, or let it be smarter to detect no theme app case.

Comment 19

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #18)
> I'm sorry we've landed partial theme patches in bug 1055104 and other bugs.
> I also found the scenario doesn't work now because there is no certified
> theme app. We should pref off this feature on master, or let it be smarter
> to detect no theme app case.

filed bug 1066830.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Kevin, you should have talk to me first before flagging feature-b2g. We really can't spend time on processes and breaking it every time.

and why is this bug still unconfirmed and unassigned but with a patch and blocking status?

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... and this is not a blocker because this is not a 2.1 feature.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Evelyn, this patch has no any test in place. Plus with no any theme app in the source tree it is not possible to even manually test this patch. Please re-confirm you are indeed intend to r+ the patch.

Comment 23

[[:fabrice] Fabrice Desré]

Tim, this is needed for Tako. I aggree we need tests and a couple of sample theme apps in dev_apps/.

Olle, can you provide these?

Comment 24

[Evelyn Hung]

Comment on attachment 8486235 [details] [diff] [review]
pull-request

So please add test case.

Comment 25

[Evelyn Hung]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #22)
> (...)
> Plus with no any theme app in
> the source tree it is not possible to even manually test this patch. 

Yes, that's why I filed bug 1066830. 
(In reply to Fabrice Desré [:fabrice] from comment #23)
> Tim, this is needed for Tako. I aggree we need tests and a couple of sample
> theme apps in dev_apps/.
> 
> Olle, can you provide these?

I don't think providing theme apps in dev_apps/ solves the problem. Olle, please fix bug 1066830.

Comment 26

[Kai-Chih Hu [:khu]]

Candice, are you on top of it? Thanks.

Comment 27

[Olle Klang]

(In reply to Evelyn Hung [:evelyn] from comment #25)
> I don't think providing theme apps in dev_apps/ solves the problem. Olle,
> please fix bug 1066830.

I don't think hiding the feature is a way forward, instead we should add the default theme. So I've provided the default theme in bug 1066830.

Comment 28

[Candice Serran (:cserran)]

(In reply to Kevin Hu [:khu] from comment #26)
> Candice, are you on top of it? Thanks.

Yes, we have a separate tako thread going on that Olle is cced in...thanks

Comment 29

[Olle Klang]

Attachment: 0001-Test-themes-and-settings-for-testing-the-themes-sett.patch

Test themes for testing the themes settings section.

Comment 30

[howie [:howie]]

Comment 12 , ni Rob for UX opinion and review, thanks.

Comment 31

[Rob MacDonald [:robmac]]

Flagging Maria as UX owner of this area.

Comment 32

[Maria Sandberg [:mushi]]

Hi there - is there a Github link or similar I can use to get the patch? 
Thanks!

Comment 33

[[:fabrice] Fabrice Desré]

(In reply to Maria Sandberg [:mushi] from comment #32)
> Hi there - is there a Github link or similar I can use to get the patch? 
> Thanks!

https://github.com/mozilla-b2g/gaia/pull/23836/files

Comment 34

[Maria Sandberg [:mushi]]

Attachment: theme-settings.png

I'm having a hard time reviewing this, seems like I may be missing a piece. Here are the patches I've added:

1. Update-links-to-fetch-shared-style-from-default-theme
2. bug_1015853_changing_theme_should_update_wallpaper_image
3. bug-1069791-Add-Firefox-OS-default-theme

There is one theme available under Settings/Themes, it's called "default theme" and seems to already be selected. I can't perform any action from there. Screenshot attached. Is there a patch I'm missing? Do we have a spec for what functionality is supposed to be there?

Comment 35

[Olle Klang]

(In reply to Maria Sandberg [:mushi] from comment #34)
> Created attachment 8494914 [details]
> theme-settings.png
> 
> I'm having a hard time reviewing this, seems like I may be missing a piece.
> Here are the patches I've added:
> 
> 1. Update-links-to-fetch-shared-style-from-default-theme
> 2. bug_1015853_changing_theme_should_update_wallpaper_image
> 3. bug-1069791-Add-Firefox-OS-default-theme
> 
> There is one theme available under Settings/Themes, it's called "default
> theme" and seems to already be selected. I can't perform any action from
> there. Screenshot attached. Is there a patch I'm missing? Do we have a spec
> for what functionality is supposed to be there?


These two patches should be enough to test the wallpaper update:
> 2. bug_1015853_changing_theme_should_update_wallpaper_image
and the following patch:
https://bug1015853.bugzilla.mozilla.org/attachment.cgi?id=8490017

Comment 36

[Maria Sandberg [:mushi]]

(In reply to Olle Klang from comment #35)
> and the following patch:
> https://bug1015853.bugzilla.mozilla.org/attachment.cgi?id=8490017

Olle, could you point me to where this patch lives? I can't see it in your Github repository. It should be called "Test themes and settings for testing the themes settings panel", right?

Comment 37

[Olle Klang]

Attachment: wallpaper test themes on github

Comment 38

[Olle Klang]

(In reply to Maria Sandberg [:mushi] from comment #36)
> (In reply to Olle Klang from comment #35)
> > and the following patch:
> > https://bug1015853.bugzilla.mozilla.org/attachment.cgi?id=8490017
> 
> Olle, could you point me to where this patch lives? I can't see it in your
> Github repository. It should be called "Test themes and settings for testing
> the themes settings panel", right?

Hi Maria, I've added the test themes here:
https://github.com/oklang/gaia/tree/wallpaper-test-themes.

Comment 39

[[:fabrice] Fabrice Desré]

(In reply to Olle Klang from comment #38)
> (In reply to Maria Sandberg [:mushi] from comment #36)
> > (In reply to Olle Klang from comment #35)
> > > and the following patch:
> > > https://bug1015853.bugzilla.mozilla.org/attachment.cgi?id=8490017
> > 
> > Olle, could you point me to where this patch lives? I can't see it in your
> > Github repository. It should be called "Test themes and settings for testing
> > the themes settings panel", right?
> 
> Hi Maria, I've added the test themes here:
> https://github.com/oklang/gaia/tree/wallpaper-test-themes.

Olle, that would be nice to land them in dev_apps/ so that QA could also do some testing.

Comment 40

[Maria Sandberg [:mushi]]

Tack Olle :)

I've been trying to get both patches on my device and it seems to not work (or there is a bug). If I apply https://github.com/oklang/gaia/tree/wallpaper-test-themes I do see the "default theme" and two "test themes", but switching between them does not change the wallpaper or anything else as far as I can tell. 

Here is what I've tried:
1. git checkout oklang/bug_1015853_changing_theme_should_update_wallpaper_image
2. git checkout oklang/wallpaper-test-themes
3. make reset-gaia

I've also gone through different permutations of all steps and my impression is I'm only getting whatever patch is the last one checked out. Sorry to be such a pain here but could we possibly squash the two patches into one that I can apply? Another option is for someone to help me with the workflow for adding two patches... Happy to meet/IM/email or see someone in MTV or SF about it on Monday. Let me know what the easiest path forward is here.

Comment 41

[Olle Klang]

(In reply to Maria Sandberg [:mushi] from comment #40)
> Tack Olle :)
> 
> I've been trying to get both patches on my device and it seems to not work
> (or there is a bug). If I apply
> https://github.com/oklang/gaia/tree/wallpaper-test-themes I do see the
> "default theme" and two "test themes", but switching between them does not
> change the wallpaper or anything else as far as I can tell. 
> 
> Here is what I've tried:
> 1. git checkout
> oklang/bug_1015853_changing_theme_should_update_wallpaper_image
> 2. git checkout oklang/wallpaper-test-themes
> 3. make reset-gaia
> 
> I've also gone through different permutations of all steps and my impression
> is I'm only getting whatever patch is the last one checked out. Sorry to be
> such a pain here but could we possibly squash the two patches into one that
> I can apply?

I've put a new branch with the two patches here: 
https://github.com/oklang/gaia/tree/bug-1015853-update-wallpaper-ux-review

Varsågod :)

Comment 42

[Olle Klang]

(In reply to Fabrice Desré [:fabrice] from comment #39)
> Olle, that would be nice to land them in dev_apps/ so that QA could also do
> some testing.

Added bug 1074107 for that.

Comment 43

[Maria Sandberg [:mushi]]

(In reply to Olle Klang from comment #41) 
> I've put a new branch with the two patches here: 
> https://github.com/oklang/gaia/tree/bug-1015853-update-wallpaper-ux-review
> 
> Varsågod :)

Hurra! It's working! Very exciting. Changing a theme now changes my background wallpaper. Do we have a spec or user stories for what the requirements are here? That would help me review. Some UX feedback just looking at it:

- There is no feedback that anything has changed after switching themes, we need to add something here to let the user know their action had an effect. 
- Does the theme change anything else but the wallpaper? Since we already have functionality for changing the wallpaper (with different terminology) I'd expect the theme to include more than a wallpaper change.
- Currently there is no way to tell what changing a theme will do until I've tried it. This may be for a future release but we should consider some kind of previews or extending the flow so the user can see the effect immediately. 

Please let me know about the requirements for this release.

Comment 44

[[:fabrice] Fabrice Desré]

Maria, I agree that giving feedback to the user (like a toaster notification) would be better, but we are past string freeze for 2.1. Also usually a theme change should impact the settings app itself and as such be pretty obviously noticeable for the user!

Comment 45

[Maria Sandberg [:mushi]]

(In reply to Fabrice Desré [:fabrice] from comment #44)
> Maria, I agree that giving feedback to the user (like a toaster
> notification) would be better, but we are past string freeze for 2.1. Also
> usually a theme change should impact the settings app itself and as such be
> pretty obviously noticeable for the user!

Thanks Fabrice, that's totally fair. It also is a great example of why I keep asking for requirements/user stories for this. I can't review the UX when I don't know what the feature is supposed to be able to do in 2.1 :) If this bug only tracks a theme being able to change the wallpaper I'd agree that it does (on my Flame with Olle's latest patch), and that the UX will need some improvements before adding real value to the user. 

Adding Stephany to see if she knows anything I don't about specs/requirements for this one.

Comment 46

[Stephany Wilkes]

Patryk or Candice, do you know where any UX might be for this feature? We cannot do UX reviews without it. Thanks!

Comment 47

[Patryk Adamczyk [:patryk] UX]

Hey Stephany, no one from my team has done any UX work on this. I believe this was partnered owned and driven from a UX side of things.

Comment 48

[Candice Serran (:cserran)]

catching up today and I think fabrice mentioned it was determined there wasn't a spec?

Comment 49

[Stephany Wilkes]

Removing the UX flag because I don't know what or how we can review, without any design specs or requirements to reference. At this stage, the most prudent thing to do may be to do a full review and file bugs against the existing design to be fixed in 2.2. Thoughts?

Comment 50

[howie [:howie]]

If UX is good with the behavior of the current patch, then let's land this. Product/UX could come out with a spec and user story for v2.2.

Comment 51

[Maria Sandberg [:mushi]]

Hi. Yep, UX is fine with this behavior and ready to move forward. I've filed bug 1078381 for adding a confirmation when a theme is changed.

Comment 52

[GoodMike]

Comment on attachment 8486235 [details] [diff] [review]
pull-request

Maria already followed up.

Comment 53

[howie [:howie]]

Hi Olle, can you help to land this? Thanks.

Comment 54

[Olle Klang]

Attachment: GitHub pull-request

Rebased patch

Comment 55

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

Comment on attachment 8501573 [details] [review]
GitHub pull-request

Thanks for the patch Olle! I left some comments in github, please check them. And we also need unit tests for newly added functions before merging.

Comment 56

[howie [:howie]]

Hi Olle, please help with this, thanks.

Comment 57

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

Attachment: Pointer to PR 25199

Hi Arthur,

I've been asked to try to unblock here, so I'm providing a PR based on the original PR from Olle, adding your suggestions to move the independent functions to return promises and adding unit tests as well.

Comment 58

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

Comment on attachment 8505889 [details] [review]
Pointer to PR 25199

Thank you for helping on the patch. I left some comments in github, would you mind check them?

Comment 59

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

Comment on attachment 8505889 [details] [review]
Pointer to PR 25199

Suggestions by Arthur added.
Ready for second round.

Now saving in the method onBeforeHide, this will save the unnecessary writes to the DB, but have the effect that config is not applied until we go back. Which I'm not so sure.

It was nice to just click on the theme and you could go to the homescreen and see the change.

Also if we click on a theme, and we don't apply inmediately we could get to a wrong state, since the theme is setup, we kill settings, and we didn't save the wallpaper setting.

So, I'll be happy if we go back to the previous solution.

What do you think Arthur?

Comment 60

[Olle Klang]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #59)
> Now saving in the method onBeforeHide, this will save the unnecessary writes
> to the DB, but have the effect that config is not applied until we go back.
> Which I'm not so sure.
> 
> It was nice to just click on the theme and you could go to the homescreen
> and see the change.
> 
> Also if we click on a theme, and we don't apply inmediately we could get to
> a wrong state, since the theme is setup, we kill settings, and we didn't
> save the wallpaper setting.
>
>So, I'll be happy if we go back to the previous solution.

Hi Francisco and Arthur, thanks for going forward with this, I had to put my effort elsewhere. I just like to add that I agree that the previous solution is more desirable as the wallpaper change appear quicker and we assure that the selected themes wallpaper is showing. Waiting for the wallpaper to change every time we change theme is more annoying than the issue described by Arthur (on github).

Comment 61

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

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #59)
> Comment on attachment 8505889 [details] [review]
> Pointer to PR 25199
> 
> Suggestions by Arthur added.
> Ready for second round.
> 
> Now saving in the method onBeforeHide, this will save the unnecessary writes
> to the DB, but have the effect that config is not applied until we go back.
> Which I'm not so sure.
> 
> It was nice to just click on the theme and you could go to the homescreen
> and see the change.
> 
> Also if we click on a theme, and we don't apply inmediately we could get to
> a wrong state, since the theme is setup, we kill settings, and we didn't
> save the wallpaper setting.

Yes, that would be a problem. Let's use the previous solution.

> 
> So, I'll be happy if we go back to the previous solution.
> 
> What do you think Arthur?

Comment 62

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

Comment on attachment 8505889 [details] [review]
Pointer to PR 25199

r=me with the previous solution and the comments addressed. Let's land the patch first as this seems already late for v2.1. We can then polish the UX with another followup bug. Thanks!

Comment 63

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

Thanks Arthur,

I've updated to the original solution addressing the comments that gave.

I agree on landing this in 2.1 and follow up, since everything ins promise based now, I think we could disable any theme selection until we fulfill the last promise that saves the data.

So the user won't be able to see different writes, since we will allow to change theme once the previous selected one has been applied.

Will merge once I get a gree in gaia-try.

Thanks!

Comment 64

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

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/6c9771f328c4ac14c4c07fc21dec2b4e1d228956

(Got a bookmarks integration test failled, but this patch doesn't have any dependency with it)

Comment 65

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

Comment on attachment 8505889 [details] [review]
Pointer to PR 25199

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Feature themes
[User impact] if declined:
User wont be able to enjoy the new themes
[Testing completed]:
Smoke tests on the phone, added more unit tests
[Risk to taking this patch] (and alternatives if risky):
Low, small modification to read the file from configuration and storing in settings a blob
[String changes made]:
Nop!

Comment 66

[Ryan VanderMeulen [:RyanVM]]

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

Comment 67

[Paladin]

This issue has been verified successfully on Flame 2.1.

Setp: 
Set up:Set a specific wallpaper as wallpaper.
    1) Open Settings.
    2) Tap Themes in settings.
    3) Select another themes.
    4) Tap home button.
** The wallpaper changed accordingly.

See attachment: Verify_video.3gp
Reproducing rate: 0/5
Flame2.1 build:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 68

[Paladin]

Attachment: Verify_video.3gp

Comment 69

[SandKing]

Attachment: Verified video: Verify.3GP

This bug has been verified as "Pass" on latest Nightly build of Flame v2.2
STR:
Same STR as comment67.

Actual result:
The wallpaper will be changed after user changes the theme

See video:"Verify.3GP"
Repro rate: 0/10

Device info:
Flame 2.2 build(Pass):
Build ID               20150615002503
Gaia Revision          e7a0c6d5f4df04d45fb3f726efb9e8223600cb79
Gaia Date              2015-06-15 06:12:18
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f278c675d51f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150615.040806
Firmware Date          Mon Jun 15 04:08:18 EDT 2015
Bootloader             L1TC000118D0