ChatSecure / ChatSecure-iOS

ChatSecure is a free and open source encrypted chat client for iOS that supports OTR and OMEMO encryption over XMPP.
https://chatsecure.org
Other
3.13k stars 1.03k forks source link

Add a possibility to delete downloaded media to free up storage #1189

Closed stigger closed 4 years ago

chrisballinger commented 4 years ago

Thanks! This is awesome

stigger commented 4 years ago

Would be really great to see this in the app store build sometime. Slowly running out of disk space on my device :). If there are any problems with the feature, just let me know.

chrisballinger commented 4 years ago

@stigger Definitely! I am planning on including it in the next release :)

stigger commented 4 years ago

I tested this scenario extensively, but only in a simulator. Does it crash for you in the simulator or on a device? Can you show me the crashlog, so I know, what exactly to look for?

chrisballinger commented 4 years ago

On the simulator, iPhone 11 Pro Max 13.4 using Xcode 11.4. The crash is coming from inside JSQMessagesVC during collectionView:cellForItemAtIndexPath:. It's being given a 0x0 frame size and crashing on invalid layout, likely because the underlying media data is nil. I don't have the exact stack trace in front of me right now unfortunately.

To clarify the above repro steps, I didn't delete the media messages from the conversation itself before going to storage management. So after clearing via storage management, there are some messages that are missing their underlying media data.

stigger commented 4 years ago

Yeah, that's how I understood. That's exactly the use-case I wanted to cover, and tested it many times: after deleting the media, placeholders should appear instead of messages, as if the automatic downloading is disabled. You should be able to download the media again, and repeat this cycle many times. Never seen any crashes like what you're describing.

I'll try to reproduce some more this weekend.

stigger commented 4 years ago

I tried to reproduce, but couldn't, I get the expected placeholders instead of the media, no crashes. Could it be that this was some unrelated crash?

chrisballinger commented 4 years ago

@stigger Can you try rebasing your changes against the latest master branch? Perhaps I was only seeing it because I had merged it with the latest changes on master? There is a change to the YapDatabase enumeration APIs, so you'll need to cherry-pick the changes from this commit: https://github.com/ChatSecure/ChatSecure-iOS/commit/772d3332aa26af52c6275da68aa8c955edca6b31

Neustradamus commented 4 years ago

@stigger: Any news on it?

chrisballinger commented 4 years ago

@stigger After testing again this evening I couldn't reproduce the crash. 🤷‍♂

However I don't see a proper placeholder for re-downloading the media again for either OMEMO or plaintext messages. I don't necessarily expect OMEMO media messages to be re-downloaded, but plaintext ones should in theory still work.

Screen Shot 2020-04-07 at 10 38 10 PM

This might be a bug beyond the scope of your PR, although I haven't seen this particular layout issue before. I'm testing with your changes merged into master here if that helps: https://github.com/ChatSecure/ChatSecure-iOS/tree/stigger-storage_management

stigger commented 4 years ago

Checkout out the branch, seems to be working correctly. Perhaps, some other messages in the conversation are affecting the layout? Screen Recording 2020-04-09 at 23 19 08 2020-04-09 23_27_33

Neustradamus commented 4 years ago

Nice!