<table><tr><td style="">sredman added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D11698">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11698#inline-58425">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mtijink</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:44</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">getSMSURIBad</tt>? I think <tt style="background: #ebebeb; font-size: 13px;">getSMSURIBase</tt> is more logical. Also, these can easily be inlined in the <tt style="background: #ebebeb; font-size: 13px;">getSMSUri</tt> function.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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</p>

<p style="padding: 0; margin: 8px;">With that in mind, the reason I left these two in functions is:</p>

<ol class="remarkup-list">
<li class="remarkup-list-item">In case we need to add more special-case URI-getting code in the future</li>
<li class="remarkup-list-item">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 :)</li>
</ol>

<p style="padding: 0; margin: 8px;">I suppose getSMSURIGood could be inlined, since it doesn't need much explaination</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11698#inline-58426">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mtijink</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:76</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The return type could use some improvements. I'd suggest a data class holding these result with logical names.</p>

<p style="padding: 0; margin: 8px;">Also, <tt style="background: #ebebeb; font-size: 13px;">getSMS</tt> is not really discriptive for what the function actually does (getting SMS threads).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">These are both good points, especially with respect to creating a custom class for the return type. I will definitely do that.</p>

<p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D11698#inline-58427">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mtijink</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:103</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Android coding style.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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!</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R225 KDE Connect - Android application</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11698">https://phabricator.kde.org/D11698</a></div></div><br /><div><strong>To: </strong>sredman<br /><strong>Cc: </strong>mtijink, KDE Connect, Danial0_0, krantideep95, Pitel, adeen-s, SemperPeritus, ahmedbesbes, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol<br /></div>