1146541 - Merge autothemer back to master branch

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Theme Editor, defect, P1)

Description

[Doug Sherk (:drs) (inactive)]

Julien made some changes to the Theme Editor to support pulling colors out of pictures from the Gallery or taken using the camera, but it's sitting on his own branch.

We should get this (UI-)reviewed and merged back to the master branch.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Demo video

Jacqueline, I made a video of a quick look at the changes in this branch. It will be helpful for you when working on the spec.

Comment 2

[Julien Wajsberg [:julienw]]

From the video, it looks like the "autotheme"  does not work, but it really should :) Maybe it doesn't work from where you triggered it.

Here are 2 additional videos:
http://people.mozilla.org/~jwajsberg/fxos/v3/autotheme-1.webm
http://people.mozilla.org/~jwajsberg/fxos/v3/autotheme-2.webm

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: github PR

Just an untested rebased version

Comment 4

[Julien Wajsberg [:julienw]]

Works as intended in Firefox.

(same profile as the test-agent ("DEBUG=1 DESKTOP=0 make"), run "python -mSimpleHTTPServer" in the directory, access http://localhost:8000)

Comment 5

[Julien Wajsberg [:julienw]]

Needs the patch from bug 1111961 to be able to install the theme.

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8584686 [details] [review]
github PR

Here is a commented PR :)

Remember you need a gecko with the patch from bug 1111961 so that installing a theme works !

There is some known bugs that I didn't have the time to debug:
* sometimes, in the "create theme" dialog, I have the "clear autotheme" cross even when no autotheme palette is displayed.
* sometimes, creating a theme doesn't create it.

I think we can try to fix theme in follow-up bugs.

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 8584686 [details] [review]
github PR

Here we go, some small comments on github, and lots of :+1:! This is a great improvement to the app.

But I want to discuss two bigger points here:

* until now I tried to keep the app bare-minimum "external libs"-wise, except for gaia-components (which are heavily used in other lightsaber apps).
I'm not completely opposed to adding dependencies, but if we do it, it should make the app *more* like other lightsaber apps, not diverge further. To put it a bit bluntly studio should become more like the sharing app with time [1] not more like the sms app :/
There's room for compromise :
|defer.js| is small enough, but I'm not so sure about introducing |event_dispatcher.js| as part of an Auto Theme patch. CustomEvents on the window should perfectly do the trick for now since you're only emitting from singleton objects anyway.

* the global state AutoTheme.active is probably going to cause trouble down the line :) But I'm fine with just filing a follow up for now.


[1] https://github.com/fxos/sharing

Comment 8

[Jacqueline Savory [:jsavory] UX]

Thanks for the videos guys, it was very helpful when creating the spec!

Comment 9

[Julien Wajsberg [:julienw]]

I fixed Etienne's comments + other bugs, but I found that the activity was not working well if the app was previously running. So I want to investigate this before asking a new review.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8584686 [details] [review]
github PR

This time I think it's ready.

The activity issue was caused by bug 818000 (I hate this bug), I worked around it by removing the hash in the activity entry point.

Please test properly on a cypress branch as I don't have one handy :/

Thanks !

Comment 11

[Etienne Segonzac (:etienne)]

(tested on cypress too)

https://github.com/fxos/studio/commit/d10b7864ad0bb1f185ce1349da9be03b861badcf