1078381 - Provide confirmation that theme has been changed

Status: RESOLVED FIXED

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

Description

[Maria Sandberg [:mushi]]

Based on comment 43 in bug 1015853 we need to give the user some feedback that a theme has been changed. 

The easiest solution here would probably be using a toast notification that confirms to the user that the theme has been changed.

Comment 1

[Maria Sandberg [:mushi]]

Adding Stephany as and FYI and to add any potential flags I don't know about :)

Comment 2

[Stephany Wilkes]

Candice, should this be tracked as part of Tako work? Not sure what additional flags we might need here, so this is a "just in case." Thanks!

Comment 3

[Candice Serran (:cserran)]

ni'ing Fabrice and Francisco who picked this up last week...

Comment 4

[[:fabrice] Fabrice Desré]

The issue with a toast notification is that we would need a new string, and we are waaaayyy past string freeze!

Comment 5

[Maria Sandberg [:mushi]]

Flagging Jaime as she asked for this bug to be filed.

Comment 6

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

Hi,

just jumped past week to give a hand, and commented some of the problems in the bug, but we were in a hurry so we landed the first version.

As Mr. Desré comments, a string is totally out of discussion, I was proposing in the following bug doing a simple thing:

When we click in a theme, we 'disable', the rest of the themes until the process of changing the theme finish.

With that we will be killing two birds with a stone:
- We will provide visual feedback.
- We will prevent the situation when a user can chante 30 times the selected them, and we will see the changes secuentially happening even after settings is closed (Yes, writing in the settings db, fetching files and changing wallpaper takes time)

Comment 7

[[:fabrice] Fabrice Desré]

Mr. Jordano, how can you implement that since we don't have any notification when the process of applying themes finishes?

Comment 8

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

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Mr. Jordano, how can you implement that since we don't have any notification
> when the process of applying themes finishes?

Dear Fabrice,

I totally agree, but we know when we save changes to settings DB. That won't give us the exact moment when the them will be applied, but we'll be closer ;)

Comment 9

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

Also, right now if a theme fails to load, (ie the wallpaper information was invalid), we don't show anything :(

Comment 10

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

Attachment: v1

Hi Arthur,

this is a first version, doing some user feedback meanwhile we save and fetch all the data.

There is no toast or something similar to be able to merge it in 2.1, we can do a follow up in master to give the user a visual feedback of what just happened.

Also included a 'broken' theme, to test that we fallback to the previous theme if something went wong.

Comment 11

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

Fabrice anyone from product, Candice perhaps, or UX that kind of 'validate' this?

Thanks!

Comment 12

[[:fabrice] Fabrice Desré]

That looks sane to me.

Comment 13

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

Comment on attachment 8509405 [details] [review]
v1

It seems we could rollback the theme in a simpler way. Please check my comments in github, thanks.

Comment 14

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

Attachment: v2

Arthur, added your comment about reusing setTheme

Comment 15

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

Comment on attachment 8513521 [details] [review]
v2

Thanks for the explanation to the two steps setting mechanism and it makes sense. However, I would suggest not to update the UI directly in a function that accessing the database. It would make the code difficult to trace. Please check my comments in github, thanks!

Comment 16

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

Comment on attachment 8513521 [details] [review]
v2

Thanks Arthur,

Hope I addressed your comments in this 3rd round.

Comment 17

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

Comment on attachment 8513521 [details] [review]
v2

Looks great!!

I listed some nits in github. In addition, it would be better to have unit tests for `setTheme` and `rollbackTheme`, thanks!

Comment 18

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

Comment on attachment 8513521 [details] [review]
v2

Thanks Arthur for the review, comments addressed and new unit test added.

Comment 19

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

Comment on attachment 8513521 [details] [review]
v2

r=me. Please remove the test themes before merging, thank you!!

Comment 20

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

Thanks Arthur for the review, the current test themes already exists, we are just adding a new one, the one that is faulty, to check the process of rollback.

Those themes are on the dev_apps folders, so they will never end up in a build image. I think we should keep it for the engineering builds in case that we need to work with this kind of problems/features.

Comment 21

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

Oh, I see. Thanks!

Comment 22

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

Landed:

https://github.com/mozilla-b2g/gaia/commit/813056a1628b9322994a687593116223b6605bb3

Comment 23

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

Fabrice do you still need this in 2.1?

Comment 24

[[:fabrice] Fabrice Desré]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #23)
> Fabrice do you still need this in 2.1?

No.