925179 - [Media] [Gallery] [User Story] Arrange content in the gallery app based on month

Status: RESOLVED FIXED

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

Description

[Sri Kasetti]

User Story:
As a user, I want the gallery app to automatically arrange content by the month 

Acceptance Criteria:
- When the gallery app is opened the user can see that all the content (images, videos) are arranged by month

Comment 1

[Diego Marcos [:dmarcos]]

We need UX spec for this.

Comment 2

[Jacqueline Savory [:jsavory] UX]

Here is a link to the latest spec for Gallery Sort by month. Let me know if anyone has any questions about this document.

https://mozilla.app.box.com/s/0u4jt353ei9ov2c150ip/1/1211595131/10819296157/1

Comment 3

[Hema Koka [:hema]]

Diego - Why is this related to bug 891030?

Comment 4

[Diego Marcos [:dmarcos]]

I was assuming that to sort the pictures by month we are going to use the time stamp contained in the EXIF metadata.

Comment 5

[Diego Marcos [:dmarcos]]

I changed the dependencies. I think is more accurate now. 891030 depends on 928612 but this bug shouldn't be depending on 891030

Comment 6

[Punam Dahiya [:pdahiya]]

Attachment: PR to group gallery content

Hi David,
Attaching the PR for your feedback on implementation approach.
Thanks

Comment 7

[David Flanagan [:djf]]

Comment on attachment 829108 [details] [review]
PR to group gallery content

Punam,

It looks like you're on the right track.  I left lots of comments on github, though. I think a lot of your date comparison code could be simplified.

The more I think about the requirement to group months, the more brutally hard it seems. I think we should push back on UX on that and just do single month groups (obviously, though, not displaying months with no photos).

Comment 8

[Hema Koka [:hema]]

Punam, please go ahead with the single month grouping i.e the scope of this bug is for arranging gallery content by single month only. 

We will open another bug for tracking the multi-month display for gallery/video apps. 

Thanks
Hema

Comment 9

[Hema Koka [:hema]]

Not a 1.3 blocker, however targeted for 1.3

Comment 10

[Punam Dahiya [:pdahiya]]

Comment on attachment 829108 [details] [review]
PR to group gallery content

Hi David

I have updated the PR to group gallery content by single months. I have used the thumbnail_date_group , thumbnail_list classes from video app (Bug 908380) with minor additions. I propose moving these classes to shared media folder. Please review.

Thanks

Comment 11

[David Flanagan [:djf]]

Comment on attachment 829108 [details] [review]
PR to group gallery content

Punam,

This patch has problems with the way it matains the offset position of each of the thumbnail groups.  I'm also troubled by the fact that some of the code for maintaining the dataset.index values is in gallery.js and some is in the new thumbanil list classes.

I think part of the problem is that for the video app, John was able to design this thumbnail list to completely maintain the list of videos.  The app does not have a separate videos[] array. So John's classes serve as both the model (the actual list of videos) and the view (thumbnail display). 

But gallery is heavily dependent on the files[] array. So unless you do a major refactor that array is going to be the model.  So for this app, the new thumbnail classes are just the view part.  It seems like maybe the fit of John's classes isn't quite right for Gallery.  Maybe if you give up any hope of creating classes that can be shared between the two apps you'll have a better chance of getting a working solution here.

For any given thumbnail, you do need a way to figure out what its index in the files[] array is. But maintaining the dataset.index value on all of those thumbnails was always kind of a hack, and maybe it is time to get rid of that code completely.

You can always use the dom to find the position. Find a the thumbnail's position within its own group, then add the sizes of previous groups. You could do that dynamically every time you needed to find a file index.

Or, given that the files[] array is sorted by date, if you used a dataset.date and dataset.filename on each thumbnail, you could then take the date for the thumbanil and do a binary search on the files[] array to find the right date (and then use the filename in case there are two files with the same date).

Or even simpler, just do a linear search of the files[] array to find a matching filename.  I bet with even 1000 photos, it would still take < 1ms to search the array.

If you do that, the time to find the index of a thumbnail goes up a bit, but all code to update dataset.index on each insertion and deletion goes away.

Comment 12

[Punam Dahiya [:pdahiya]]

Comment on attachment 829108 [details] [review]
PR to group gallery content

Hi David
Thanks for your feedback,  I have  removed the dependency on dataset.index on each insertion and deletion, and have simplified it by adding getFileIndex function which calculates index dynamically when needed.
Please review.

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 829108 [details] [review]
PR to group gallery content

Hi John
Can you please review the pull request as David is tied up with keyboard and camera refactoring work?

Thanks

Comment 14

[Punam Dahiya [:pdahiya]]

Attachment: Gallery group by month - Packaged App for UI-review

Setting the ui-review flag for both Jacqueline and Rob. 

Hi Jacqueline, Rob
Can one of you please review the attached UI changes grouping gallery content by month?

Thanks

Comment 15

[Punam Dahiya [:pdahiya]]

Attachment: Screen Shot Gallery Content Group by month

Comment 16

[Jacqueline Savory [:jsavory] UX]

Comment on attachment 8335692 [details]
Gallery group by month - Packaged App  for UI-review

Hi, we had a chance to look at this patch and everything looks fine to us from an interaction standpoint.

Flagging Eric to review the patch as well.

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 829108 [details] [review]
PR to group gallery content

Punam,

Nice patch. I left some comments at github. Please update them before landing. The main portion of that are we don't need to parseInt anymore.

Comment 18

[Eric Pang [:epang] UX]

Comment on attachment 8335692 [details]
Gallery group by month - Packaged App  for UI-review

Aligns well with the video gallery, looks good to me, thanks!

Comment 19

[Punam Dahiya [:pdahiya]]

(In reply to John Hu [:johnhu] from comment #17)
> Comment on attachment 829108 [details] [review]
> PR to group gallery content
> 
> Punam,
> 
> Nice patch. I left some comments at github. Please update them before
> landing. The main portion of that are we don't need to parseInt anymore.

Thanks John for review and your feedback. I have updated pull request with your suggestions. I don't have permissions to land PR in master. I will appreciate if you can help merge the PR. Thanks

Comment 20

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks. I will help to land it.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/14ed05fcf07b43ec8bed744c3b2e3e2977ebb40f

Comment 22

[Marcia Knous [:marcia]]

The following Moztrap test cases have been added to Moztrap to cover this functionality:

*#11086
*#11087
*#11088
*#11089