902995 - [Video] [User Story] Display file information for each of the videos stored in the video app

Status: RESOLVED FIXED

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

Description

[Sri Kasetti]

User Story:

As a user, I want to see the file type, size, resolution, date of creation and duration of the video for each of the videos stored in the video application

Comment 1

[Punam Dahiya [:pdahiya]]

Hi Patryk

Please attach the visual design specs for this user story. I will be needing an info icon image for this feature.

Sri, can you pl. provide details such as which strings needs to be localized while displaying video information or let me know who should i reach out for the same.

I have the specs from UX in the email. Let me know if i should attach that to the bug. 

Thanks

Comment 2

[Punam Dahiya [:pdahiya]]

Including Rob for UI question on gallery view. 

when we display file size and video type we are showing it below duration in a single line. In few cases when the file size text is big e.g 2224 KB, the video type moves in the next line (See attached). Is this ok or we need to always show it in one line?
Please clarify

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Video app - gallery view image displaying video type in next line

Comment 4

[Punam Dahiya [:pdahiya]]

(In reply to Punam Dahiya from comment #2)
> Including Rob for UI question on gallery view. 
> 
> when we display file size and video type we are showing it below duration in
> a single line. In few cases when the file size text is big e.g 2224 KB, the
> video type moves in the next line (See attached). Is this ok or we need to
> always show it in one line?
> Please clarify

Got clarification from Rob on IRC and it's ok for video type text to wrap to new line in gallery view.

Comment 5

[Patryk Adamczyk [:patryk] UX]

Attachment: Specs + Icon

Hi, can you please have a look at the specs, judging from the 1.1 builds, the gallery view and the player seem to be off.

Comment 6

[Punam Dahiya [:pdahiya]]

Thanks Patryk, the specs look good. Grouping of videos by date in gallery view is outside the scope and will be covered in a separate bug.

Comment 7

[Punam Dahiya [:pdahiya]]

Attachment: Pull request to display file info for videos stored in video app

Comment 8

[Punam Dahiya [:pdahiya]]

(In reply to Punam Dahiya from comment #1)
> Hi Patryk
> 
> Please attach the visual design specs for this user story. I will be needing
> an info icon image for this feature.
> 
> Sri, can you pl. provide details such as which strings needs to be localized
> while displaying video information or let me know who should i reach out for
> the same.
> 
> I have the specs from UX in the email. Let me know if i should attach that
> to the bug. 
> 
> Thanks

All the field labels and button labels on video info screen and size units are localized.

Comment 9

[Punam Dahiya [:pdahiya]]

Adding Patryk and Sri for UX and Product review. Thanks

Comment 10

[David Flanagan [:djf]]

Comment on attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app

Reassigning the review to John, as I won't have time to look at it for the next few days, and because John has been kind enough to volunteer to take some reviews from me :-)  (Thanks, John!)

John: Punam and I have discussed the addition of a the media_utils.js file to the shared/js/media directory, and I like the idea. She'll be using those utilities for Gallery as well.

Comment 11

[Patryk Adamczyk [:patryk] UX]

Hey Punam I loaded the fix, but for some reason the device now can't read any SD cards, so I can't review the player.
Can you just post some screenshots? Thanks.

Comment 12

[Punam Dahiya [:pdahiya]]

Attachment: Screen Shots of implemented video info screen, player and gallery view

Comment 13

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

Comment on attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app

This is a nice patch. Thanks for the test case that are the first test case of video app. I think most of things are correct, except the followings:
1. make code more readable js video.js
2. do not use the magic 7rem on last-child. try to use calc on height of ul to have the correct height.
3. please also change view.js to use media_utils to formatDuration.

I will give r+ with those three items are fixed.

I also put some suggestions on github about css and timezone of test case. If I am wrong, please tell me.


And final question: do we need to land 1.5x image in this patch??? How do you feel, Patryk?

Comment 14

[Punam Dahiya [:pdahiya]]

Attachment: Screen Shots of implemented video info screen, player and gallery view

Screen Shots of updated video info screen , video player and gallery view

Comment 15

[Patryk Adamczyk [:patryk] UX]

Attachment: review.png

I think its close, but there are things off can you please look at the spec, and make sure everything is align... top issues are:
A. In Gallery view, the divider shouldn't extend to the edge of the screen.
B. In info view, the white text needs to be higher within the row
C. In info view, the button should have a dark grey background
D. In player view, the player should be flush with the divider like (>) shouldn't have a gap.
E. The scrubber should be longer.

Comment 16

[Punam Dahiya [:pdahiya]]

(In reply to Patryk Adamczyk [:patryk] UX from comment #15)
> Created attachment 796196 [details]
> review.png
> 
> I think its close, but there are things off can you please look at the spec,
> and make sure everything is align... top issues are:
> A. In Gallery view, the divider shouldn't extend to the edge of the screen.
> B. In info view, the white text needs to be higher within the row
will try to adjust paddings below texts to raise the content within the row

> C. In info view, the button should have a dark grey background
can you pl provide the color code of dark grey background?

> D. In player view, the player should be flush with the divider like (>)
> shouldn't have a gap.
Are you referring to the play (>),duration and slider controls at the bottom of the player view and gap/difference in combined view of attachment? 

> E. The scrubber should be longer.
By scrubber you mean , slider straight line joining the time durations in video player. Am i correct?

Thanks Patryk for your feedback, i will try to address top issues. It will help if you can answer questions posted inline.

Comment 17

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

Patryk, 

I have another question for you. Do we need to land 1.5x image with the same bug??

Comment 18

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

(In reply to Punam Dahiya from comment #16)
> (In reply to Patryk Adamczyk [:patryk] UX from comment #15)
> > Created attachment 796196 [details]
> > review.png
> > 
> > I think its close, but there are things off can you please look at the spec,
> > and make sure everything is align... top issues are:
> > A. In Gallery view, the divider shouldn't extend to the edge of the screen.

Well, this is not your problem. We have that kind of UI from the version 1.1. But since patryk mentioned it, please also change it.

> > B. In info view, the white text needs to be higher within the row
> will try to adjust paddings below texts to raise the content within the row
> 
> > C. In info view, the button should have a dark grey background
> can you pl provide the color code of dark grey background?

You should see the building block for it: http://buildingfirefoxos.com/building-blocks/action-menu.html .

If you can change your implementation based on building block, it will be better. But we don't have a simple dialog or list dialog, like android, for this case. So, don't stuck yourself in it.

> > D. In player view, the player should be flush with the divider like (>)
> > shouldn't have a gap.
> Are you referring to the play (>),duration and slider controls at the bottom
> of the player view and gap/difference in combined view of attachment? 

Well, this is not your problem, again. Please help to change it.

> > E. The scrubber should be longer.
> By scrubber you mean , slider straight line joining the time durations in
> video player. Am i correct?

I think you are right, please find the bottom right image of Patryk's review. The horizontal line from duration to total time should be longer.

Well, this is not your problem, again. Please help to change it.

> Thanks Patryk for your feedback, i will try to address top issues. It will
> help if you can answer questions posted inline.

Comment 19

[Punam Dahiya [:pdahiya]]

Thanks John for your feedback. I have updated pull request with your review comments in https://bugzilla.mozilla.org/show_bug.cgi?id=902995#c13. Please review.

Patryk, 

I have implemented fix of B and C from https://bugzilla.mozilla.org/show_bug.cgi?id=902995#c15 in the updated PR

I looked into A, D and E UI fixes and those needs more time for fixing and testing in both portrait and landscape mode. I would like to fix A, D and E as a separate bug and have created https://bugzilla.mozilla.org/show_bug.cgi?id=910094 for that.

Attaching the video info screen screen shot with fix of B and C from #c15. Please review. Thanks

Comment 20

[Punam Dahiya [:pdahiya]]

Attachment: Video Info Screen Shot with the fix of B and C UI review feedback in #c15

Comment 21

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

Comment on attachment 794218 [details] [review]
Pull request to display file info for videos stored in video app

Punam, 

Thanks for this patch and the good test case for timezone side effect. It looks good. I give this patch r+. r=me.

But there is a small risky place that is to check the position of cutting the 'video/' from mimetype out. You should use indexOf to find the position '/' instead of hardcode the number. Although the mime type may always be video/*, we can't make sure it is always to be that in the future. Please change it before land the code.

Comment 22

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

Punam, 

I don't know if you have the authority to land the code. If no, please ping me and I can help you when everything is ready (got r+ from patryk and sri).

Comment 23

[Punam Dahiya [:pdahiya]]

Attachment: Video Info Screen with UI fixes and Indented text content

Comment 24

[Punam Dahiya [:pdahiya]]

Attachment: Video-Info-Screen updated indentation and UI fixes

Comment 25

[Patryk Adamczyk [:patryk] UX]

Comment 24: Looks good ship it.

Comment 26

[Patryk Adamczyk [:patryk] UX]

Attachment: actionicon_media_info_45x45.png

Action icon @1.5x

Comment 27

[Patryk Adamczyk [:patryk] UX]

Attachment: actionicon_media_info_45x45.png

Optimized

Comment 28

[Punam Dahiya [:pdahiya]]

(In reply to John Hu [:johnhu] from comment #22)
> Punam, 
> 
> I don't know if you have the authority to land the code. If no, please ping
> me and I can help you when everything is ready (got r+ from patryk and sri).
Hi John
I have updated the PR with  your feedback to use indexOf to get the position of '/'.
Pull request is also updated with 1.5x image and small css fixes for UI changes from Patryk. 

We have a go from Patryk, please land the code in master. Thanks

Comment 29

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

Thank you, Punam.

The patch is landed to master:

https://github.com/mozilla-b2g/gaia/commit/7c211ce75b15326e1a8fdf41cbbb3c490b9100be