Closed Bug 925179 Opened 6 years ago Closed 6 years ago
[Media] [Gallery] [User Story] Arrange content in the gallery app based on month
47 bytes, text/x-github-pull-request
|Details | Review|
488.36 KB, application/zip
146.12 KB, image/png
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
We need UX spec for this.
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
Diego - Why is this related to bug 891030?
I was assuming that to sort the pictures by month we are going to use the time stamp contained in the EXIF metadata.
I changed the dependencies. I think is more accurate now. 891030 depends on 928612 but this bug shouldn't be depending on 891030
Hi David, Attaching the PR for your feedback on implementation approach. Thanks
Attachment #829108 - Flags: feedback?(dflanagan)
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).
Attachment #829108 - Flags: feedback?(dflanagan) → feedback+
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
Not a 1.3 blocker, however targeted for 1.3
blocking-b2g: 1.3? → ---
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
Attachment #829108 - Flags: review?(dflanagan)
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.
Attachment #829108 - Flags: review?(dflanagan) → review-
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.
Attachment #829108 - Flags: review- → review?(dflanagan)
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
Attachment #829108 - Flags: review?(dflanagan) → review?(johu)
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 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 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.
Attachment #829108 - Flags: review?(johu) → review+
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!
Attachment #8335692 - Flags: ui-review?(epang) → ui-review+
(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
Thanks. I will help to land it.
merged to master: https://github.com/mozilla-b2g/gaia/commit/14ed05fcf07b43ec8bed744c3b2e3e2977ebb40f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
The following Moztrap test cases have been added to Moztrap to cover this functionality: *#11086 *#11087 *#11088 *#11089
Flags: in-moztrap?(mozillamarcia.knous) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.