search

signalapp / Signal-iOS

A private messenger for iOS.
https://signal.org
GNU Affero General Public License v3.0
10.77k stars 3.02k forks source link

PNGs are not editable - regression from changing PNG mimetype & maybe-animated status? #5761

Closed frederickding closed 4 months ago

frederickding commented 7 months ago

Bug description

PNG screenshots are not offered the options to markup or crop when attaching to a Signal message in the iOS app (version 6.60.0.10), but this has been inconsistent. Possibly related to recent commit 79dacfb040bc5a1e93a31806c239e427f565606c about treating PNGs as animated, which affects the gating check for the image editor, but I can't tell if the commit introduced the bug or was an intended fix.

Steps to reproduce

Actual result: no cropping or markup tools offered, only send quality

Expected result: cropping should be offered just like for photographs

Device info

Device: iPhone 15 Pro iOS version: 17.3 Signal version: 6.60.0.10

Link to debug log

N/A

frederickding commented 7 months ago

Found the bug.

The refactor in a97772265d4ea83e92de0f5067c14ff1142caaaf (changing SignalServiceKit/src/Util/MIMETypeUtil.m from previously a single "animated image" category to now 2 categories: "maybe animated" and "definitely animated") incorrectly defined the supportedAnimatedImageUTITypes from the maybe animated list:

https://github.com/signalapp/Signal-iOS/blob/a97772265d4ea83e92de0f5067c14ff1142caaaf/SignalServiceKit/src/Util/MIMETypeUtil.m#L576-L583

because now PNGs will ALWAYS be treated as "isAnimated" = true in SignalAttachment.swift: https://github.com/signalapp/Signal-iOS/blob/e6ba17aa451b6fa66ccc492b078a0d5d41e6e8d6/SignalMessaging/attachments/SignalAttachment.swift#L496-L500 https://github.com/signalapp/Signal-iOS/blob/e6ba17aa451b6fa66ccc492b078a0d5d41e6e8d6/SignalMessaging/attachments/SignalAttachment.swift#L524-L530

SignalAttachment.swift was intended to handle PNGs as a special case. Lines 525-528 are supposed to check the internal metadata to decide if the PNG is truly animated or not. But notice that if the PNG is not animated, there is no return false -- yet, PNGs are included in the animatedImageUTISet.

Where this matters

This propagates to the attachment UI because:

  1. AttachmentApprovalToolbar.swift only shows the crop/pen buttons if the currentAttachmentImage.type is "image".
  2. Condition 1 is only possible if the imageEditorModel was initialized: https://github.com/signalapp/Signal-iOS/blob/e6ba17aa451b6fa66ccc492b078a0d5d41e6e8d6/SignalUI/ViewControllers/AttachmentApproval/AttachmentItemCollection.swift#L31-L32
  3. However, the guard statement in AttachmentItemCollection.swift prevents initializing the imageEditorModel if isAnimatedImage is true for that attachment. https://github.com/signalapp/Signal-iOS/blob/e6ba17aa451b6fa66ccc492b078a0d5d41e6e8d6/SignalUI/ViewControllers/AttachmentApproval/AttachmentItemCollection.swift#L59-L62
  4. The isAnimatedImage check is defined in lines 524-531 of SignalAttachment.swift, discussed above.
stale[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 months ago

This issue has been closed due to inactivity.