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.
Adding Stephany as and FYI and to add any potential flags I don't know about :)
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!
ni'ing Fabrice and Francisco who picked this up last week...
The issue with a toast notification is that we would need a new string, and we are waaaayyy past string freeze!
Flagging Jaime as she asked for this bug to be filed.
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)
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.
Fabrice anyone from product, Candice perhaps, or UX that kind of 'validate' this? Thanks!
That looks sane to me.
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.
Created attachment 8513521 [details] [review] v2 Arthur, added your comment about reusing setTheme
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 on attachment 8513521 [details] [review] v2 Thanks Arthur, Hope I addressed your comments in this 3rd round.
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 on attachment 8513521 [details] [review] v2 Thanks Arthur for the review, comments addressed and new unit test added.
Comment on attachment 8513521 [details] [review] v2 r=me. Please remove the test themes before merging, thank you!!
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!
Fabrice do you still need this in 2.1?
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #23) > Fabrice do you still need this in 2.1? No.