Rework "text only MMS" implementation #674
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Otherwise the queryCursor will fail silently (since showErrors = false) with the exception
column 'sub' does not exist. Available columns: [_id, creator, ct_t, d_rpt, date, date_sent, locked, m_type, msg_box, read, rr, seen, text_only, st, sub_cs, sub_id, tr_id]
and will return no results at all.
Currently, the flag `includeTextOnlyAttachment` is never used in the export logic, despite being of useful value: Some MMS aren't immediately visible to users as such (e.g., very long "SMS" messages are often sent as MMS). Users wanting to only export "true" MMS (images, etc.), may be confused as to why "SMS" are included in the export. As such, introduce a logic to set this flag if only MMS export is checked, otherwise export them as-is. Similarly, users exporting SMS only may be missing these exact messages. Analogously, introduce a new flag `includeTextOnlyMMSasSMS` for SMS export, that is suppoed to export long MMS "as SMS".
This effectively converts MMS that are "text only" to SMS as part of the export process. Property values from MMS are converted to equivalents in SMS.
This flag behaved oddly, as text only attachments were *always* included, even if it is not set. However, setting it, would filter the MMS results for those MMS exclusively, that had TEXT_ONLY set. This old behavior would've been better described by "onlyIncludeTextOnlyAttachments". In addition, however, the logic is flawed as TEXT_ONLY is a property set by the PduPersister when receiving MMS. If the MMS wasn't processed by the Persister, the default value of the table row, 0, is set. This is quite often the case (in my case, all MMS were thus set to be non-text-only, despite being very much text-only). The AOSP source code also mentions that this is only a UI hint. For exports, we thus re-implement the same logic and check whether MMS messages are indeed "text only" or not.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change(s)
What changed and why
includeTextOnlyAttachmentflag in MMS exportincludeTextOnlyMMSasSMSflag for SMS exportNote: This is just a proposed feature/implementation. If the current behavior is to be kept (despite being superficially confusing), I'd propose removing the logic around
includeTextOnlyAttachmententirely as it's not used. In addition, one could forego the entire split of "export MMS/SMS only" and always export all data. My PR tries to match most closely to what perhaps was intended, not necessarily what is "best".Tests performed
Checklist
CHANGELOG.md(if applicable).