Closed Bug 873301 Opened 9 years ago Closed 9 years ago

Gallery should store image previews and video posters on same volume as image/video.

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: dhylands, Assigned: squib)

Details

(Whiteboard: [mozilla-triage])

Attachments

(1 file, 1 obsolete file)

Now that we can support multiple volumes, I think that it makes sense for picture previews (aka thumbnails) and video posters (aka previews) to be stored on the same volume as the original data.
Assignee: nobody → dhylands
Noming since this should go into the leo devices, which use multiple storage areas.
blocking-b2g: --- → leo?
Whiteboard: [mozilla-triage]
Attachment #756340 - Flags: review?(dflanagan)
Comment on attachment 756340 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10124

A few nits on github, but r+ once you've considered those and fixed the lint errors.

I notice that the bug title also talks about video poster images.  Those are actually created by the camera.  (In camera/js/filmstrip.js, iirc.)

If you want to fix that in the camera app as part of this bug I'm happy to re-review.
Attachment #756340 - Flags: review?(dflanagan) → review+
in traiges, it's a blocker for leo.
blocking-b2g: leo? → leo+
Stealing per IRC.
Assignee: dhylands → squibblyflabbetydoo
Status: NEW → ASSIGNED
I just remembered that both the Gallery and Video apps can delete videos and both have code to delete the video preview image in addition to the video itself.  That deletion code may need to be updated to use the right path. (Or maybe it already works--I'm not sure.)
(In reply to David Flanagan [:djf] from comment #6)
> I just remembered that both the Gallery and Video apps can delete videos and
> both have code to delete the video preview image in addition to the video
> itself.  That deletion code may need to be updated to use the right path.
> (Or maybe it already works--I'm not sure.)

Looking at the code, I'm pretty sure this is already handled. We save the filename we built, and then refer to it again when deleting. Aside from addressing your review comments, is there anything else I should be handling here?
Ok, here's an updated PR with the review comments addressed (99% of the credit for this goes to Dave Hylands). As mentioned earlier, I don't think we need to do anything special for deleting previews. Our code should already handle that nicely.
Attachment #756340 - Attachment is obsolete: true
Attachment #759977 - Flags: review?(dflanagan)
Jim, did you look at camera/js/filmstrip.js ?
Flags: needinfo?(squibblyflabbetydoo)
Comment on attachment 759977 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/10271

r=djf for github commit ebff6ad

I did not test, so please make sure you do before landing.  From my inspection of the camera code, it looks like we're already okay there, but be sure you test that, on a Leo device, too with both internal and external storage as the default.
Attachment #759977 - Flags: review?(dflanagan) → review+
(In reply to David Flanagan [:djf] from comment #10)
> I did not test, so please make sure you do before landing.  From my
> inspection of the camera code, it looks like we're already okay there, but
> be sure you test that, on a Leo device, too with both internal and external
> storage as the default.

Well, I'll try to flash my Leo device, but I still don't think I've fully resolved the issues I was having last week. If I can't get it to work, I might need to bug you to run the tests on the device...
Flags: needinfo?(squibblyflabbetydoo)
(In reply to Jim Porter (:squib) from comment #11)
> Well, I'll try to flash my Leo device, but I still don't think I've fully
> resolved the issues I was having last week. If I can't get it to work, I
> might need to bug you to run the tests on the device...

Yeah, it still doesn't work right. I had tried running the flashing tools in Wine, but it doesn't work. I'm not even sure that would fix the issue I'm having anyway...
The patch works for me on my Leo device.

Landed to master: https://github.com/mozilla-b2g/gaia/commit/fc0ed1fb2a72327378946c2d6601c63eae31b718
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: in-moztrap-
Uplifted fc0ed1fb2a72327378946c2d6601c63eae31b718 to:
v1-train: a9b004a693ec59b5cfd8750b6200eecb861a6583
1.1hd: a9b004a693ec59b5cfd8750b6200eecb861a6583
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.