Closed Bug 971704 Opened 9 years ago Closed 8 years ago

Clean up Wallpaper File Structure in Gaia

Categories

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

Other
Gonk (Firefox OS)
defect

Tracking

(b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: padamczyk, Assigned: joshua-s)

References

Details

(Whiteboard: ux-tracking, visual design, visual-tracking, bokken)

Attachments

(1 file)

46 bytes, text/x-github-pull-request
pdahiya
: review+
Details | Review
Currently the file structure is messy and confusing, the gaia wallpaper folder looks like:

https://github.com/mozilla-b2g/gaia/tree/master/apps/wallpaper/resources

There are 2 folders: 
+ 320x480
+ 480x800

I believe these 2 folder can be removed, and everything from "320x480" folder should be moved into the root folder: "resources".

Also the "320x480" folder contains old and unused wallpapers those should also be removed:
- leaves.png
- leaves@1.5x.png
- leaves@2x.png
- morningdew.png
- morningdew@1.5x.png
- morningdew@2x.png
- sunset@2x.png
- water.png
- water@1.5x.png
- water@2x.png

Everything in the "480x800" folder can be deleted.
No longer depends on: 950147
Flags: needinfo?(dflanagan)
What code would need to be updated to deal with this change?
Joshua: are you volunteering? I'd love your help :-)

The "list.json" file in the same directory as the wallpapers would have to be updated.

And the code in apps/wallpaper/js that reads list.json would have to be updated to reflect the new path.

Punam: If Joshua wants this bug, will you help him if he has questions and review his patch when it is ready?  And if he doesn't want to take it, will you add it to your list, please?  Note that this is low-prioirty.

Patryk: It looks like we don't have a full set of @2x images, but I think that the nexus-4 uses them. If you attach the missing images to this bug, we can get them in as part of the cleanup.
Flags: needinfo?(pdahiya)
Flags: needinfo?(padamczyk)
Flags: needinfo?(dflanagan)
Helen Huang, will attach 2x images in the next few days to this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=949024
Flags: needinfo?(padamczyk)
(In reply to David Flanagan [:djf] from comment #2)
> Joshua: are you volunteering? I'd love your help :-)

Yes :)
Assignee: nobody → joshua-smith
Thanks Joshua for taking it, please feel free to set review flag or need info for me if you have any questions.
Flags: needinfo?(pdahiya)
Attached file PR on GitHub
Attachment #8376477 - Flags: review?(pdahiya)
Comment on attachment 8376477 [details] [review]
PR on GitHub

Hi Joshua

I see attached PR has below files checked in.
apps/browser/js/init.json
apps/communications/contacts/config.json
apps/communications/contacts/oauth2/js/parameters.js

My understanding is these files are not needed for this fix. Can you pl. look into it and update PR accordingly. Thanks
Attachment #8376477 - Flags: review?(pdahiya) → review-
Oh no :(  I really don't understand this.  Maybe my fork wasn't up to date with Mozilla's or something (although I thought I re-pulled).

Rebasing...
Attachment #8376477 - Flags: review- → review?(pdahiya)
Comment on attachment 8376477 [details] [review]
PR on GitHub

Try it now.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → Other
Thanks Joshua, patch looks good. As noted by Patryk in Comment #3, it will be great if we can include the new sizes in 949024 as part of this clean up. 

Updating the bug with dependency on bug 949024.
Depends on: 949024
Hi Joshua
With fix of bug 949024, we have new wallpapers. If you have bandwidth, can you please include these updates in your patch and submit for review. Thanks!
Flags: needinfo?(joshua-smith)
I can do that!
Flags: needinfo?(joshua-smith)
I updated the patch to include those changes.  Review please!
Punam, can you please review? Thanks
Flags: needinfo?(pdahiya)
Hi Patryk
Sorry for the delay. I will review the bug this week right after the blocker bug i am working on right now. Thanks
Flags: needinfo?(pdahiya)
Hi Joshua

The attached patch looks good. I noticed few build scripts are referencing the old wall paper path
./apps/wallpaper/build/build.js:WallPaperAppBuilder.prototype.WALLPAPER_PATH = 'resources/320x480';
./build/test/integration/distribution.test.js:    helper.checkFileContentByPathInZip(zipPath, 'resources/320x480/list.json',
./build/test/integration/distribution.test.js:      'resources/320x480/customize.png',

Can we update new wallpaper paths for above build files. With that fixed you have my r+ and patch is good to land on master.

Thanks
Punam
Flags: needinfo?(joshua-smith)
Hi Kevin
I am not familiar with builds, so i am including you in the loop for the change in build files (#comment 16) as part of clean up of wall paper file structure in gaia. 
Thanks!
Flags: needinfo?(kgrandon)
Oh, I didn't catch those.  Thanks!

I did update the Travis tests for the wallpaper app.  Is there anyone else who needs to be cc'd for that change?
Flags: needinfo?(joshua-smith)
(In reply to Joshua Smith [:joshua-s] from comment #18)
> Oh, I didn't catch those.  Thanks!
> 
> I did update the Travis tests for the wallpaper app.  Is there anyone else
> who needs to be cc'd for that change?

Thanks Joshua, i reviewed the unit tests and those changes look good.
The files you listed were updated!  I searched the rest of the repository to find other instances of the old url, and there were none.  Should be ready to merge :)
Flags: needinfo?(pdahiya)
Comment on attachment 8376477 [details] [review]
PR on GitHub

Thanks Joshua, r+ from me. Please let me know if you need me to merge patch on master.
Attachment #8376477 - Flags: review?(pdahiya) → review+
Flags: needinfo?(pdahiya)
I am going to forward the ni? over to Yuren to see if anything needs to be updated here.

Yuren - also if there is any process for notifying partners about this change or updating some documentation it would be good to ensure we don't miss anything. Thanks!
Flags: needinfo?(kgrandon) → needinfo?(yurenju.mozilla)
@punam: yes, I will need someone to merge it for me.  Is there a way that contributors can get push access to Mozilla repos, or is it only for employees?
Generally it's easier to just specify "checkin-needed" as a keyword, and it will automatically be checked in once the patch is reviewed. Contributors can get commit access though after a few patches: https://www.mozilla.org/hacking/commit-access-policy/

Thanks for your contribution!
(In reply to Kevin Grandon :kgrandon from comment #24)
> Generally it's easier to just specify "checkin-needed" as a keyword, and it
> will automatically be checked in once the patch is reviewed. 

Done, thanks!

> Contributors can get commit access though after a few patches:
> https://www.mozilla.org/hacking/commit-access-policy/

I have a few questions about this, but don't want to get too off-topic with the bug.  I will ping you in IRC sometime to discuss them.
Keywords: checkin-needed
it's not easy to contact each OEM so send an email to dev-gaia will help, and I also needinfo? Albert who is in charge of customization work for telefonica to know this change.

You also need to modify GAIA_DIR/customization/wallpaper/list.json because default wallpapers will be changed.

Albert, this change could affect telefonica's customization repository, could you take a look?
Flags: needinfo?(yurenju.mozilla) → needinfo?(acperez)
(In reply to Yuren [:yurenju][:小朱] from comment #26)
> it's not easy to contact each OEM so send an email to dev-gaia will help,
> and I also needinfo? Albert who is in charge of customization work for
> telefonica to know this change.
> 
> You also need to modify GAIA_DIR/customization/wallpaper/list.json because
> default wallpapers will be changed.
> 
> Albert, this change could affect telefonica's customization repository,
> could you take a look?

Sure, thank you for pointing it out.
Flags: needinfo?(acperez)
Master: https://github.com/mozilla-b2g/gaia/commit/53d9e0608345c63be4faa225ecbbf6ee4dd9e5de
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: ux-tracking, visual design, visual-tracking, bokken → ux-tracking, visual design, visual-tracking, bokken
Target Milestone: --- → 1.5 S1 (9may)
You need to log in before you can comment on or make changes to this bug.