1088526 - [Messages] Rework how the images are resized in compose.js/attachment.js/attachment_renderer.js

Status: RESOLVED WONTFIX

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

Description

[Julien Wajsberg [:julienw]]

Follow-up of buf 1071514

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: github PR (WIP)

Comment 2

[Julien Wajsberg [:julienw]]

(In reply to Jenny Lee from Bug 1071514 comment #17)
> Hi Julien,
> 
> Steve showed me both patch 1 and 2. I think patch 1 is a better fix for the
> flickering situation and I'd suggest we apply this fix to 2.0 ;) The reason
> is that the placeholder appears before actual picture also gives a
> flickering visual experience, as it is only present for under 0~1ish sec
> (from what I see).
> 
> We could potentially work on a solution based on your placeholder idea, for
> example replacing it with a spinner. Ideally what would happen is:
> 
> 1. Attach a photo
> 2. Show spinner (during the time spinner is shown, do resizing)
> 3. Show thumbnail that's resized along with the size info
> 
> So we give user instant feedback on attach action by immediately show a
> spinner, the spinner sends a clear message that the device is undergoing a
> process, and then, when resize is done, we show the thumbnail directly as
> well as the updated size info. Let me know what you think!

I don't really like the spinner in this case. There is one technical reason: it takes quite a lot of CPU, and we need this CPU while resizing.

I tried adding the placeholder because I wanted something to show up in the same place as soon as the user attached the image, to give a sense of speed. But it could be something lighter (because I agree that the placeholder is quite visible): a simple frame with a light background color and possibly a light border.

What do you think?

Comment 3

[Jenny Lee]

The spinner doesn't have to be an actual spinner, it can be a still image (or 2 consecutive spinner image at different progress if it takes more than a certain amount of time to resize). In a way it is a placeholder, I think spinner is more indicative than a placeholder image or plain background color ;)

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1088526-image-resizing-refactoring > mozilla-b2g:master

Comment 5

[Julien Wajsberg [:julienw]]

Rebased to master but I didn't try this rebased version at all.
Unit tests are not updated either.

I think there are more things we can do now that attachments are kept in a Map -- for example the full size (Compose.size) can be computed without adding the attachments to the DOM first, so maybe we can do better.

Also we may not want to keep the last commit, following Jenny's comment in bug 1071514 comment 17. We should investigate this spinner as well.

We could want to move forward the patch as it is though, as I think it's much cleaner than the current solution.

Comment 6

[Julien Wajsberg [:julienw]]

OK I just pushed a new commit that makes things work properly.

Comment 7

[Julien Wajsberg [:julienw]]

(last commit is still the CSS change, I think it's better to move them away, possibly just remove them as Jenny suggested)

Comment 8

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 9

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(