908380 - [Video] - Group videos in gallery view by date

Status: RESOLVED FIXED

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

Description

[Punam Dahiya [:pdahiya]]

Attachment: Gallery_View_Spec_130821.jpg

In gallery view of Video app, the user should see the videos sorted and grouped by date. See attached for visual design specs

Comment 1

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

This is a nice bug. We may use to refactor gallery view of video app. We should separate the thumbnail view/list out as an object and try to use template based to implement it.

Comment 2

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

I will do the following changes and refactoring:
1. move thumbnail view and list as an object which makes unit tests easy to make
2. use template to create thumbnail view. the template means the HTML parts of UI and using the similar binding mechanism similar to populateMediaInfo, but may not exactly the same.
3. load template with lazyloader and write template in HTML comment node which is similar to settings app. I will also try the link/import polyfill which is introduced by Kevin Grandon recently to check if performance changed.
4. try to make the thumbnail list and thumbnail view run in MVC.

Comment 3

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

Attachment: separate the thumbnail list out and write most unit tests.

David, 

I made the refactoring of thumbnail list with this bug. I moves the following things out from video.js and db.js:
1. binarysearch to MediaUtils
2. thumbnail list handling, grouping, sorting and view handling to ThumbnailList, ThumbnailDateGroup, and ThumbnailItem.
3. replace the global variable videos with ThumbnailList.

The newly introduced things are:
1. ThumbnailList: the controller of the list object, it uses videodata from mediadb as its data model.
2. ThumbnailDateGroup: the implementation of group class which is used by ThumbnailList. It is the controller of grouping and sorting mechanism. It groups videodata by date and sorted them descendant.
3. ThumbnailItem: the controller of UI operations of a videodata It handles watched, poster, and selection state. It uses videodata as its data model.
4. lots unit tests, 35 tests.

You may find this patch is big. But half of them are unit tests, about 800 lines, ;-).

Comment 4

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

I had tested with the mdpi device, hdpi device, and nightly build. It works.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 798723 [details]
separate the thumbnail list out and write most unit tests.

John,

I'm glad you're taking this opportunity to do this refactoring.

I don't think I found any errors in your code (though I did not actually run it).  

I did leave many comments and suggestions on github. Some are nits or style issues. Some are larger questions issues like:

1) use prototypal inheritance for the classes?
2) have the group element actually contain the thumbnail elements?
3) do you really need to expose the render() method as part of your class API?

I'm not giving r- because the code overall seems strong. But not giving r+ yet, because I suspect you'll want to take at least some of my suggestions. In the end, you're the one who is going to have to maintain the code, so I'm pretty flexible about how you'd like to structure it.  Just reset r? if you want me to review again.

Comment 6

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

Thanks, David for these questions.

I will change the patch. As to the questions, the following is my replies:
> 1) use prototypal inheritance for the classes?

   I have to admit that I don't know the performance difference between prototype and object creation for JS class. I use it because we may have a change to forget to type "this". In some cases, it works. But in some, it may get undeclared error. And another advantage is that programmers don't need to take care about the current "thisArg" of a function call. I had use prototype to create class before. But my other ex-team members who are not familiar the codebase may drop into the "thisArg" pitfalls. Some variables need to type this but others not, and some variables look very similar. 

   If the performance difference is huge and I will try to test and search it, I will change to use prototype. Basically, I like it before. It makes JavaScript like a strong typed language.

> 2) have the group element actually contain the thumbnail elements?

   Well, this also has story. Dominic and I had discussed about how to implement a large list view with high fps. He wrapped the thumbnail elements to the group element in Music app, before. When we want to implement a list view supporting the recycle of each list item, it is hard to do so. Because we need to construct the group back and recycle each list item. And if we want to use the same mechanism to render the middle view of a large list, we have to render group before rendering visible children even if the group header is not visible. But if we put group header and thumbnail element in the same layer, it becomes easier to do so but harder to posit the thumbnail according to group.

   I will rethink about it and try to prevent over-design. Maybe, I do think too much now.

> 3) do you really need to expose the render() method as part of your class
> API?

   I had read your suggestion about this in Github. It makes sense. I will remove this api and use it internally.

Comment 7

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

Ok. I found a performance code: http://stackoverflow.com/a/4041582. Note the code in this page does not totally equal to our case. I had changed the code of Z class to use my way to create object. The testing result is "prototype is the best and object based is about 3 times of prototype". I will change the implementation to use prototype.

Comment 8

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

Comment on attachment 798723 [details]
separate the thumbnail list out and write most unit tests.

David, 

I accept most of your suggestions. This patch is changed to:
1. use prototype to write class
2. put ThumbnailItems into group container
3. removed some useless API and expose properties
4. change the gallery.js to use the binarySearch in MediaUtils

Please review it again.

Thanks.
John

Comment 9

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

Attachment: bug908380-gallery-view.zip

Hi Sri and Patryk, 

Please review this patch.

Comment 10

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

Attachment: screenshots

Hi Sri and Patryk, 

If you have trouble with zipped version, this attachment is the screenshot of gallery view of video app.

Comment 11

[Sri Kasetti]

Either Rob or Jacqueline should review this as they had done the UX specs.
John, Product owner doesn't need to review as long as UX person gets to review the changes

Comment 12

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

Comment on attachment 800569 [details]
bug908380-gallery-view.zip

change ui-review to rob

Comment 13

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

David,

I found the test coverage was landed about 1 hour ago. I had added more test case to have almost 100% coverage:

      http://video.gaiamobile.org:8080/js/thumbnail_date_group.js - 50/51 - 98.04 %
      http://video.gaiamobile.org:8080/js/thumbnail_item.js - 89/89 - 100 %
      http://video.gaiamobile.org:8080/js/thumbnail_list.js - 67/67 - 100 %

I don't know how to make the last one in thumbnail_date_group. If I find a way to make it 100%, I will open a bug and make it.

Comment 14

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

I did it:
      http://video.gaiamobile.org:8080/js/thumbnail_date_group.js - 50/50 - 100 %
      http://video.gaiamobile.org:8080/js/thumbnail_item.js - 89/89 - 100 %
      http://video.gaiamobile.org:8080/js/thumbnail_list.js - 67/67 - 100 %

Testing coverage is 100% of my new introduced code. But that doesn't mean it is bug free.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 798723 [details]
separate the thumbnail list out and write most unit tests.

I'm really glad you're doing this refactoring, John.

This code looks good to me. I left a few comments on github. The only one that is serious is that I think you should use splice() in your function to remove event listeners. The delete operator does not do what you expect it to do for arrays.

I did not try the patch out myself, but I trust that you have tested it carefully.

Comment 16

[Rob MacDonald [:robmac]]

Comment on attachment 800569 [details]
bug908380-gallery-view.zip

Looks good. Thanks, John!

Comment 17

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

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