If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Provide confirmation that theme has been changed

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mushi, Assigned: arcturus)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Adding Stephany as and FYI and to add any potential flags I don't know about :)
Flags: needinfo?(swilkes)

Comment 2

3 years ago
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!
Flags: needinfo?(swilkes) → needinfo?(cserran)
ni'ing Fabrice and Francisco who picked this up last week...
Flags: needinfo?(francisco)
Flags: needinfo?(fabrice)
Flags: needinfo?(cserran)
The issue with a toast notification is that we would need a new string, and we are waaaayyy past string freeze!
Flags: needinfo?(fabrice)
(Reporter)

Comment 5

3 years ago
Flagging Jaime as she asked for this bug to be filed.
Flags: needinfo?(jachen)
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)
Flags: needinfo?(francisco)
Mr. Jordano, how can you implement that since we don't have any notification when the process of applying themes finishes?
(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 ;)
Also, right now if a theme fails to load, (ie the wallpaper information was invalid), we don't show anything :(
Created attachment 8509405 [details] [review]
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.
Attachment #8509405 - Flags: review?(arthur.chen)
Fabrice anyone from product, Candice perhaps, or UX that kind of 'validate' this?

Thanks!
Flags: needinfo?(fabrice)
That looks sane to me.
Flags: needinfo?(fabrice)
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.
Attachment #8509405 - Flags: review?(arthur.chen)
Created attachment 8513521 [details] [review]
v2

Arthur, added your comment about reusing setTheme
Attachment #8509405 - Attachment is obsolete: true
Attachment #8513521 - Flags: review?(arthur.chen)
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!
Attachment #8513521 - Flags: review?(arthur.chen)

Updated

3 years ago
Flags: needinfo?(jachen)
Comment on attachment 8513521 [details] [review]
v2

Thanks Arthur,

Hope I addressed your comments in this 3rd round.
Attachment #8513521 - Flags: review?(arthur.chen)
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!
Attachment #8513521 - Flags: review?(arthur.chen)
Comment on attachment 8513521 [details] [review]
v2

Thanks Arthur for the review, comments addressed and new unit test added.
Attachment #8513521 - Flags: review?(arthur.chen)
Comment on attachment 8513521 [details] [review]
v2

r=me. Please remove the test themes before merging, thank you!!
Attachment #8513521 - Flags: review?(arthur.chen) → review+
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.
Oh, I see. Thanks!
Landed:

https://github.com/mozilla-b2g/gaia/commit/813056a1628b9322994a687593116223b6605bb3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Fabrice do you still need this in 2.1?
Flags: needinfo?(fabrice)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #23)
> Fabrice do you still need this in 2.1?

No.
Flags: needinfo?(fabrice)

Updated

3 years ago
Assignee: nobody → francisco
You need to log in before you can comment on or make changes to this bug.