<table><tr><td style="">sredman added a comment.
</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/D17002">View Revision</a></tr></table><br /><div><div><p>This is good work! Thank you. Were you able to get the app with this patch working for your own phone? If not, could you share the full stack trace (paste.kde.org) so I can have a look?<br />
Don't forget to mark the GCi task as completed so I can approve it once we get this merged.</p></div></div><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/D17002#inline-92657">View Inline</a><span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:122-123</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * @param <span class="bright">selectionArgs Parameters for selection. May be null.</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * @<span class="bright">return List of messages matching the filter</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> *<span class="bright">/</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">private</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">static</span></span><span class="bright"> </span><span class="n"><span class="bright">List</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"></span><span class="n"><span class="bright">Message</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">></span></span><span class="bright"> </span><span style="color: #004012"><span class="bright">getMessagesWithFilter</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">Context</span></span><span class="bright"> </span><span class="n"><span class="bright">context</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">String</span></span><span class="bright"> </span><span class="n"><span class="bright">selection</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">String</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">[]</span></span><span class="bright"> </span><span class="n"><span class="bright">selectionArgs</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">)</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @param <span class="bright">context android.content.Context running the request.</span></span><span class="bright">
</span></div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @<span class="bright">param selection Parameterizable filter to use with the ContentResolver query. May be null with mode = GetMessagesMode.CONVERSATIONS.</span></span><span class="bright">
</span></div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> *<span class="bright"> @param selectionArgs Parameters for selection. May be null with mode = GetMessagesMode.CO NVERSATIONS.</span></span><span class="bright">
</span></div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> <span class="bright"> * @return Returns HashMap<ThreadID, List<Message>>, which is transformed in caller functions into other classes.</span></span><span class="bright">
</span></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I missed this before (sorry). These comments still refer to one of the earlier revisions</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/D17002#inline-92658">View Inline</a><span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:141-142</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">do</span> <span style="color: #aa2211">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">int</span> <span class="n">thread</span> <span style="color: #aa2211">=</span> <span class="n">myCursor</span><span style="color: #aa2211">.</span><span style="color: #354bb3">getInt</span><span style="color: #aa2211">(</span><span class="n">threadColumn</span><span style="color: #aa2211">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">HashMap</span><span style="color: #aa2211"><</span><span class="n">String</span><span style="color: #aa2211">,</span> <span class="n">String</span><span style="color: #aa2211">></span> <span class="n">messageInfo</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">HashMap</span><span style="color: #aa2211"><>();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This variable is unused because we pull threadID from the Message later, so might as well get rid of it</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/D17002#inline-92660">View Inline</a><span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:152-158</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span><span style="color: #aa2211">(</span><span class="n">toReturn</span><span style="color: #aa2211">.</span><span style="color: #354bb3">get</span><span style="color: #aa2211">(</span><span class="n">threadID</span><span style="color: #aa2211">)</span> <span style="color: #aa2211">==</span> <span style="color: #000a65">null</span><span style="color: #aa2211">)</span> <span style="color: #aa2211">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">List</span><span style="color: #aa2211"><</span><span class="n">Message</span><span style="color: #aa2211">></span> <span class="n">messageList</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">ArrayList</span><span style="color: #aa2211"><>();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">messageList</span><span style="color: #aa2211">.</span><span style="color: #354bb3">add</span><span style="color: #aa2211">(</span><span class="n">message</span><span style="color: #aa2211">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">toReturn</span><span style="color: #aa2211">.</span><span style="color: #354bb3">put</span><span style="color: #aa2211">(</span><span class="n">threadID</span><span style="color: #aa2211">,</span> <span class="n">messageList</span><span style="color: #aa2211">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa2211">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">else</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">toReturn</span><span style="color: #aa2211">.</span><span style="color: #354bb3">get</span><span style="color: #aa2211">(</span><span class="n">threadID</span><span style="color: #aa2211">).</span><span style="color: #354bb3">add</span><span style="color: #aa2211">(</span><span class="n">message</span><span style="color: #aa2211">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">A common way to do this same check is:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (!toReturn.containsKey(threadID)) {
toReturn.put(threadID, new ArrayList<Message>());
}
toReturn.get(threadID).add(message);</pre></div>
<p style="padding: 0; margin: 8px;">The code you currently have is fine, and if you like it this way then you should leave it this way. I only mention this because you are working on GCi, so I should share my wisdom :)</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/D17002#inline-92659">View Inline</a><span style="color: #4b4d51; font-weight: bold;">SMSHelper.java:157-158</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa2211">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">else</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">toReturn</span><span style="color: #aa2211">.</span><span style="color: #354bb3">get</span><span style="color: #aa2211">(</span><span class="n">threadID</span><span style="color: #aa2211">).</span><span style="color: #354bb3">add</span><span style="color: #aa2211">(</span><span class="n">message</span><span style="color: #aa2211">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa2211">}</span> <span style="color: #aa4000">while</span> <span style="color: #aa2211">(</span><span class="n">myCursor</span><span style="color: #aa2211">.</span><span style="color: #354bb3">moveToNext</span><span style="color: #aa2211">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Even though you <em>can</em> have an <tt style="background: #ebebeb; font-size: 13px;">else</tt> with no braces, I strongly recommend against it. The problem is that a future programmer might come along and try to add to this block (maybe he is more experienced with Python than C++) and the code will not get executed where he expects it to. Moreover, there is a possibility that could get get inserted by mistake, for instance by a <tt style="background: #ebebeb; font-size: 13px;">git merge</tt><br />
For a real-world example of where this resulted in a serious disaster, see this article: <a href="https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/" class="remarkup-link" target="_blank" rel="noreferrer">https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/</a></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/D17002">https://phabricator.kde.org/D17002</a></div></div><br /><div><strong>To: </strong>alexkovrigin, sredman<br /><strong>Cc: </strong>sredman, alexkovrigin, kdeconnect, shivanshukantprasad, skymoore, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, mikesomov, tctara, apol<br /></div>