feat: rework "text only MMS" implementation #674
+171
−37
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.
Member
|
Still working on this? |
Author
|
It is effectively in RFC state: Done as-is and mergeable, but I don't know whether you agree with the design choice I described in the PR notes |
naveensingh
changed the title
naveensingh
requested changes
Feb 3, 2026
Member
naveensingh
left a comment
naveensingh
left a comment
There was a problem hiding this comment.
Ah, I assumed otherwise as it was marked as a draft.
Yeah, I don't agree with the current approach in this PR. This makes the export behavior implicit and converts MMS messages to SMS, which means that on import, those text-only MMS messages will be imported as SMS. This is not ideal, given that the goal of a backup feature is back up and restore things as-is.
Not closing this because I do think exporting text-only MMS messages might be useful for some users, so if you want to continue work on this, a cleaner way to solve that usecase will be to split the "Export MMS" checkbox in the export dialog:
- Export SMS
- Export MMS (text only)
- Export MMS (media)
That way, users who want text-only MMS can export them without converting them to SMS, and without hardcoding a hidden policy.
I'd propose removing the logic around includeTextOnlyAttachment entirely as it's not used.
If not the above, we can do this to clean up the code.
PS: New features/changes require an issue first, as stated in the contribution guidelines.
Comment on lines
+104
to
+214
|
|
||
| /* stores text only MMS as SMS */ | ||
| if (!includeTextOnlyMMSasSMS) { return smsList } | ||
| val mmsProjection = arrayOf( | ||
| Mms._ID, | ||
|
|
||
| Mms.SUBSCRIPTION_ID, | ||
| /* ADDRESS : getMmsAddresses() */ | ||
| /* BODY : getParts() */ | ||
| Mms.DATE, | ||
| Mms.DATE_SENT, | ||
| Mms.LOCKED, | ||
| /* PROTOCOL : null */ | ||
| Mms.READ, | ||
| Mms.STATUS, | ||
| Mms.MESSAGE_BOX, | ||
| /* SERVICE_CENTER : null */ | ||
| ) | ||
|
|
||
| val mmsSelection = "${Mms.THREAD_ID} = ?" | ||
|
|
||
| threadIds.map { it.toString() }.forEach { threadId -> | ||
| val selectionArgs = arrayOf(threadId) | ||
| context.queryCursor(Mms.CONTENT_URI, mmsProjection, mmsSelection, selectionArgs) { cursor -> | ||
| val mmsId = cursor.getLongValue(Mms._ID) | ||
|
|
||
| val subscriptionId = cursor.getLongValue(Mms.SUBSCRIPTION_ID) | ||
| val addresses = getMmsAddresses(mmsId) | ||
| val address = addresses.first { it.type == PduHeaders.FROM }.address | ||
|
|
||
| /* body done at the end */ | ||
|
|
||
| val date = cursor.getLongValue(Mms.DATE) * 1000 | ||
| val dateSent = cursor.getLongValue(Mms.DATE_SENT) * 1000 | ||
| val locked = cursor.getIntValue(Mms.LOCKED) | ||
| val protocol = null | ||
| val read = cursor.getIntValue(Mms.READ) | ||
| val mmsStatus = cursor.getIntValueOrNull(Mms.STATUS) | ||
| val status = when (mmsStatus) { | ||
| null, PduHeaders.STATUS_UNRECOGNIZED, PduHeaders.STATUS_INDETERMINATE -> { | ||
| Telephony.TextBasedSmsColumns.STATUS_NONE | ||
| } | ||
| PduHeaders.STATUS_EXPIRED, PduHeaders.STATUS_UNREACHABLE, PduHeaders.STATUS_REJECTED -> { | ||
| Telephony.TextBasedSmsColumns.STATUS_FAILED | ||
| } | ||
| PduHeaders.STATUS_DEFERRED, PduHeaders.STATUS_FORWARDED -> { | ||
| Telephony.TextBasedSmsColumns.STATUS_PENDING | ||
| } | ||
| PduHeaders.STATUS_RETRIEVED -> { | ||
| Telephony.TextBasedSmsColumns.STATUS_COMPLETE | ||
| } | ||
| else -> { | ||
| /* unreachable? */ | ||
| Telephony.TextBasedSmsColumns.STATUS_NONE | ||
| } | ||
| } | ||
|
|
||
| val messageBox = cursor.getIntValue(Mms.MESSAGE_BOX) | ||
| val type = when (messageBox) { | ||
| Telephony.BaseMmsColumns.MESSAGE_BOX_INBOX -> { | ||
| Telephony.TextBasedSmsColumns.MESSAGE_TYPE_INBOX | ||
| } | ||
| Telephony.BaseMmsColumns.MESSAGE_BOX_OUTBOX -> { | ||
| Telephony.TextBasedSmsColumns.MESSAGE_TYPE_OUTBOX | ||
| } | ||
| Telephony.BaseMmsColumns.MESSAGE_BOX_DRAFTS -> { | ||
| Telephony.TextBasedSmsColumns.MESSAGE_TYPE_DRAFT | ||
| } | ||
| Telephony.BaseMmsColumns.MESSAGE_BOX_FAILED -> { | ||
| Telephony.TextBasedSmsColumns.MESSAGE_TYPE_FAILED | ||
| } | ||
| Telephony.BaseMmsColumns.MESSAGE_BOX_SENT -> { | ||
| Telephony.TextBasedSmsColumns.MESSAGE_TYPE_SENT | ||
| } | ||
| else -> { | ||
| /* unreachable */ | ||
| Telephony.TextBasedSmsColumns.MESSAGE_TYPE_ALL | ||
| } | ||
| } | ||
|
|
||
| val serviceCenter = null | ||
|
|
||
| /* We do not rely on TEXT_ONLY MMS flag, see also getMmsMessages() */ | ||
| val parts = getParts(mmsId) | ||
| val smil = parts.filter { it.contentType == ContentType.APP_SMIL } | ||
| val plain = parts.filter { it.contentType == ContentType.TEXT_PLAIN } | ||
| val others = parts.filter { it.contentType != ContentType.APP_SMIL && it.contentType != ContentType.TEXT_PLAIN } | ||
|
|
||
| if (smil.size <= 1 && plain.size == 1 && others.size == 0) { | ||
| val body = plain.first().text | ||
|
|
||
| smsList.add( | ||
| SmsBackup( | ||
| subscriptionId = subscriptionId, | ||
| address = address, | ||
| body = body, | ||
| date = date, | ||
| dateSent = dateSent, | ||
| locked = locked, | ||
| protocol = protocol, | ||
| read = read, | ||
| status = status, | ||
| type = type, | ||
| serviceCenter = serviceCenter | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
Member
There was a problem hiding this comment.
The getMmsMessages() function should have been reused here to avoid code duplication. Also, some code comments here are unnecessary.
naveensingh
added
the
waiting for author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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).