859284 - [Video Player] after completion of video playing in landscape mode time stamps for video files in list view are not displayed.

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : after completion of video playing in landscape mode time stamps for video files in list view are not displayed.
2. Precondition : Video player app running
3. Tester's Action : Play video file in landscape mode till end of the file.
4. Detailed Symptom (ENG.) : after completion of video playing in landscape mode time stamps for video files in list view are not displayed.
5. Gaia Revision # :   "8e7262e3d98ea9574d7f79c1e890ad80cfa40c27"
6. Expected : after completion of video playing in landscape mode time stamps for video files in list view should be displayed
7.Reproducibility: Yes
                1)Frequency Rate : 100%
8.Comparison Results : 
                1)Model Comparing : 
9. Attached files: 
                1)Log : 
                2)Test Contents : 
                3)Video file :

Comment 1

[Leo]

Attachment: Pointer to pull request

Comment 2

[Wayne Chang [:wchang]]

Triage 4/12 - Leo+ as seems to be a function failure (time information missing)

Comment 3

[David Flanagan [:djf]]

Comment on attachment 735772 [details]
Pointer to pull request

I'm asking Samuel to take this review. He's the one who added the textTruncate() function as part of bug 819062. I worry that removing the call to textTruncate() in the resize event handler, as this patch does, will fix this bug but break something else.  But I don't understand what textTruncate is doing well enough to know what will break.

Sam: would you take a look at this?  Is the patch okay? And if not, is there a bug in textTruncate itself?

(What is textTruncate for, by the way? Is it just a way to get an ellipsis at the end of a long video title since the CSS-based solution in inefficient? It seems like a lot of work for what is likely to be a real corner case)

Comment 4

[Sam Joch [:samjoch]]

Comment on attachment 735772 [details]
Pointer to pull request

Thanks to point this bug out.

You don't need to remove those lines that will broke the title truncation on resize.

See https://github.com/mozilla-b2g/gaia/pull/9091/files for review.
Please keep me inform if that fix the bug. I test it on my device.

David: textTruncate truncate a multiple line text and append duration at the end. So it's kind of ellipsis but for text.

Comment 5

[sarsenyev]

Attachment: screenshot_to_compare.img

Environmental  Variables:
Inari Build ID: 20130416070200
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6bac24e14538
Gaia: c883af5ecd0998f78d9aaa4c2337c842e1dbb5a0

The similar issue reproduces with "Inari" device.
Repro Steps:
1) Open "Video" application
2) Tap on video file for playback
3) While it's playing tap on pause or just tap on "<" button
4) Go back to the video files list

Actual result:
Total time is not displayed in the video file list

Expected result:
Total time is displayed in the video file list

Notes:
The issue reproduce in both: portrait and landscape views
see the screenshot

Comment 6

[Sam Joch [:samjoch]]

Attachment: Gaia PR

Comment 7

[David Flanagan [:djf]]

Comment on attachment 738959 [details]
Gaia PR

This patch fixes this bug but re-introduces the bug that this whole manual text formatting thing was there to fix.

If you have a movie with a really long name that fits in landscape mode but does not fit in portrait mode and you start playing the movie in landscape, then rotate to portrait and then go back to the list of videos, the title text will not be reformatted, and it will overflow its container instead of being truncated with ...

So if you're going to ignore resize events while the video list is hidden, you also have to add code to reformat the list every time you re-display it again.

Comment 8

[Leo]

Attachment: Pointer to pull request

Please review patch.

Comment 9

[David Flanagan [:djf]]

Comment on attachment 741793 [details]
Pointer to pull request

r- for the same reason as the last patch. See comment 7.  If you ignore the resize event then, when you go back to the thumbnail list, the text may be improperly formatted. 

In addition to the code you have so far, you must also reformat the text when you return to thumbnail mode, if the size has changed.

Comment 10

[Sam Joch [:samjoch]]

Working with Vivien on this, here is a nicer solution instead of textTruncate.
(There maybe issue cause 'deviceorientation' event doesn't act as expected).

https://github.com/mozilla-b2g/gaia/pull/9271/files

We use the 'overflow' event of an element and force it to repaint again as long as the content fit the container. This should be take care with an ellipsis. I file a bug for it.

https://bugzilla.mozilla.org/show_bug.cgi?id=866102

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Not sure what info you need from me ?

Comment 12

[Leo]

Attachment: Pointer to Pull request

Made changes in 
1. video.js file for string length comparison.
2. Added word break in details class of video.css
3. added display block in After class to show time duration in a new line

Comment 13

[Sam Joch [:samjoch]]

Hey Leo: We already try to use the 'resize' events but it is trigger many times during the rotation of the device. That's why i use the orientation variable with the 'deviceorientation' events which is more appropriate. Also having two differents pr for this bug may be confusing, can you just remove your PR and leave comment so we can be able to improve the patch, thanks.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 743566 [details]
Pointer to Pull request

I just closed this pull request because Sam's work in the other attachment is closer to what we want, I think.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 738959 [details]
Gaia PR

I didn't know about the overflow event. I love that you can replace the old code with this overflow-based code.

But I'm giving r- because using deviceorientation is not appropriate here, and also because I think you can simplify the overflow code significantly.  See my comments on github.

Comment 16

[Leo]

My pull request was closed

Comment 17

[Sam Joch [:samjoch]]

Comment on attachment 738959 [details]
Gaia PR

David, thanks for your review.

I removed the counter, and use the resize event.

The reset of the overflow is a trick to bind the 'overflow' event. Actually it works outsize the function that is triggered by the events. 

You should try with a single very long word without the timeout the 'overflow' event is not triggered.

What do you think ?

Comment 18

[David Flanagan [:djf]]

Sam,

The code is looking really good. I've added comments on github.

Comment 19

[Dylan Oliver [:doliver]]

Sam, can you provide current status on this bug?

Comment 20

[Sam Joch [:samjoch]]

David: i added a clearTimeout on the 'underflow' event as you suggested. Let me know.

Comment 21

[David Flanagan [:djf]]

Sam: I don't remember suggesting that. I'm not sure what the particular issue you're trying to fix is, but I don't understand why you'd need the clearTimeout(). Isn't it safe to leave the element with overflow:hidden even if it no longer overflows?  If you can make it work without an underflow handler that would be simpler and better.

I haven't tested this yet, but assuming it works right, I'm basically ready to give an r+, especially if you can remove the clearTimeout and get rid of the underflow handler completely.

Sorry about my video patch landing before yours did. That must have been tricky to merge since I split up the file...

Comment 22

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

Is this bug (https://bugzilla.mozilla.org/show_bug.cgi?id=840009) related to this?
The outcome is similar...

Comment 23

[Sam Joch [:samjoch]]

David: Ok the PR is ready.

John: You should mark it as duplicate.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 738959 [details]
Gaia PR

The code looks good, and seems to work well.  It is so nice having all that other formatting code gone!  Thanks!

Comment 26

[David Flanagan [:djf]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/9947ab827649f9dc7b0a4c4e796fefb5c02d64dd

Comment 27

[David Flanagan [:djf]]

Uplifted to v1-train: https://github.com/mozilla-b2g/gaia/commit/b1c0bb2a3c2a501791d27906fdc5d8c40ddeca34

Comment 29

[:CSchmoeckel]

Added Video Suite Test Case #8793 - [B2G Video] Verify that after a video in landscape mode ends that the Video List resumes normally