Closed Bug 1008020 Opened 6 years ago Closed 5 years ago

Create a smart collections application

Categories

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

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.0, tracking-b2g:backlog, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Whiteboard: [p=13],[systemsfe])

Attachments

(8 files, 1 obsolete file)

46 bytes, text/x-github-pull-request
amirn
: review+
ranbena
: review+
Details | Review
44 bytes, text/x-github-pull-request
ranbena
: review+
kgrandon
: feedback+
Details | Review
44 bytes, text/x-github-pull-request
ranbena
: review+
Details | Review
44 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
44 bytes, text/x-github-pull-request
ranbena
: review+
kgrandon
: feedback+
Details | Review
44 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
46 bytes, text/x-github-pull-request
amirn
: review+
Details | Review
46 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
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.
feature-b2g: --- → 2.0
Comment on attachment 8420416 [details] [review]
WIP - github pull request

Hey guys - what do you think of something like this?
Attachment #8420416 - Flags: review?(ran)
Attachment #8420416 - Flags: review?(amirn)
Depends on: 1008044
Comment on attachment 8420416 [details] [review]
WIP - github pull request

LGTM. Waiting for Amir's feedback as he starts working on top of it.
Attachment #8420416 - Flags: review?(ran) → review+
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.
Attachment #8420416 - Flags: review?(amirn) → review+
I too think "collection" should be referred to as "smart-collection" in the code.
(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.
let's use EverythingMe/gaia:origin
created a PR here - https://github.com/EverythingMe/gaia/pull/17

Thanks!
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
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.
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?
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.
Attachment #8424757 - Flags: review?(ran)
Attachment #8424757 - Flags: feedback?(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.
Attachment #8424757 - Flags: feedback?(kgrandon) → feedback+
Attachment #8424757 - Flags: review?(ran) → review+
merged to EverythingMe/master:

Bug 1008020 - [Collections App] Create Smart Collections [r=ranbena]
97304800d00f739a699d7c2190636c3660ffb282
Attachment #8425372 - Flags: review?(ran)
Attached file Homebutton ignore PR
Attachment #8425413 - Flags: review?(kgrandon)
Attachment #8425372 - Flags: review?(ran) → review+
Comment on attachment 8425413 [details] [review]
Homebutton ignore PR

Looks good to me, but please update the event name first. Thanks!
Attachment #8425413 - Flags: review?(kgrandon) → review+
Attachment #8424757 - Attachment description: PR for create-collections functionality → PR #1 create-collections activity
Attachment #8425372 - Attachment description: PR: deviceId from settings → PR #2 deviceId from settings
Attachment #8425534 - Flags: review?(ran)
Attachment #8425534 - Flags: feedback?(kgrandon)
Landed on master
"Disabling home button homescreen reaction when collection web activities are active"
https://github.com/EverythingMe/gaia/commit/0f27e782f74d570a1cc164a634ae43fddead322b
Comment on attachment 8425534 [details] [review]
PR #3 view-collections activity

Just a few comments on GH
Attachment #8425534 - Flags: review?(ran) → review+
Comment on attachment 8425534 [details] [review]
PR #3 view-collections activity

Starting to look good. Thanks!
Attachment #8425534 - Flags: feedback?(kgrandon) → feedback+
landed on EverythingMe/master
"Bug 1008020 - [Collections App] View Smart Collections"
https://github.com/EverythingMe/gaia/commit/0c579b8378e6e0c1a9d733854a5478ab197c053b
Attachment #8426098 - Flags: review?(kgrandon)
Attachment #8426098 - Flags: review?(kgrandon) → review+
Attached file PR #5 Collections icon (obsolete) —
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
Attachment #8428371 - Flags: review?(ran)
Attachment #8428371 - Flags: review?(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!
Attachment #8428371 - Flags: review?(kgrandon) → review+
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
Attached file 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!
Attachment #8428722 - Flags: review?(amirn)
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.
Attachment #8428371 - Attachment is obsolete: true
Attachment #8428371 - Flags: review?(ran)
Attachment #8428730 - Flags: review?(ran)
Attachment #8428730 - Flags: review?(kgrandon)
Comment on attachment 8428722 [details] [review]
PR #6 - JSHint fixes

r+ 10x :)
Attachment #8428722 - Flags: review?(amirn) → review+
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.
Attachment #8428730 - Flags: review?(kgrandon) → review+
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
Attachment #8428730 - Flags: review?(ran)
(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.
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
blocking-b2g: --- → backlog
Nice work here. Let's continue futher work in other bugs. Thanks all!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 990032
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.