D11698: Add helper for reading SMS messages

Simon Redman noreply at phabricator.kde.org
Sat Mar 31 22:18:39 UTC 2018


sredman added inline comments.

INLINE COMMENTS

> mtijink wrote in SMSHelper.java:44
> `getSMSURIBad`? I think `getSMSURIBase` is more logical. Also, these can easily be inlined in the `getSMSUri` function.

As the comment suggests, using this method to access messages is a bad idea. Thus the method name. In the standard library it resolves to the same thing as Telephony.Sms.CONTENT_URI, so I expect it will work most of the time, but it is very likely that some older phones have designed their own way of doing things and this method will not work, possibly even causing spectacular explosions

With that in mind, the reason I left these two in functions is:

1. In case we need to add more special-case URI-getting code in the future
2. So that I could put a big header comment on getSMSURIBad explaining why it was a bad idea to use that method, but I guess I didn't do a good enough job :)

I suppose getSMSURIGood could be inlined, since it doesn't need much explaination

> mtijink wrote in SMSHelper.java:76
> The return type could use some improvements. I'd suggest a data class holding these result with logical names.
> 
> Also, `getSMS` is not really discriptive for what the function actually does (getting SMS threads).

These are both good points, especially with respect to creating a custom class for the return type. I will definitely do that.

The more I think about what I need from the SMS database in order to support the SMS app, the more I realize that this code in its current form won't work anyway. I will need more helper methods, meaning I will need to do better names.

> mtijink wrote in SMSHelper.java:103
> Android coding style.

Darn it. I was doing so well up to here. I have been doing brace-on-next-line for so long it's hard to not!

REPOSITORY
  R225 KDE Connect - Android application

REVISION DETAIL
  https://phabricator.kde.org/D11698

To: sredman
Cc: mtijink, #kde_connect, Danial0_0, krantideep95, Pitel, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180331/01392cc2/attachment.html>


More information about the KDEConnect mailing list