Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Apr 14, 2020

Conversation

stigger
Copy link
Contributor

@stigger stigger commented Feb 26, 2020

No description provided.

@chrisballinger
Copy link
Member

Thanks! This is awesome

@stigger
Copy link
Contributor Author

stigger commented Mar 16, 2020

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
Copy link
Member

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

Copy link
Member

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stigger This is a great start! Before merging we should fix the following crash:

  • Send/receive some media messages
  • Go to Storage Management and delete media for that contact
  • Go back to conversation

The crash is related to an invalid layout, but I think the underlying issue is that there are now media messages that no longer have a corresponding OTRMediaItem. I think we'd at least want some placeholder to appear where the old media message used to be.

Btw here's a fix for the YapDatabase 4.0 migration: 772d333

@stigger
Copy link
Contributor Author

stigger commented Mar 27, 2020

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
Copy link
Member

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
Copy link
Contributor Author

stigger commented Mar 27, 2020

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
Copy link
Contributor Author

stigger commented Mar 29, 2020

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
Copy link
Member

@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: 772d333

@Neustradamus
Copy link

@stigger: Any news on it?

@chrisballinger
Copy link
Member

@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
Copy link
Contributor Author

stigger commented Apr 9, 2020

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
Copy link

Nice!

Copy link
Member

@chrisballinger chrisballinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must have been some other layout bug, after further testing I think this is good to merge! Thank you!

@chrisballinger chrisballinger merged commit aa6c3bb into ChatSecure:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants