1008020 - Create a smart collections application

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

We would like to create an standalone smart collections app. The app will follow in the steps of the bookmarks application, and be used to sync collections across both homescreens.

Assigning to myself to handle the initial prototyping work for now - though anyone is welcome to snag parts of this/contribute patches. Until we have basic functionality in place we may want to develop this feature on a branch.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: WIP - github pull request

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8420416 [details] [review]
WIP - github pull request

Hey guys - what do you think of something like this?

Comment 3

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8420416 [details] [review]
WIP - github pull request

LGTM. Waiting for Amir's feedback as he starts working on top of it.

Comment 4

[Amir Nissim (:amirn)]

Comment on attachment 8420416 [details] [review]
WIP - github pull request

Looks great.

I added a collection suggestions list using the E.me API:
https://github.com/EverythingMe/gaia/commit/0dc959ddd30ce342aad0167a8283e198058b1157

I am having trouble getting the deviceId from the settings shared with the Search app. Can you give it a quick look?

General note - I'm sure if 'collection' is the right terminology to use since Marketplace has a similar concept. I think we were once asked to stick with 'Smart Collections' but I don't know if it is important enough.

Thanks Kevin.

Comment 5

[Ran Ben Aharon [:ranbena] EverythingMe]

I too think "collection" should be referred to as "smart-collection" in the code.

Comment 6

[Amir Nissim (:amirn)]

view-collection:
https://github.com/EverythingMe/gaia/commit/2e1e279e2bcce663426eb3142926cf3713bbfae8

Comment 7

[Kevin Grandon :kgrandon]

(In reply to Amir Nissim (Everything.me) from comment #4)
> Comment on attachment 8420416 [details] [review]
> WIP - github pull request
> 
> General note - I'm sure if 'collection' is the right terminology to use
> since Marketplace has a similar concept. I think we were once asked to stick
> with 'Smart Collections' but I don't know if it is important enough.

We currently have no concept of collections in gaia general, and I would prefer to stick with a shorter name, just to make things easier. If there is a good reason to use the full name, we absolutely can - though I didn't really see a need to. Something like "WebCollection" might make more sense, modeled after the "WebResults" in the search app.

(In reply to Amir Nissim (Everything.me) from comment #4)
> Comment on attachment 8420416 [details] [review]
> I am having trouble getting the deviceId from the settings shared with the
> Search app. Can you give it a quick look?

I took a look and couldn't find that code in your commit - the usage should be the same as we do in the search app though. Here is where we implemented it for the search app: https://github.com/mozilla-b2g/gaia/commit/ccc91448373caee5ce4ae5497534860feb493955#diff-82f82e127a9859fade83143cd5592fe5R343

Nice work. We should have a centralized branch to open pull requests and stuff to. For now we can either use my branch, or we can work off of the EverythingMe/gaia branch if that is easier. Let me know what's beset for you guys.

Comment 8

[Amir Nissim (:amirn)]

let's use EverythingMe/gaia:origin
created a PR here - https://github.com/EverythingMe/gaia/pull/17

Thanks!

Comment 9

[Amir Nissim (:amirn)]

reuse Home2's Grid system in collection app:
https://github.com/EverythingMe/gaia/commit/b2ea946c32269dd89937931db8bfb2f88671cd52

currently using symlinks (app.js is the only file duplicated)
we should later move these files to /shared

Comment 10

[Kevin Grandon :kgrandon]

Nice. If it's working I think going with symlinks is probably fine until we move the apps out of dev_apps and into the apps/ folder.

Comment 11

[Amir Nissim (:amirn)]

I have some concerns about the Collections app rendering the collection UI.

I believe it is reasonable to assume that when opening a Collection, the grid
layout should match the layout of the homescreen that opened it (a collection
opened from home2 should have home2's layout).
That's why the WIP implementation re-uses home2 grid layout and objects by loading the
same files. I am afraid that:
- it means we reload the same files (bookmark.js, layout.js +4 other files)
  when opening a collection while actually they have already been loaded by the
  home2 app. Maybe we can work around this one, but more importantly
- going forward, I think this will make it difficult to support new homescreens.
  If the Collections app renders the Collection UI, then all homescreens are
  bound to display Collections the same way. If tomorrow we need to ship home3
  with a new Collection UI, it will not be possible without re-writing the
  collections app.

What do you think?

Comment 12

[Kevin Grandon :kgrandon]

I will open an email thread with :pdol and some ux guys today. My thoughts were the opposite - that smart collection UI should not be bound by the homescreen UX. I thought that you should be able to open smart collections from anywhere, and the smart collection UX could remain the same, leading to easier implementation by replaceable homescreens. This also makes it trivial to open from the search app for example.

I'm also not too worried about loading the same JS files. It's something we need to do in haida, and we're working on ways to optimize for this.

Anyway, let's see what UX/product says. Overall I think we're going to have a lot less code complexity if we can have a single smart collection experience.

Comment 13

[Amir Nissim (:amirn)]

Attachment: PR #1 create-collections activity

Comment 14

[Kevin Grandon :kgrandon]

Comment on attachment 8424757 [details] [review]
PR #1 create-collections activity

Nice work. I made a few nits that will generally apply to other files (new lines and variable declaration). These don't need to be addressed now though.

Comment 15

[Amir Nissim (:amirn)]

merged to EverythingMe/master:

Bug 1008020 - [Collections App] Create Smart Collections [r=ranbena]
97304800d00f739a699d7c2190636c3660ffb282

Comment 16

[Amir Nissim (:amirn)]

Attachment: PR #2 deviceId from settings

Comment 17

[Ran Ben Aharon [:ranbena] EverythingMe]

Attachment: Homebutton ignore PR

Comment 18

[Kevin Grandon :kgrandon]

Comment on attachment 8425413 [details] [review]
Homebutton ignore PR

Looks good to me, but please update the event name first. Thanks!

Comment 19

[Amir Nissim (:amirn)]

Attachment: PR #3 view-collections activity

Comment 20

[Ran Ben Aharon [:ranbena] EverythingMe]

Landed on master
"Disabling home button homescreen reaction when collection web activities are active"
https://github.com/EverythingMe/gaia/commit/0f27e782f74d570a1cc164a634ae43fddead322b

Comment 21

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8425534 [details] [review]
PR #3 view-collections activity

Just a few comments on GH

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8425534 [details] [review]
PR #3 view-collections activity

Starting to look good. Thanks!

Comment 23

[Amir Nissim (:amirn)]

landed on EverythingMe/master
"Bug 1008020 - [Collections App] View Smart Collections"
https://github.com/EverythingMe/gaia/commit/0c579b8378e6e0c1a9d733854a5478ab197c053b

Comment 24

[Amir Nissim (:amirn)]

Attachment: PR #4 delete collection

Comment 25

[Amir Nissim (:amirn)]

landed in EverythingMe/master
"Remove Collections"
https://github.com/EverythingMe/gaia/commit/47a76e4aeb74db88eb09be3c8f01c9a23973da01

Comment 26

[Amir Nissim (:amirn)]

Attachment: PR #5 Collections icon

There are icon quality issues. Not sure why.
Background was not yet implemented. Waiting server side API to support that feature.

Commit: https://github.com/EverythingMe/gaia/commit/83b5653b97d8908dd78da7f13b5f0d4a00f8ac06

Comment 27

[Kevin Grandon :kgrandon]

Comment on attachment 8428371 [details] [review]
PR #5 Collections icon

Responded to the icon sizing question on github. I haven't tested this on a device yet, but I'm going to spend some time now getting a pull request with the reviewed patches landed into master. With Ran's R+ we should be good to go. I would also like us to start thinking about tests for this stuff, maybe a reference test where the code has to generate an icon that matches a png on disk? Anyway, it's something we can do once it all works. Thanks for your awesome work!

Comment 28

[Kevin Grandon :kgrandon]

Patches up to #4 in this list have been merged to upstream master (6 patches in total, all reviewed). Some work was necessary to ensure that they are atomic (fixing of jshint, unit tests, etc), so you probably want to do a hard reset of EverythingMe/gaia from mozila-b2g/gaia. 

The only attached patch that has not yet been merged is PR #5 Collections icon. Once that lands I would recommend using separate bugs for work - it may be easier to track.

https://github.com/mozilla-b2g/gaia/commit/93470dbfe4212c020857bccdcb79cb16e2fafda3

Comment 29

[Kevin Grandon :kgrandon]

Attachment: PR #6 - JSHint fixes

Hey Amir - here's a few fixes to make JSHint happy, as well as add it to the paths that we lint on travis and pre-commit. Please take a quick look when you have time. Thanks!

Comment 30

[Amir Nissim (:amirn)]

Attachment: PR #5 Collections icon (mozilla-b2g/master)

Kevin, Ran - I addressed your comments in this new commit:
https://github.com/EverythingMe/gaia/commit/a4dc3247aef302bf170a31499733966dbf625d56

If you r+ I will squash and land in mozilla-b2g/master.

Thanks.

Comment 31

[Amir Nissim (:amirn)]

Comment on attachment 8428722 [details] [review]
PR #6 - JSHint fixes

r+ 10x :)

Comment 32

[Kevin Grandon :kgrandon]

Comment on attachment 8428730 [details] [review]
PR #5 Collections icon (mozilla-b2g/master)

Looks fine to me, but needs a rebase against master. Also is this supposed to show the icon on the homescreen? I haven't tracked down why yet, but I'm not seeing it appear on my device.

Comment 33

[Amir Nissim (:amirn)]

Comment on attachment 8428730 [details] [review]
PR #5 Collections icon (mozilla-b2g/master)

created new separate bug for Collection icons: https://bugzilla.mozilla.org/show_bug.cgi?id=1016204

Comment 34

[Amir Nissim (:amirn)]

(In reply to Kevin Grandon :kgrandon from comment #32)
> Comment on attachment 8428730 [details] [review]
> PR #5 Collections icon (mozilla-b2g/master)
> 
> Looks fine to me, but needs a rebase against master. Also is this supposed
> to show the icon on the homescreen? I haven't tracked down why yet, but I'm
> not seeing it appear on my device.

rebased and created new PR in bug 1016204.

It should work. I am running gecko 564e4e1 (5/22) with the disabled bookmarks patch, and it works.

Comment 35

[Kevin Grandon :kgrandon]

Nice work here. Let's continue futher work in other bugs. Thanks all!

Comment 36

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.