Closed
Bug 951025
Opened 11 years ago
Closed 11 years ago
[Video]Fast forward/Rewind option in Video app
Categories
(Firefox OS Graveyard :: Gaia::Video, defect)
Tracking
(feature-b2g:2.0)
RESOLVED
FIXED
feature-b2g | 2.0 |
People
(Reporter: vsireesha246, Assigned: vsireesha246)
References
Details
(Whiteboard: [m+][g+])
Attachments
(9 files, 16 obsolete files)
(deleted),
application/zip
|
Details | |
(deleted),
application/pdf
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/html
|
rnicoletti
:
review+
johnhu
:
review+
tif
:
ui-review+
|
Details |
(deleted),
application/zip
|
vsireesha246
:
review-
|
Details |
(deleted),
image/png
|
vsireesha246
:
review-
|
Details |
(deleted),
application/zip
|
Details | |
(deleted),
application/x-rar
|
Details |
1.There is no forward/Rewind buttons in video app to fast forward and Rewind the current playing video.
Proposed Scenario:Video app should have the forward and rewind buttons like in music app.
2.Like Music app on single touch on FF/Rwd buttons move the next music file and on long touch on FF/Rwd buttons will FF/Rwd the current music file.
Similarly same behavior should be present in Video app.
Please suggest any change related to the above two features.
Assignee | ||
Comment 1•11 years ago
|
||
Would you please provide the feedback for this issue.
Flags: needinfo?(wchang)
Comment 2•11 years ago
|
||
Over to Sri.
However as IIRC we had a discussion on this and given there is the progress bar where you can seek it is not essential to have FWD or RWD buttons.
Flags: needinfo?(wchang) → needinfo?(skasetti)
Assignee | ||
Comment 3•11 years ago
|
||
This issue UX can be implemented in two ways.
1.Similar to Music app(refer the attached screenshot)
2.By reducing the seek-bar size and add the Previous button to left of seek-bar and next button to right.
So please suggest me which UX i need to follow to implement this issue.
As this is the the mandatory item for MADAI,i will upload the patch once you conform me about UX spec.
Flags: needinfo?(wchang)
Assignee | ||
Comment 4•11 years ago
|
||
Comment 6•11 years ago
|
||
If I recalled correctly the UX of video player is Chris who is already in the cc'ing list.
Yes. I just cc Chris.
My comment is that it is good to have FF/REW in video app.
1.Same behavior as Music.
2.Easy for navigation. ( In current video, user always has to go back to video list to select next or another clip.
p.s. But for the video player in Gallery, we shall keep current implementation.
Thanks.
Assignee | ||
Comment 8•11 years ago
|
||
Would you please review the attached Work in Progress patch and provide me the review comments.
Thanks,
Sireesha
Attachment #8356058 -
Flags: review?(johu)
Comment 9•11 years ago
|
||
It's an UI change of video player. Let's ask Chris about the proposed UI first.
Chris, may you take a look at the attached UI at comment 4 and give us some input about the changes of this feature?
Flags: needinfo?(clam)
Comment 10•11 years ago
|
||
BTW,
I can't make sure the different between this bug and bug 948260. Are they duplicated??
Assignee | ||
Comment 11•11 years ago
|
||
This bug and bug-948260 are almost similar.
But in this bug,i added one more feature,like on long press on FF/Rwd buttons we can do seeking also.
Comment 12•11 years ago
|
||
IMHO, the bug 948260 is for previous/next video navigation and this one is for fast forwarding or rewinding within a video. So, we should still view them as two separate features.
Comment 13•11 years ago
|
||
UX did the review on the patch (using Hamachi) and here is the feedback :
In the Video, the FWD and REW icons are not visible but it works when I touch on the right and left side of the Play button.
Issue :
In the patch version, if you long-press FWD, it fastforward to the end and start from beginning and keep fast forwarding as a loop.
Proposed Scenario :
Long press FWD - It should fastforward action should stop at the end of the timeline. Just like Music app.
When long press REW, it rewinds all the way back to the beginning and stops there. This is correct, the same as Nusic app.
Thanks.
Mike
Assignee | ||
Comment 14•11 years ago
|
||
I modified the code as per the review comments and attached the png files in the attachment.
Attachment #8356058 -
Attachment is obsolete: true
Attachment #8356058 -
Flags: review?(johu)
Attachment #8356997 -
Flags: review?(mtsai)
Comment 15•11 years ago
|
||
I thought we should have the wireframes and visual design first then implement this? and I agree with John we should treat bug 948260 and this bug as two separate features, or it will become a huge patch which is not easy to be uplifted.
Comment 16•11 years ago
|
||
Feedback after reviewing the patch again :
The long-press FWD issue has not be solved yet. It needs to align with Music app FWD long-press behavior.
Ni? Chris and Rob who are in charge of the spec for advice.
Please take over this issue since you have come back from vacation just now.
Thank you.
Flags: needinfo?(clam) → needinfo?(rmacdonald)
Comment 17•11 years ago
|
||
Comment on attachment 8356997 [details]
Bug_951025.zip
Feedback after reviewing the patch again :
The long-press FWD issue has not be solved yet. It needs to align with Music app FWD long-press behavior.
Ni? Chris and Rob who are in charge of the spec for advice.
Please take over this issue since you have come back from vacation just now.
Thank you.
Attachment #8356997 -
Flags: review?(mtsai) → review?(rmacdonald)
Comment 18•11 years ago
|
||
Comment on attachment 8356997 [details]
Bug_951025.zip
As I commented in comment 15, let's wait for Chris or Rob to give us some inputs, and if vsireesha246@gmail.com wants to get the patch into the review process, it should go to ui-review first, not the regular review for devs, thanks.
Attachment #8356997 -
Flags: review?(rmacdonald)
Updated•11 years ago
|
Flags: needinfo?(clam)
Comment 19•11 years ago
|
||
vsireesha246: this feature is currently in the plan for our upcoming release (1.4). mozilla ui folks are working on coming up with the right ux proposal. once we have the recommendation from ux you can certainly help with the implementation (since you have already started on it).
thanks
hema
Comment 20•11 years ago
|
||
Hi everyone...
Jacqueline and I are working on a proposal and will post the results to this bug on Thursday.
Thanks!
Flags: needinfo?(clam) → needinfo?(jsavory)
Comment 21•11 years ago
|
||
Hello,
I've attached the proposal for this feature, please let me know if anyone has any questions. Thanks!
Flags: needinfo?(jsavory)
Updated•11 years ago
|
Flags: needinfo?(rmacdonald)
Comment 23•11 years ago
|
||
NI for Peter to provide any visuals for this proposal.
Flags: needinfo?(pla)
Comment 24•11 years ago
|
||
(In reply to jsavory from comment #21)
> Created attachment 8368359 [details]
> Video_Jan2014.pdf
>
> Hello,
>
> I've attached the proposal for this feature, please let me know if anyone
> has any questions. Thanks!
I have some opinions as below.
1. 10 seconds jump may be too big.
User can simply use seek to do that. Besides normally the duration of videos recorded by phone is not long. Probably within several ten seconds. In this cases, only several frames would be displayed. One way to have a smooth fast-forward or rewind is to play video key-frames only.
2. Long press FWD/RWD
Maybe it is not necessary to be consistent with music app. As I found some video players, like Apple QuickTime that if you want to play it in FWD, just press once. And you can go back to normal speed by pressing play/pause button. Would it be much easier for user?
Comment 25•11 years ago
|
||
Hi all,
About swipe to navigate, it may not easy to implement. Because we only have one video decoder in our devices. We may need to use a thumbnail image for another video or use two thumbnail images for both video files. The video player in video app is different than gallery app. We use video element to load the video file directly instead of loading thumbnail image first. While swiping, gallery app uses two images to do the transition. But in video app, we may need to pause the video element and load another image for swiping. There may be two issues here:
1. which image do we use and the image quality
The only thumbnail image we current have is the image for list view. That image is created at a fixed resolution, 210x120. The algorithm of thumbnail image is to fit the video file into a 210x120 rectangle. When the video's ratio is not the same as 210x120, it fills black at the other place. So, a portrait video will be scaled down to fit into 120px in its hight, and the scale ratio will be high. If we use a portrait video in landscape layout, the quality of this image may be bad. In gallery app, it respects video resolution ratio to create thumbnail image and use css to place the thumbnail image into the list view. If we use the same algorithm here, it may give us an acceptable image quality in fullscreen view and may have a manageable scaling ratio.
2. loading performance
If we use one image and one video for swiping, we may need to handle the time of video loaded and first frame decoded while swiping quickly. To compare the loading time of image and video, I may prefer to use two images for swipe transition. After the transition is finished, we use video element to load the video file but not auto-play. The timing of video loading may be an issue here.. If we use two images for transition, the timing to load the video file may be critical when user swipe quickly. In gallery app, we load the video when user tap the play button. We may use the same policy to do so.
(In reply to jsavory from comment #21)
> Created attachment 8368359 [details]
> Video_Jan2014.pdf
>
> Hello,
>
> I've attached the proposal for this feature, please let me know if anyone
> has any questions. Thanks!
Assignee | ||
Comment 26•11 years ago
|
||
Hi John,
Would you please help me to get the approval for resources and code review.
As of now i attached PR for work in progress patch.
Seeking having some issues like in Comment#24.
Thanks..
Sireesha
Attachment #8371446 -
Flags: feedback?(johu)
Assignee | ||
Comment 27•11 years ago
|
||
This PR contains know issues like
1.FF/RWD buttons alignment
2.More options screen(Share,Delete,Info) don't include Icons
3.Seeking behaviour is too fast.
(In reply to vsireesha246 from comment #26)
> Created attachment 8371446 [details]
> Pointer to Pull Request.html
>
> Hi John,
>
> Would you please help me to get the approval for resources and code review.
> As of now i attached PR for work in progress patch.
>
> Seeking having some issues like in Comment#24.
>
> Thanks..
> Sireesha
Assignee | ||
Comment 28•11 years ago
|
||
Navigating between Previous and Next Video(Bug 948260)implementation needs the below changes.
1.Like Gallery app,Video app needs to maintain an global array and this array is initially filled when we enumerate video database, and has elements added and removed when we receive create and delete events from the media databases.
2.Like Gallery app,Video app UI need to maintain 3 <div> elements for prev,current and next Videos be to displayed.But current video app UI is using single <video> element.
3.Video app need to include transition related css similar to Gallery app.
Along with the below points mentioned by John,prev/next video navigation of UI spec is little complex and need more code changes in Video app.
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #25)
> Hi all,
>
> About swipe to navigate, it may not easy to implement. Because we only have
> one video decoder in our devices. We may need to use a thumbnail image for
> another video or use two thumbnail images for both video files. The video
> player in video app is different than gallery app. We use video element to
> load the video file directly instead of loading thumbnail image first. While
> swiping, gallery app uses two images to do the transition. But in video app,
> we may need to pause the video element and load another image for swiping.
> There may be two issues here:
>
> 1. which image do we use and the image quality
> The only thumbnail image we current have is the image for list view.
> That image is created at a fixed resolution, 210x120. The algorithm of
> thumbnail image is to fit the video file into a 210x120 rectangle. When the
> video's ratio is not the same as 210x120, it fills black at the other place.
> So, a portrait video will be scaled down to fit into 120px in its hight, and
> the scale ratio will be high. If we use a portrait video in landscape
> layout, the quality of this image may be bad. In gallery app, it respects
> video resolution ratio to create thumbnail image and use css to place the
> thumbnail image into the list view. If we use the same algorithm here, it
> may give us an acceptable image quality in fullscreen view and may have a
> manageable scaling ratio.
>
> 2. loading performance
> If we use one image and one video for swiping, we may need to handle the
> time of video loaded and first frame decoded while swiping quickly. To
> compare the loading time of image and video, I may prefer to use two images
> for swipe transition. After the transition is finished, we use video element
> to load the video file but not auto-play. The timing of video loading may be
> an issue here.. If we use two images for transition, the timing to load the
> video file may be critical when user swipe quickly. In gallery app, we load
> the video when user tap the play button. We may use the same policy to do so.
>
>
>
> (In reply to jsavory from comment #21)
> > Created attachment 8368359 [details]
> > Video_Jan2014.pdf
> >
> > Hello,
> >
> > I've attached the proposal for this feature, please let me know if anyone
> > has any questions. Thanks!
Comment 29•11 years ago
|
||
Comment on attachment 8371446 [details]
Pointer to Pull Request.html
This patch works. But I feel it need to be refactored to have a separate file to handle this feature. And there are some few css nits that I already marked in PR.
As you mentioned, the forward/rewind step seems incorrect when we long press it. Maybe, we need to change to use seek and seekdone event instead of setInterval.
Attachment #8371446 -
Flags: feedback?(johu) → feedback+
Comment 30•11 years ago
|
||
Moreover, the performance of each seek should not be consistent. This is because now seek is a precise seeking which means if the seeking time is not close to key-frame, it takes some time to decode from the nearest key-frame to the seeking time. This performance problem could be observable easily (ex. the playback speed of long-press FWD should be stable and smooth), unless bug 778077 is fixed or Gaia can find the time of key-frame and seek to that time.
Comment 31•11 years ago
|
||
Thanks Blake,
I can understand the performance issue. We should need info Jacqueline to give us her thought.
Hi Jacqueline,
Please find the comment 24 and comment 30 to understand the concern of blake. And I feel we should discuss the previous/next navigation at bug 948260, since Sireesha has some concerns about the implementation. I will also move my comment 25 to there, too.
Flags: needinfo?(jsavory)
Assignee | ||
Comment 32•11 years ago
|
||
Hi John,
I squashed the review comments for js changes.
I will do css changes once i will get UX input.
So please review the js changes and let me know your comments.
Thanks...
Sireesha
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(johu)
Assignee | ||
Comment 33•11 years ago
|
||
Hi Eric,
Would you please help me for Visual spec(resources) for this bug.
I have some review comments for this bug related to css at https://github.com/mozilla-b2g/gaia/pull/16035/files.
Thanks..
Sireesha
Flags: needinfo?(echang)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(echang)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(epang)
Comment 34•11 years ago
|
||
Also adding Rob since Jacqueline is away this week
Flags: needinfo?(rmacdonald)
Comment 35•11 years ago
|
||
We had discussed at IRC and Github. Clear the NI.
(In reply to vsireesha246 from comment #32)
> Hi John,
>
> I squashed the review comments for js changes.
> I will do css changes once i will get UX input.
>
> So please review the js changes and let me know your comments.
>
> Thanks...
> Sireesha
Flags: needinfo?(johu)
Assignee | ||
Comment 36•11 years ago
|
||
Hi John,
Would you please review the css & js part for this bug.
I added some of your review changes and action menu with icon changes.
Thanks..
Sireesha.
Comment 37•11 years ago
|
||
clearing need info on my since Peter has already been flagged.
Flags: needinfo?(epang)
Comment 38•11 years ago
|
||
Hi guys,
Sorry for the delay in chiming in here. I am currently trying to find a designer to take this on, we will be addressing this shortly and posting specs asap.
Peter.
Flags: needinfo?(jsavory)
Updated•11 years ago
|
Flags: needinfo?(rmacdonald)
Comment 39•11 years ago
|
||
Just a quick update - we have Peko working on this today, and Hung will refine and finish it off by EOD Wednesday, at which point we'll post the specs.
These specs shouldn't hold up development though - the functional things can still be coded, but once the specs are up, you'll know exactly where to place things, have final graphics assets, etc.
Peter.
Flags: needinfo?(pla)
Assignee | ||
Comment 40•11 years ago
|
||
Hi Peter,
I am already working on it and uploaded the PR for the same.I got the Review comments related to UX alignment along with some JS changes.I done the JS Review changes and i am waiting for UX input from you and the squash review changes need to be reviewed by Johnhu.
Hi John,
Currently I am working on Unit testcases.Would you please review the JS changes,so that i can add the some more Unit testcase for this bug.
(In reply to Peter La from comment #39)
> Just a quick update - we have Peko working on this today, and Hung will
> refine and finish it off by EOD Wednesday, at which point we'll post the
> specs.
>
> These specs shouldn't hold up development though - the functional things can
> still be coded, but once the specs are up, you'll know exactly where to
> place things, have final graphics assets, etc.
>
> Peter.
Comment 41•11 years ago
|
||
Hi Sireesha,
I had read your code with the PR here. This version is better. But I found the following issues:
1. We may need to have the same feature at view.js (open activity).
2. Show/hide options menu should be moved to video.js
3. Since we already give seekForward and seekRewind to controller, we may add event listeners inside of controller.
BTW, I still feel we need to wait for Peter's input to have correct image and layout.
Assignee | ||
Comment 42•11 years ago
|
||
Hi John,
Thank You for your review comments.
Sorry i missed to add in view.js.
I squash the below review changes.
Would you please review it once again.
Thanks..
Sireesha
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #41)
> Hi Sireesha,
>
> I had read your code with the PR here. This version is better. But I found
> the following issues:
>
> 1. We may need to have the same feature at view.js (open activity).
> 2. Show/hide options menu should be moved to video.js
> 3. Since we already give seekForward and seekRewind to controller, we may
> add event listeners inside of controller.
>
> BTW, I still feel we need to wait for Peter's input to have correct image
> and layout.
Flags: needinfo?(johu)
Assignee | ||
Comment 43•11 years ago
|
||
Hi John,
I added 2 Unit testcases for OptionView(newly added screen).
Is it fine or i need to added any other Unit testcases.
I think for FF/Rwd instead of Unit testcases,Gaia-UI testcases(Marionette) we need to write.
Comment 44•11 years ago
|
||
Hi Sireesha,
Apologies for the delay. Attached please find the visual specs and icon assets for the video controls @1, @1.5 and @2 times resolutions.
We will load the patch and check the implementation on Monday.
Peter
Comment 45•11 years ago
|
||
Hi Sireesha,
Please update the PR to have all information from Peter La. I had checked the PR and Spec carefully again. I found:
1. We only implement the press and hold case but not tap case. We need to add them back.
2. The implementation of press and hold may be not exactly the same as spec. We still need to improve that.
3. As to unit tests, you are correct, we need to add more cases for Forward/Rewind controller.
Flags: needinfo?(johu)
Assignee | ||
Comment 46•11 years ago
|
||
Hi John,
As discussed in IRC i tried to use playBackrate and its not working for me.Same i updated in github.So i used 1000ms instead of 15ms.
Remaing review comments are modified and squashed.
If you are ok with the js part i will start doing the css part given by Peter La.
So,please review it.
Thanks...
Sireesha
Flags: needinfo?(johu)
Assignee | ||
Comment 47•11 years ago
|
||
Comment on attachment 8371446 [details]
Pointer to Pull Request.html
Hi Peter La,
Would you please review the UI-Changes and let me know your review comments.
Hi John,
Would you please let me know your review commets for the functionality and css.
Thanks..
Sireesha
Attachment #8371446 -
Flags: ui-review?(pla)
Attachment #8371446 -
Flags: review?(johu)
Attachment #8371446 -
Flags: feedback-
Attachment #8371446 -
Flags: feedback+
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(johu)
Assignee | ||
Comment 48•11 years ago
|
||
Hi,
I have one doubt in page 5 Fat Farward and Rewind.
In case of Hold: according to screenshot 3 and 4, if user hold for 3secs then the seeking should increase 3sec*10 times?
Would you please clarify Hold case...
(In reply to jsavory from comment #21)
> Created attachment 8368359 [details]
> Video_Jan2014.pdf
>
> Hello,
>
> I've attached the proposal for this feature, please let me know if anyone
> has any questions. Thanks!
Flags: needinfo?(jsavory)
Comment 49•11 years ago
|
||
Hi Sireesha,
I just reviewed your patch and noticed there are some layout issues. We also discussed within UX that we'll need a small IxD change.
Both Jacqueline and I will be posting updates to our specs tomorrow, and I will also post a document outlining the layout issues that I've seen.
Stay tuned.
Peter.
Comment 50•11 years ago
|
||
Hi Sireesha,
I've attached an updated spec that has changed the layout issue mentioned by Peter and removed the previous/next case.
Regarding your comment, the fast forward and rewind speed will be x10 the regular speed of play. Let me know if this is considered too fast or slow and we can discuss another rate.
Flags: needinfo?(jsavory)
Assignee | ||
Comment 51•11 years ago
|
||
Hi Peter La,
I modified the UX according to Video_Mar2014.pdf.
Would you please review the UX changes once again and let me know your comments.
Thanks..
Sireesha
(In reply to Peter La from comment #49)
> Hi Sireesha,
>
> I just reviewed your patch and noticed there are some layout issues. We
> also discussed within UX that we'll need a small IxD change.
>
> Both Jacqueline and I will be posting updates to our specs tomorrow, and I
> will also post a document outlining the layout issues that I've seen.
>
> Stay tuned.
> Peter.
Flags: needinfo?(pla)
Assignee | ||
Comment 52•11 years ago
|
||
Hi,
Case1:The normal playBackRate=1.You mean to say in Hold case we need to increase playBackRate=10?
Case2:But according to the UX spec given by you it says..user holds for 3secs,it forwarded to 3*10=30sec.
So i need to consider case1 or case2?
If case2 is correct,then what about 3sec holded duration by user..means in this 3sec holding time video should play or pause? and we need to forward the play at a time..means suppose user holds for 3sec then 3*10=30sec at a time we need to forward/rwd?
(In reply to jsavory from comment #50)
> Created attachment 8385749 [details]
> Video_Mar2014.pdf
>
> Hi Sireesha,
>
> I've attached an updated spec that has changed the layout issue mentioned by
> Peter and removed the previous/next case.
>
> Regarding your comment, the fast forward and rewind speed will be x10 the
> regular speed of play. Let me know if this is considered too fast or slow
> and we can discuss another rate.
Assignee | ||
Comment 53•11 years ago
|
||
(In reply to vsireesha246 from comment #52)
> Hi,
>
> Case1:The normal playBackRate=1.You mean to say in Hold case we need to
> increase playBackRate=10?
> Case2:But according to the UX spec given by you it says..user holds for
> 3secs,it forwarded to 3*10=30sec.
>
> So i need to consider case1 or case2?
> If case2 is correct,then what about 3sec holded duration by user..means in
> this 3sec holding time video should play or pause? and we need to forward
> the play at a time..means suppose user holds for 3sec then 3*10=30sec at a
> time we need to forward/rwd?
>
>
> (In reply to jsavory from comment #50)
> > Created attachment 8385749 [details]
> > Video_Mar2014.pdf
> >
> > Hi Sireesha,
> >
> > I've attached an updated spec that has changed the layout issue mentioned by
> > Peter and removed the previous/next case.
> >
> > Regarding your comment, the fast forward and rewind speed will be x10 the
> > regular speed of play. Let me know if this is considered too fast or slow
> > and we can discuss another rate.
Flags: needinfo?(pla) → needinfo?(jsavory)
Comment 54•11 years ago
|
||
Hi Sireesha,
I'm uploading a slightly modified Visual Spec to account for the change in the menu in the header, as well as fix a couple of small issues from before.
Please refer to this spec for your implementation. In particular:
1) The header bar should be black with 85% opacity, rather than the current dark grey, fully opaque header that is in the implementation.
2) The bottom toolbar that houses the playback controls should be 45px high. It is currently 40px in your patch that I reviewed.
3) The play controls in your patch are slightly off center (skewed to the right) and slightly low compared to the spec. Please check this and align with the spec.
4) The play button has a lighter rectangle behind it. This is not as spec'd. Please remove this.
Comment 55•11 years ago
|
||
Hi Sireesha,
Please refer to this image. It outlines the problems I have found when reviewing your patch. Please contact me if you need further clarification or assistance!
Thanks!
Peter
Comment 56•11 years ago
|
||
Updated with a couple more observations.
Attachment #8388299 -
Attachment is obsolete: true
Assignee | ||
Comment 57•11 years ago
|
||
Hi Peter La,
I updated the PR with your review comments.
But still alignment problem is present,may be due to play/pause images.
Would you please recheck and let me know your review comments(Bug_951025_Screenshots.7z).
I attached the screen shots with the review comments.
Thanks..
Sireesha
Attachment #8355159 -
Attachment is obsolete: true
Attachment #8356997 -
Attachment is obsolete: true
Attachment #8389080 -
Flags: ui-review?(pla)
Comment 58•11 years ago
|
||
Comment on attachment 8371446 [details]
Pointer to Pull Request.html
Sorry for late review. This version is far more better. I find some nits and questions in the PR. Please find them.
I clear the PR because the visual seems incorrect at the landscape mode. I will upload the image.
Thanks for this patch.
Attachment #8371446 -
Flags: review?(johu)
Comment 59•11 years ago
|
||
The time slider and buttons are overlapping.
Comment 60•11 years ago
|
||
This patch breaks the tablet UI. We had met similar issue here. The minimum requirement of this kind issue is that we still have a way to use it. For tablet, we may hide the forward and rewind feature but we still need to have the way to play it. With the current patch, we cannot tap the play button at all.
Assignee | ||
Comment 61•11 years ago
|
||
Hi John,
I updated the PR with your review comments.
I have two small queries and same i mentioned in github.
So,review and let me know your review comments.
Thanks..
Sireesha
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #59)
> Created attachment 8389591 [details]
> control overlaps
>
> The time slider and buttons are overlapping.
Assignee | ||
Comment 62•11 years ago
|
||
Hi John,
Its better to added followup bug for all the issues related to fix patch.
Please let me know if your fine adding new for for the tablet.
Thanks..
Sireesha
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #60)
> Created attachment 8389598 [details]
> tablet UI broken
>
> This patch breaks the tablet UI. We had met similar issue here. The minimum
> requirement of this kind issue is that we still have a way to use it. For
> tablet, we may hide the forward and rewind feature but we still need to have
> the way to play it. With the current patch, we cannot tap the play button at
> all.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(johu)
Comment 63•11 years ago
|
||
For tablet case, we should not generate any regression. That's our policy.
We should do minimum efforts to keep that working. So, I think the only requirement is to
1. make "play" button work as usual,
2. turn off forward and rewind feature in tablet, just hiding the buttons,
3. file a bug to turn on forward and rewind feature in tablet.
Flags: needinfo?(johu)
Comment 64•11 years ago
|
||
I still find some nits and put them at PR. Please update it.
For tablet, please find the comment 63.
Assignee | ||
Comment 65•11 years ago
|
||
Hi John,
I updated the PR please check,i added hidden for FF/Rwd buttons for tablet.
But i am unable to verify it in simulator.
Simulator is showing upto "Add videos to get started screen".
If it possible to verify in simulator please let me know.
Thanks..
Sireesha
Flags: needinfo?(johu)
Comment 66•11 years ago
|
||
You may put ogv and webm files under your Movies folder under your account. So, the video app can list videos in its list view.
Flags: needinfo?(johu)
Assignee | ||
Comment 67•11 years ago
|
||
Hi John,
For Tablet with this patch Play,FF/Rwd buttons are not working and positions are not proper.
As per comment#c63 to make play button to work properly i need to know the proper resolutions in simulator and my doubt is due to this patch(code changes) or due to resources this is it stopped working.
So please let me know how can i proceed to make it work for tablet and to fix this bug completely.
Thanks..
Sireesha
Flags: needinfo?(johu)
Comment 69•11 years ago
|
||
Hi Sireesha,
Sorry for the confusing example! From your comment 52, I believe case 1 is what I'm trying to convey with my spec document. Thanks!
Flags: needinfo?(jsavory)
Comment 70•11 years ago
|
||
Hi Jacqueline,
About comment 52, what's the difference between 1. we tap ff/rwd button once and 2. hold them for 1 sec? For both case, they all jump 10 seconds.
With the implementation of Sireesha, it is more easy to tap it multiple times to jump 10 * n seconds then hold them for n seconds to have the same result.
Flags: needinfo?(jsavory)
Assignee | ||
Comment 71•11 years ago
|
||
Hi John,
I fixed the Tablet UI crash and now play,FF/Rwd buttons are working.
Play button alignment is still in Tablet also.
So,Please let me know your review comments for the PR.
Hi Peter La,
Can you please provide me the Ui-Review for Tablet also with this PR.
Thanks..
Sireesha
Flags: needinfo?(johu)
Updated•11 years ago
|
Whiteboard: [m+]
Comment 72•11 years ago
|
||
Hi John,
The idea behind having the two different functions is if the user quickly wants to jump ahead a bit they can tap the fast forward button, but if they want to fast forward through a larger section they can hold down the button for an extended period of time.
I don't necessarily think its easier to tap too many times to get through a larger section. However, through testing if there are recommendations in changing these rates, I am certainly open to hear them.
Flags: needinfo?(jsavory)
Comment 73•11 years ago
|
||
Removing dependency as Prev/Next track requirement was dropped.
No longer depends on: 948260
Assignee | ||
Comment 74•11 years ago
|
||
HI John,
I created new PR for this issue and please review it.
Thanks,
Sireesha
Attachment #8371446 -
Attachment is obsolete: true
Attachment #8371446 -
Flags: ui-review?(pla)
Attachment #8406086 -
Flags: review?(johu)
Comment 75•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Sireesha,
Thanks for this patch. It is almost finished. I clear the review flag because:
1. According to UX spec, we should have option buttons shown at header bar when device is at landscape mode. IIRC, we have it at last patch but it is missing is this patch. It should be easy to pull that back to this patch.
2. Thanks for your unit tests. But we break one video unit test. We need to fix them.
3. This patch breaks fullscreen mode of tablet version. The play button cannot be used and please hide the ff and rwd button in tablet.
4. The line-feed(\n) is leaded by a carriage-return(\r) at forwardrewind_controller.js. We may need to use dos2unix to fix that.
Attachment #8406086 -
Flags: review?(johu)
Flags: needinfo?(johu)
Comment 76•11 years ago
|
||
Comment on attachment 8368359 [details]
Video_Jan2014.pdf
Sorry, I found I read wrong spec. The review result item 1 is invalid. It exists in old spec but not in new spec. So, I make the old spec obsoleted.
Attachment #8368359 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8389598 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #8389591 -
Attachment is obsolete: true
Assignee | ||
Comment 77•11 years ago
|
||
Hi John,
All your review commented are squashed.
But for Tablet Full screen case,i cant check it in simulator because my fullscreen button is not working.
So i modified or removed the videoToolBar from hiding.
Can you please check now it is working fine in Tablet.
Thanks..
Sireesha
Assignee | ||
Updated•11 years ago
|
Attachment #8406086 -
Flags: review?(johu)
Comment 78•11 years ago
|
||
Bug 778077 has been fixed, so using fastSeek API instead of using currentTime will have a better performance.
Comment 79•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Sireesha,
I feel the patch is ready to master. But the tablet is still broken. I cannot touch any button anymore and no way to enter fullscreen mode. We need to fix it before landing to master. And we also need ui-review, of course.
Attachment #8406086 -
Flags: review?(johu) → review-
Assignee | ||
Comment 80•11 years ago
|
||
Hi John,
I corrected Tablet UI errors and now iam able to use the buttons in normal and fullscreen mode.
Would you please recheck and let me know in Tablet UI is fine or not.
Thanks,
Sireesha
Assignee | ||
Updated•11 years ago
|
Attachment #8406086 -
Flags: review- → review?(johu)
Comment 81•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
There are few suggestions here:
1. Please remove fullscreen-tool-bar and its descendants, including code in html, css, and js.
2. Please use "dos2unix" tool to convert the "\r\n" to "\n" for new line.
Attachment #8406086 -
Flags: review?(johu)
Assignee | ||
Comment 82•11 years ago
|
||
Hi John,
I squashed the review changes.
Please check and let me know your review comments.
Thanks..
Sireesha
Assignee | ||
Updated•11 years ago
|
Attachment #8406086 -
Flags: ui-review?(johu)
Assignee | ||
Updated•11 years ago
|
Attachment #8406086 -
Flags: ui-review?(johu)
Attachment #8406086 -
Flags: ui-review-
Attachment #8406086 -
Flags: review?(johu)
Assignee | ||
Comment 83•11 years ago
|
||
Hi John,
I corrected missing delete and share buttons in select view screen.
So,please check and let me know your comments.
Thanks..
Sireesha
Comment 84•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Sireesha,
This is good patch. But the portrait mode of tablet version cannot be used because of play button cannot be tapped. The same css changes at the landscape part of "video_tablet.css" should also be played at portrait part. They are similar but not exactly the same.
Attachment #8406086 -
Flags: ui-review-
Attachment #8406086 -
Flags: review?(johu)
Assignee | ||
Comment 85•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Hi John,
I did 2 changes in video_tablet.css for Portrait mode same as landscape.
So please check and let me the review comments.
Thanks..
Sireesha
Attachment #8406086 -
Flags: review?(johu)
Comment 86•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Thanks for your patient for revising so many times. I think this version is better now.
Nice job!
Attachment #8406086 -
Flags: review?(johu) → review+
Assignee | ||
Comment 87•11 years ago
|
||
Thank You John for your quick reviews and suggestions.
Assignee | ||
Comment 88•11 years ago
|
||
Hi Wyane,
Would you please help me to do the UI-Review for this issue from UX.
Thanks..
Sireesha
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(wchang)
Assignee | ||
Updated•11 years ago
|
Attachment #8406086 -
Flags: ui-review?(pla)
Comment 89•11 years ago
|
||
Rob, are you still looking after video?
Flags: needinfo?(wchang) → needinfo?(rmacdonald)
Comment 90•11 years ago
|
||
(In reply to Wayne Chang [:wchang] from comment #89)
> Rob, are you still looking after video?
Tiffanie is now on video, needinfo'd. Please flag her for any UI reviews. Thanks!
Flags: needinfo?(rmacdonald) → needinfo?(tshakespeare)
Comment 91•11 years ago
|
||
I'm having issues reviewing the patch on my device. Please attach video/photos if you need me to ui-review.
Thanks!
Flags: needinfo?(tshakespeare)
Assignee | ||
Comment 92•11 years ago
|
||
Hi,
I attached the screenshots(Bug_951025_Screenshots.rar) for Ui-review.
Please let me know your review comments.
Thanks..
Sireesha
Attachment #8417199 -
Flags: ui-review?(tshakespeare)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [m+] → [m+][g+]
Comment 93•11 years ago
|
||
Comment on attachment 8417199 [details]
Bug_951025_Screenshots.rar
Adding Amy for visual design feedback.
Attachment #8417199 -
Flags: ui-review?
Comment 94•11 years ago
|
||
Comment on attachment 8417199 [details]
Bug_951025_Screenshots.rar
Adding Amy for visual design feedback.
Attachment #8417199 -
Flags: ui-review?(amlee)
Updated•11 years ago
|
Attachment #8417199 -
Flags: ui-review?(amlee) → ui-review?(pla)
Updated•11 years ago
|
Attachment #8417199 -
Flags: ui-review?(pla) → ui-review?(hnguyen)
Comment 95•11 years ago
|
||
Comment on attachment 8417199 [details]
Bug_951025_Screenshots.rar
In the action menu, there shouldn't be icons - this was a change we made to camera's preview and I want to be consistent. Amy/Hung can comment more if needed.
Also, I was finally able to flash the patch - is this the latest? I noticed a few issues with it.
- the progression of the circle on the playback control isn't as smooth as it is in music
- the rewind/fast forward buttons didn't seem to do anything (both for tap and press&hold)
- when tapping "more" options icon, followed by "delete":
* the main button should be red and say "Delete" (rather than blue ok)
* let's remove the title "Video" and replace message with "Delete video?" This will make it consistent with gallery and camera's preview delete confirmation.
Thanks!
Attachment #8417199 -
Flags: ui-review?(tshakespeare) → ui-review-
Comment 96•11 years ago
|
||
Threw together an explanation of what is wrong with the current implementation in terms of visuals.
The explanation includes:
1. Comparison of the current vs intended design.
2. Alignment guide.
3. Redline of the UI.
I've also updated the play icon so its center aligned. The assets are included in the zip file.
Please let me know if you have any questions.
Comment 97•11 years ago
|
||
Comment on attachment 8418086 [details]
Explanation_PlayIconUpdate.zip
Please have a look.
Thanks!
Attachment #8418086 -
Flags: review?(vsireesha246)
Assignee | ||
Updated•11 years ago
|
Attachment #8418086 -
Flags: review?(vsireesha246) → review-
Assignee | ||
Comment 98•11 years ago
|
||
Hi,
I have some aligment problem with play and pause buttons with your new UI resource and values.
Please check the screenshots Bug_951025_UIReview.rar
Thanks..
Sireesha
Flags: needinfo?(hnguyen)
Comment 99•11 years ago
|
||
You're so close! Added a quick guide of how much more you have to move the elements.
Please update and send over a screencap when ready.
Thanks Sireesha
Attachment #8418747 -
Flags: review?(vsireesha246)
Flags: needinfo?(hnguyen)
Comment 100•11 years ago
|
||
Also, can you please post an updated patch so I can check out the fixes for the interaction issues I mentioned in comment 95?
Thanks Sireesha! :)
Assignee | ||
Comment 101•11 years ago
|
||
Hi,
I updated the PR with some changes.
Please check and let me know your review comments.
Thanks..
Sireesha
(In reply to Tiffanie Shakespeare from comment #100)
> Also, can you please post an updated patch so I can check out the fixes for
> the interaction issues I mentioned in comment 95?
>
> Thanks Sireesha! :)
Flags: needinfo?(tshakespeare)
Assignee | ||
Comment 102•11 years ago
|
||
Hi,
Would you please check this screenshots(Bug_951025_May08.rar).
This is my current status.
As your previous review review says "Move 8px left".Do you mean i need to maintain 7.1rem to 7.1-0.8=6.3 rem?
Thanks..
Sireesha
Flags: needinfo?(hnguyen)
Comment 103•11 years ago
|
||
Hey Sireesha - I checked out your patch (branch - Bug_951025_Video_FF/Rwd) and my comments (below) seem to still apply. Also, can you double check the screenshots you posted today? The folder was empty for me.
Thanks!!
(In reply to Tiffanie Shakespeare from comment #95)
> Comment on attachment 8417199 [details]
> Bug_951025_Screenshots.rar
>
> In the action menu, there shouldn't be icons - this was a change we made to
> camera's preview and I want to be consistent. Amy/Hung can comment more if
> needed.
>
> Also, I was finally able to flash the patch - is this the latest? I noticed
> a few issues with it.
> - the progression of the circle on the playback control isn't as smooth as
> it is in music
> - the rewind/fast forward buttons didn't seem to do anything (both for tap
> and press&hold)
> - when tapping "more" options icon, followed by "delete":
> * the main button should be red and say "Delete" (rather than blue ok)
> * let's remove the title "Video" and replace message with "Delete video?"
> This will make it consistent with gallery and camera's preview delete
> confirmation.
>
> Thanks!
Flags: needinfo?(tshakespeare)
Assignee | ||
Comment 104•11 years ago
|
||
Hi Tiffanie,
Please find the below comments.
1.In the action menu, there shouldn't be icons - this was a change we made to
> camera's preview and I want to be consistent. Amy/Hung can comment more if
> needed.
Ans:I modified same in PR today.
2.the progression of the circle on the playback control isn't as smooth as
it is in music
Ans:This is existing bug in video app.We can raise follow up bug for this.In this patch i didn't modify anything related to progression of the circle on the playback control.If it really my mistake i will correct it.
3.the rewind/fast forward buttons didn't seem to do anything (both for tap
and press&hold)
Ans:Today i rebased with latest code,Can you please check once again.Its working for me.
4.when tapping "more" options icon, followed by "delete":
* the main button should be red and say "Delete" (rather than blue ok)
* let's remove the title "Video" and replace message with "Delete video?"
This will make it consistent with gallery and camera's preview delete
confirmation.
Ans:Same here as this is existing bug in Video app,we can raise the followup bug for this.
So,please let me know your comments for this.
Flags: needinfo?(wchang)
Flags: needinfo?(tshakespeare)
Assignee | ||
Comment 105•11 years ago
|
||
Hi,
I attached the current status of of this PR screenshots.
Still i need to make some alignment proper.
Thanks..
Sireesha
Attachment #8419409 -
Attachment is obsolete: true
Updated•11 years ago
|
Flags: needinfo?(wchang)
Assignee | ||
Comment 106•11 years ago
|
||
Hi Tiffanie & Hnguyen,
With the current UI spec i am facing the FF/Rwd and Play buttons Touch area is very less.
So i modified the UI similar to Music app(Increasing buttons size) and the Touch area on FF/Rwd and play buttons got increased.(Refer Video1.png & Video2.png attached)
We need to increase the button size for increasing the Touch or hit area on buttons.
So would you please let me know your opinion about this.
If you are fine with this,please let me know the modified alignment values for the both landscape and portrait modes.
Thanks..
Sireesha
Assignee | ||
Comment 107•11 years ago
|
||
Comment 108•11 years ago
|
||
Hi Sireesha!
Checked out the patch and overall seems good. I've made a note to file those other issues as bugs, thanks for clarifying.
I was able to get the fast forward and rewind to work, though it seemed a little buggy. It wasn't always registering my tap or doing the action; I would say about 40% of the time.
For the control placement, I don't have a problem copying the layout of the music app - more space between controls definitely improves usability. I'll let Hung weigh in and work with you on alignment if he agrees.
Thanks! :)
Flags: needinfo?(tshakespeare)
Comment 109•11 years ago
|
||
One more question to...someone.
I noticed that the photo app also has videos within it (taken from the camera app). Do we have plans to copy this UI into that app? It would be weird if the photo and video app behaved differently.
Enjoy!
Updated•11 years ago
|
Attachment #8417199 -
Flags: ui-review?(hnguyen)
Flags: needinfo?(hnguyen)
Assignee | ||
Comment 110•11 years ago
|
||
Comment on attachment 8418747 [details]
951025_pixelmovement.png
As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=951025#c108 we may need to follow the Music app UI.
So clearing the review.
Hung would you please provide me the Visual similar to Music app in both portrait and landscape mode(Music app contains only Portrait mode).
Thanks..
Sireesha
Attachment #8418747 -
Flags: review?(vsireesha246) → review-
Comment 111•11 years ago
|
||
Hey Hung! See my comment #108 and Sireesha's comment #110. Do you agree that we should make the alignment as the Music app? If so can you provide that info please? Thanks so much!
Flags: needinfo?(hnguyen)
Comment 112•11 years ago
|
||
Hey Guys
I agree we should replicate the music controls for consistency. By the looks of it, this approach should be more scalable across a variety of screen widths.
Sireesha - can you implement it on the 320x480 resolution and forward me a screenshot? Since its a copy and paste exercise, I believe you won't need any further specification but let me know otherwise.
Thanks
Flags: needinfo?(hnguyen) → needinfo?(vsireesha246)
Assignee | ||
Comment 113•11 years ago
|
||
Hi Hung,
Please find the attached Bug_951025_VideoUI_As_Music.rar.
I modified the play,FF/rwd controls as per Music app and I didnt modified Timeslider progress bar.
Please let me know if any further modifications and landscape mode changes if required because Music app doesn't have landscape mode.
Flags: needinfo?(vsireesha246) → needinfo?(hnguyen)
Comment 114•11 years ago
|
||
Thanks for making the update Sireesha.
We're almost there. Please take a look at my attachments for the finishing touches.
Cheers
Flags: needinfo?(hnguyen) → needinfo?(vsireesha246)
Assignee | ||
Comment 115•11 years ago
|
||
Hi Hung,
Please find the updated attached Screenshots(Bug_951025.rar).
I also updated PR with the latest changes.
Please review and let me know your comments.
Thanks..
Sireesha
Attachment #8424610 -
Flags: ui-review?
Flags: needinfo?(vsireesha246)
Assignee | ||
Comment 116•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Hi John,
I modified some code (index.html,video.css and forwardrewind_controller.js) as per the UI-Review comments.
Would you please review it and let me know your review comments.
Thanks..
Sireesha
Attachment #8406086 -
Flags: review+ → review?(johu)
Comment 117•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Hi Russ,
Since you had became a peer of video app, you may also want to know this bug. It's an important one, I think. I will do the review. Please review this patch, too.
Attachment #8406086 -
Flags: review?(rnicoletti)
Assignee | ||
Updated•11 years ago
|
Attachment #8422998 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8420087 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8419924 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8418729 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8389080 -
Attachment is obsolete: true
Attachment #8389080 -
Flags: ui-review?(pla)
Assignee | ||
Updated•11 years ago
|
Attachment #8417199 -
Attachment is obsolete: true
Attachment #8417199 -
Flags: ui-review?
Assignee | ||
Updated•11 years ago
|
Attachment #8424610 -
Flags: ui-review? → ui-review?(hnguyen)
Comment 118•11 years ago
|
||
Patch looks good - does anyone else notice that the controls fade away super fast? I compared this with mozilla master and it's the same. Am I crazy or did they previously stay until you tapped?
Comment 119•11 years ago
|
||
I have updated the PR with some comments, however I haven't finished my review so I'm leaving r? for now.
One question I have is the behavior of the forward/rewind functionality when the player is paused. From testing and from looking at the code, I see the feature is coded to function only while the player is playing. My reading of the UX spec, and what I think it most intuitive, is that the forward/rewind functionality would be active while playing or paused; and either way, after forwarding or rewinding the player would retain its state -- if the player was playing before forwarding/rewinding, it would play after forwarding/rewinding; if the player was paused before forwarding/rewinding, it would be paused after forwarding/rewinding.
NI for Jacqueline to comment on whether forward/rewind should work when the player is paused.
Flags: needinfo?(jsavory)
Comment 120•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
The patch looks good. But we found some nits here. I clear the review flag because it breaks the tablet's UI again.
Attachment #8406086 -
Flags: review?(johu)
Assignee | ||
Comment 121•11 years ago
|
||
Hi Russ,
I updated the PR with your review comments.
I enabled FF/Rwd functionality even in pause state(Music app is doing the same).
So,please let me know your review comments if any.
Thanks..
Sireesha
(In reply to Russ Nicoletti [:russn] from comment #119)
> I have updated the PR with some comments, however I haven't finished my
> review so I'm leaving r? for now.
>
> One question I have is the behavior of the forward/rewind functionality when
> the player is paused. From testing and from looking at the code, I see the
> feature is coded to function only while the player is playing. My reading of
> the UX spec, and what I think it most intuitive, is that the forward/rewind
> functionality would be active while playing or paused; and either way, after
> forwarding or rewinding the player would retain its state -- if the player
> was playing before forwarding/rewinding, it would play after
> forwarding/rewinding; if the player was paused before forwarding/rewinding,
> it would be paused after forwarding/rewinding.
>
> NI for Jacqueline to comment on whether forward/rewind should work when the
> player is paused.
Comment 122•11 years ago
|
||
Comment on attachment 8424610 [details]
Bug_951025.rar
From a visual perspective, we're good to go.
Thanks for all your hard work Sireesha.
Attachment #8424610 -
Flags: ui-review?(hnguyen)
Assignee | ||
Comment 123•11 years ago
|
||
Hi John,
I updated the Tablet UI crash fix and some of your review comments.
I have some queries related to unit test and same i asked in PR.
So,please let me know your comments for this.
Thanks..
Sireesha
Flags: needinfo?(johu)
Comment 124•11 years ago
|
||
Hi Russ, thanks for catching this.
Yes, I agree that the fast forward and rewind buttons should still work during the paused state.
Flags: needinfo?(jsavory)
Comment 125•11 years ago
|
||
Hey guys - I just installed the updated code and it looks like the forward/rewind buttons have stopped working. I can't get them to work at all during playing and paused.
Comment 126•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Sireesha, I added a few more comments/questions to the PR. Although r- for now, I anticipate approval after the next iteration. Good work.
One other comment, I see there are few new images that aren't used ('actionicon_media_*.png') -- it seems these are obsolete in the presence of IconAction_Media_*.png images. If so, can you delete the obsolete image files? Thanks.
Attachment #8406086 -
Flags: review?(rnicoletti) → review-
Assignee | ||
Comment 127•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Hi Russ,
I updated all your review changes and for one review i added my comment in PR.
So please review and let me know your comments.
Thanks..
Sireesha
Attachment #8406086 -
Flags: review- → review?(rnicoletti)
Comment 128•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Patch looks good from the interaction side. It also looks like Hung approved the visuals in comment #122.
Thanks Sireesha
Attachment #8406086 -
Flags: ui-review+
Comment 129•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Looks good, r+. There are a few nits I added to the PR about missing spaces and the name of the new unit test js file. If you can fix those that would be good.
One last thing, I noticed the travis build failed due to a linter error in forward_rewind_controller.js (you need to declare the export of 'ForwardRewindController') -- I also mentioned another linter issue in that file in the PR.
This is a nice addition to the video app.
Attachment #8406086 -
Flags: review?(rnicoletti) → review+
Assignee | ||
Comment 130•11 years ago
|
||
Hi Russ,
I fixed the nits related to space and renamed the file and added export for linter error.
For remaining this i added comments in PR.
Thanks for your quick review.
(In reply to Russ Nicoletti [:russn] from comment #129)
> Comment on attachment 8406086 [details]
> Pointer to Pull Request.html
>
> Looks good, r+. There are a few nits I added to the PR about missing spaces
> and the name of the new unit test js file. If you can fix those that would
> be good.
>
> One last thing, I noticed the travis build failed due to a linter error in
> forward_rewind_controller.js (you need to declare the export of
> 'ForwardRewindController') -- I also mentioned another linter issue in that
> file in the PR.
>
> This is a nice addition to the video app.
Comment 131•11 years ago
|
||
Sireesha,
It works fine but has a wired animation. That doesn't affect the usage of tablet. Thanks for taking care of this.
Flags: needinfo?(johu)
Assignee | ||
Comment 132•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Hi Tiffanie,
Would you please review the UI for this PR.(Normal Video playback,Pick activity(from SMS) and through Bluetooth transfer etc)
Thanks..
Sireesha
Flags: needinfo?(tshakespeare)
Comment 133•11 years ago
|
||
Hey Sireesha - patch looks good. Nice work!
I did run across a bug where the image would freeze during playback but you could hear the audio continuing in the background.
The STR are pretty much tapping the play, forward/back buttons and throwing a pause tap and doing the same. I didn't notice a specific sequence, but I am able to repro pretty easily. When this happens I have to force quit the video app to be able to use it again.
Flags: needinfo?(tshakespeare)
Comment 134•11 years ago
|
||
Hey John - I don't have a tablet. What sort of weird animation is happening? Even though the UI is still usable, we should still ensure the animations are correct and working.
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #131)
> Sireesha,
>
> It works fine but has a wired animation. That doesn't affect the usage of
> tablet. Thanks for taking care of this.
Flags: needinfo?(johu)
Comment 135•11 years ago
|
||
(In reply to vsireesha246 from comment #130)
> Hi Russ,
>
> I fixed the nits related to space and renamed the file and added export for
> linter error.
> For remaining this i added comments in PR.
>
> Thanks for your quick review.
>
> (In reply to Russ Nicoletti [:russn] from comment #129)
> > Comment on attachment 8406086 [details]
> > Pointer to Pull Request.html
> >
> > Looks good, r+. There are a few nits I added to the PR about missing spaces
> > and the name of the new unit test js file. If you can fix those that would
> > be good.
> >
> > One last thing, I noticed the travis build failed due to a linter error in
> > forward_rewind_controller.js (you need to declare the export of
> > 'ForwardRewindController') -- I also mentioned another linter issue in that
> > file in the PR.
> >
> > This is a nice addition to the video app.
Hi Sireesha, thanks for your quick updates per the review comments.
I updated the PR regarding the ForwardRewindController 'init' function in the unit test context. I also realized the unit tests could fairly easily provide more test coverage which I also mentioned in the PR. Thanks again for the updates.
Comment 136•11 years ago
|
||
(In reply to Tiffanie Shakespeare from comment #134)
> Hey John - I don't have a tablet. What sort of weird animation is happening?
> Even though the UI is still usable, we should still ensure the animations
> are correct and working.
Hi Tiffanie,
Thanks for asking this. Please find the attachment which contains the animation. A flying animation happens to the "play" or "pause" button when I press it. So, please focus your eye at the bottom of the video. It may be caused by few misconfigured CSS properties.
Flags: needinfo?(johu)
Assignee | ||
Comment 137•11 years ago
|
||
Hi Tiffanie,
I am unable to reproduce this issue.
Would you please let me know the Gaia and Gecko version.
Thanks..
Sireesha
(In reply to Tiffanie Shakespeare from comment #133)
> Hey Sireesha - patch looks good. Nice work!
>
> I did run across a bug where the image would freeze during playback but you
> could hear the audio continuing in the background.
>
> The STR are pretty much tapping the play, forward/back buttons and throwing
> a pause tap and doing the same. I didn't notice a specific sequence, but I
> am able to repro pretty easily. When this happens I have to force quit the
> video app to be able to use it again.
Comment 138•11 years ago
|
||
Hey Sireesha - must have been something wrong with the Build I had installed yesterday and I have since updated it. Believe me I tried, but I cant repro the issue. Easy fix :)
Assignee | ||
Comment 139•11 years ago
|
||
Hi Russ,
I modified the Unit test cases.
Would you please review and let me know the comments.
Thanks..
Sireesha
Flags: needinfo?(rnicoletti)
Assignee | ||
Comment 140•11 years ago
|
||
Hi John,
This i can't able to reproduce in simulator.
Can you please conform..is it reproduced in simulator?
Thanks..
Sireesha
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #136)
> Created attachment 8427461 [details]
> tablet-wired-animation.mp4
>
> (In reply to Tiffanie Shakespeare from comment #134)
> > Hey John - I don't have a tablet. What sort of weird animation is happening?
> > Even though the UI is still usable, we should still ensure the animations
> > are correct and working.
>
> Hi Tiffanie,
>
> Thanks for asking this. Please find the attachment which contains the
> animation. A flying animation happens to the "play" or "pause" button when I
> press it. So, please focus your eye at the bottom of the video. It may be
> caused by few misconfigured CSS properties.
Comment 141•11 years ago
|
||
Yes! I do see that.
Sireesha - can you double check the CSS and see if we can resolve this issue? I assume that why you're asking John about trying to repro the issue in the simulator.
Thanks!
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #136)
> Created attachment 8427461 [details]
> tablet-wired-animation.mp4
>
> (In reply to Tiffanie Shakespeare from comment #134)
> > Hey John - I don't have a tablet. What sort of weird animation is happening?
> > Even though the UI is still usable, we should still ensure the animations
> > are correct and working.
>
> Hi Tiffanie,
>
> Thanks for asking this. Please find the attachment which contains the
> animation. A flying animation happens to the "play" or "pause" button when I
> press it. So, please focus your eye at the bottom of the video. It may be
> caused by few misconfigured CSS properties.
Flags: needinfo?(vsireesha246)
Comment 142•11 years ago
|
||
Sorry for late replying.
The simulator doesn't have the same issue with the latest version. But it still has the same issue in tablet.
Assignee | ||
Comment 143•11 years ago
|
||
Hi Tiffanie,
I don't have Tablet with me and i need to fix this issue with simulator only.
But i checked this issue, is not reproduced in simulator.Please check the John comment 142 for the same.
Thanks..
Sireehsa
(In reply to Tiffanie Shakespeare from comment #141)
> Yes! I do see that.
>
> Sireesha - can you double check the CSS and see if we can resolve this
> issue? I assume that why you're asking John about trying to repro the issue
> in the simulator.
>
> Thanks!
>
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #136)
> > Created attachment 8427461 [details]
> > tablet-wired-animation.mp4
> >
> > (In reply to Tiffanie Shakespeare from comment #134)
> > > Hey John - I don't have a tablet. What sort of weird animation is happening?
> > > Even though the UI is still usable, we should still ensure the animations
> > > are correct and working.
> >
> > Hi Tiffanie,
> >
> > Thanks for asking this. Please find the attachment which contains the
> > animation. A flying animation happens to the "play" or "pause" button when I
> > press it. So, please focus your eye at the bottom of the video. It may be
> > caused by few misconfigured CSS properties.
Flags: needinfo?(vsireesha246)
Assignee | ||
Comment 144•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
Hi John,
I fixed the "Blue color Highlighting issue on the Play,FF/Rwd buttons" if the user swipes or clicks the button edges.
So would you please review the changes and also let me know the Tablet issue is still exists or not.
Thanks..
Sireeha
Attachment #8406086 -
Flags: review?(johu)
Comment 145•11 years ago
|
||
Is there perhaps someone with a tablet we can ping to ask for help? I totally get how impossible it would be to try to fix something without being able to reproduce.
Comment 146•11 years ago
|
||
Hi Sireeha, I added some comments to the PR regarding the unit tests (one minor issue).
Flags: needinfo?(rnicoletti)
Comment 147•11 years ago
|
||
Comment on attachment 8406086 [details]
Pointer to Pull Request.html
I had tested on phone and tablet. I think the everything is fine. Don't forget the check russ's comments and my comment about tablet animation issue.
Thanks for this patch. Nice job!!
Attachment #8406086 -
Flags: review?(johu) → review+
Assignee | ||
Comment 148•11 years ago
|
||
Hi John,
I updated the PR.Can you please check the Tablet issue is solved and i also updated Russ comments.
Thanks..
Sireesha
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] OOO from 6/2~6/6 from comment #147)
> Comment on attachment 8406086 [details]
> Pointer to Pull Request.html
>
> I had tested on phone and tablet. I think the everything is fine. Don't
> forget the check russ's comments and my comment about tablet animation issue.
>
> Thanks for this patch. Nice job!!
Flags: needinfo?(johu)
Comment 149•11 years ago
|
||
Cool. This version fixes the issue. That's really caused by transition css.
Flags: needinfo?(johu)
Assignee | ||
Updated•11 years ago
|
Attachment #8406086 -
Flags: ui-review?(pla)
Assignee | ||
Comment 150•11 years ago
|
||
Hi Russ,
I updated your review comments.Please check.
Thanks..
Sireesha
(In reply to Russ Nicoletti [:russn] from comment #146)
> Hi Sireeha, I added some comments to the PR regarding the unit tests (one
> minor issue).
Comment 151•11 years ago
|
||
Comment on attachment 8427461 [details]
tablet-wired-animation.mp4
Obsolete this attachment since it is not an issue anymore.
Attachment #8427461 -
Attachment is obsolete: true
Assignee | ||
Comment 152•11 years ago
|
||
Hi Russ,
We can land this PR to master,if your review is done?
From my side if anything is pending please let me know.
Thanks..
Sireesha
Flags: needinfo?(rnicoletti)
Comment 153•11 years ago
|
||
It looks good to land. The travis failures appear to be unrelated to your changes. Although I'm finding the patch doesn't apply cleanly in master. You may need to correct that before landing.
Flags: needinfo?(rnicoletti)
Assignee | ||
Comment 154•11 years ago
|
||
Hi Russ,
I am able to apply the patch cleanly on to the Master.
Would you please check it once and please land it on to the master,because i don't have permissions to merge.
Thanks..
Sireesha
(In reply to Russ Nicoletti [:russn] from comment #153)
> It looks good to land. The travis failures appear to be unrelated to your
> changes. Although I'm finding the patch doesn't apply cleanly in master. You
> may need to correct that before landing.
Flags: needinfo?(rnicoletti)
Comment 155•11 years ago
|
||
I verified the patch applies cleanly against latest master. Landed just now.
Flags: needinfo?(rnicoletti)
Updated•11 years ago
|
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 156•11 years ago
|
||
Updated•10 years ago
|
feature-b2g: --- → 2.0
Comment 157•10 years ago
|
||
Added 2.0 feature flag since it made it into 2.0 before branch cut
You need to log in
before you can comment on or make changes to this bug.
Description
•