[Gallery/Video]Background Overlays are not translated

RESOLVED FIXED

Status

Firefox OS
Gaia::Gallery
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vsireesha246, Assigned: russn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [LibGLA,TD83230,WW, A])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
STR:

Precondition:
Settings->USB storage->Enable

1.Open Gallery/Video app->It shows "Memory card in use" overlay
2.Press Home button to make it in background
3.Go to Settings->Languages->Change to any other language->click on OK button->Press/hold Home button
4.Open the Gallery/Video background overlay and click on it..the text is not translated.

Note:Same issue is not happening in Music app.
(Reporter)

Updated

4 years ago
Flags: needinfo?(pdahiya)
Flags: needinfo?(im)
(Reporter)

Updated

4 years ago
Flags: needinfo?(im)
(Reporter)

Updated

4 years ago
Flags: needinfo?(im)
Yes, you are right. We haven't take this as a consideration. I cc Russ, another video peer, in.
Flags: needinfo?(im)
This issue should be resolved with the fix of Bug 1038984 for gallery and video app. Please try the patch attached to 1038984. Marking the bug duplicate of bug 1038984.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(pdahiya)
Resolution: --- → DUPLICATE
Duplicate of bug: 1038984
(Reporter)

Updated

4 years ago
Whiteboard: [LibGLA,TD83230,WW, A]

Comment 3

4 years ago
Unfortunately, the video app does not use the same code as the gallery app for overlays so I am reopening this.

This could now be resolved by modifying the video app to use the shared/Dialogs.js `showOverlay` code. 
As outlined in the comments in https://github.com/mozilla-b2g/gaia/pull/21885.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
(Assignee)

Updated

4 years ago
Assignee: nobody → rnicoletti
(Assignee)

Comment 4

4 years ago
Created attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

This PR includes the patch from bug 1038984 as those changes to shared/js/dialogs.js are required for this patch.
Attachment #8473900 - Flags: review?(pdahiya)
Attachment #8473900 - Flags: review?(im)
Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

Looks good. I left some comments at PR. BTW, I am sure if we need to change the l10n key in this case.
Attachment #8473900 - Flags: review?(im) → review+
(Reporter)

Comment 6

4 years ago
Hi Russ,

https://github.com/mozilla-b2g/gaia/pull/22938

I found in this PR below problems.

1.Empty videos overlay is not shown(Without video in sdcard and internal memory).For this we need to add emptyvideo case in dialog.js

2.In dialog.js 

var textOption = options[prefix] || options[prefix + 'Text'];
should be replaced by
var textOption = options[prefix] || options[prefix + 'Id'];
Flags: needinfo?(rnicoletti)
(Reporter)

Comment 7

4 years ago
Hi Russ,

One more point i missed..we need to lazy load the confirm.css in index.html of video app.

Please let me know your comments,if am wrong.

Thanks..
Sireesha



(In reply to vsireesha246 from comment #6)
> Hi Russ,
> 
> https://github.com/mozilla-b2g/gaia/pull/22938
> 
> I found in this PR below problems.
> 
> 1.Empty videos overlay is not shown(Without video in sdcard and internal
> memory).For this we need to add emptyvideo case in dialog.js
> 
> 2.In dialog.js 
> 
> var textOption = options[prefix] || options[prefix + 'Text'];
> should be replaced by
> var textOption = options[prefix] || options[prefix + 'Id'];
Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

showOverlay method inside shared/js/dialogs.js is having too many app-specific localization ids and should be brought back inside respective app. Created bug 1055715 to remove showOverlay from shared/js/dialogs.js. 

As discussed over IRC, cancelling the review as the attached patch should be updated to remove showOverlay dependency for video on shared/js/dialogs.js
Attachment #8473900 - Flags: review?(pdahiya)
(Assignee)

Comment 9

4 years ago
I will investigate the issues in comment 6 and comment 7 when I refactor the original showOverlay in video.js per comment 8
Flags: needinfo?(rnicoletti)
(Assignee)

Comment 10

4 years ago
I have updated the PR so that the video app no longer has a dependency on shared/js/dialogs. Also, the video.js showOverlay now sets the 'data-l10n-id' attribute instead of 'textContent' to support dynamic language change.

The updated PR addresses the issues in comment 7.
(Assignee)

Comment 11

4 years ago
Correction: comment 10, "the video app no longer has a dependency on shared/js/dialogs" should be "the video app no longer has a dependency on shared/js/dialogs showOverlay"
(Assignee)

Comment 12

4 years ago
Giovanni, can you comment on John's questions regarding Dialogs.confirm here https://github.com/mozilla-b2g/gaia/pull/22938/files#r16393915 and here https://github.com/mozilla-b2g/gaia/pull/22938/files#r16394018? 

Btw, I copied shared/js/dialogs.js from your bug 1038984 patch as I needed those changes for this bug.
Flags: needinfo?(gcharles)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

John, I'm requesting another review because the fix has changed from using shared/js/dialogs showOverlay to showOverlay in the video app.
Attachment #8473900 - Flags: review+ → review?(im)
Punam is correct. The showOverlay depends on gallery's id heavier. I should find it at the previous patch review. That's my fault.

As to this version, it looks good for me, except the one that you are asking to Giovanni. We should wait for his answer before landing this. So, I keep the review flag.
(Assignee)

Comment 17

4 years ago
Giovanni, apologies for the broken links into the PR. If you search for 'giovannic' in the "files changed" tab of the PR you will find John's questions about the 'confirm' method in dialogs.js.

Comment 18

4 years ago
My comment should be the most recent. I will quote it here:

"yes this function assumes the cases are mutually exclusive. The else-if version would be safer and clearer although they should not alter the behaviour of the function given the assumptions.

Thanks for the tip :)"

I have added the suggested changes to my patch.

Comment 19

4 years ago
Something that is not mentioned in the comments is that the 2nd change proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=1047215#c6 is incorrect. 

The case shown here, https://github.com/mozilla-b2g/gaia/pull/22938/files#diff-c3e14d59821cfbb27fe47029d88349f6R31, was put in to preserve the existing behaviour of Dialogs.confirm as requested in https://bugzilla.mozilla.org/show_bug.cgi?id=1038984#c12.

Changing that line would make nonsensical behaviour.
(Assignee)

Comment 20

4 years ago
I have updated the PR for this bug with Giovanni's latest shared/js/dialogs.js patch. I also tested the gallery and video apps with this latest patch and they work as expected.

John, can you update the review flag? Thanks.
Flags: needinfo?(im)
Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

Looks good to me. Please remove the console.log before landing the code. BTW, I found this patch is conflicted with others in index.html. You should rebase the code and check if you break any gaia-try. Make sure they are all passed before landing. Thanks.
Attachment #8473900 - Flags: review?(im) → review+
Flags: needinfo?(im)
(Assignee)

Comment 22

4 years ago
I have updated the patch because the semantics of showOverlay changed so the unit tests needed to be updated. The unit tests passed on the try server. However, when I run them locally, I'm getting global leak errors from mocha:

  1) [video-test/unit/video_test.js] Video App Unit Tests "before all" hook:
     Error: global leaks detected: LAYOUT_MODE, dom, ids, $, playing, playerShowing, endedTimer, controlShowing, controlFadeTimeout, selectedFileNames, selectedFileNamesToBlobs, currentVideo, currentVideoBlob, firstScanEnded, THUMBNAIL_WIDTH, THUMBNAIL_HEIGHT, HAVE_NOTHING, storageState, currentOverlay, dragging, touchStartID, isPausedWhileDragging, sliderRect, thumbnailList, pendingPick, restoreTime, isPhone, isPortrait, currentLayoutMode, pendingUpdateTitleText, FROMCAMERA, videoControlsAutoHidingMsOverride, init, initThumbnailSize, initLayout, initPlayerControls, initOptionsButtons, addEventListeners, toggleFullscreenPlayer, toggleVideoControls, handleScreenLayoutChange, switchLayout, handleActivityEvents, showInfoView, hideInfoView, showSelectView, hideSelectView, showOptionsView, hideOptionsView, clearSelection, updateSelection, launchCameraApp, resetCurrentVideo, deleteSelectedItems, deleteFile, shareSelectedItems, share, updateDialog, updateLoadingSpinner, thumbnailClickHandler, setPosterImage, showOverlay, setControlsVisibility, updateVideoControlSlider, setVideoPlaying, deleteCurrentVideo, handlePlayButtonClick, handleCloseButtonClick, postPickResult, shareCurrentVideo, handleSliderTouchStart, setVideoUrl, scheduleVideoControlsAutoHiding, setNFCSharing, showPlayer, hidePlayer, playerEnded, play, pause, timeUpdated, handleSliderTouchEnd, handleSliderTouchMove, toCamelCase, releaseVideo, restoreVideo, showPickView, cancelPick, cleanupPick, showThrobber, hideThrobber

I don't have an explanation for why I'm seeing these leaks after the recent changes and why they're passing on the try server. John, can you take a look? Thanks.
Flags: needinfo?(im)
Russ,

I cannot tell you why because I cannot reproduce it in my site. I had done `make really-clean` before apply your patch which could make sure everything is clean. I would suggest that you may `make really-clean` at master branch, apply your patch, and try again.

The main reason of global leaks is we set a variable to window which is not declared globally in this test file. The only one may be "LazyLoader". But I didn't get the error about it. The solution to fix this kind of error is declared it is globally, like:

mocha.globals(['LazyLoader', 'others', ...]);

It should be put before the first require().

The only one you need is LazyLoader, I think. But I cannot validate it because I cannot see the error or warning of it.

BTW, I found we haven't saw any global leaks recently. It may be disabled in test-agent with "--ignore-leaks".
Flags: needinfo?(im)
(Assignee)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.