<table><tr><td style="">albertvaka 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/D5876" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I've just found time to compile the app with your patch and it looks great! Sorry for the delay. We will need to add a custom explanation for every plugin, but we can do that easily by overriding the getPermissionExplanationDialog(). Nicely done!</p>
<p>I've seen that you added a function getOptionalPermissions() that you are not using, though. Is this work in progress and you plan to use it as part of this same code review? It would be nice to have a second list of "plugins that can load, but could do more stuff if you gave extra permissions". However, we can leave this out for a second iteration and merge the current patch, because it's already functional. Feel free to tell me if you want to do more changes to this patch or if it's ready to merge :)</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/D5876#inline-24780" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">SharePlugin.java:211</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 class="bright"> </span><span style="color: #aa4000"><span class="bright">if</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">successful</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(251, 175, 175, .7);"> <span class="bright"> </span><span class="n"><span class="bright">Intent</span></span><span class="bright"> </span><span class="n"><span class="bright">intent</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">new</span></span><span class="bright"> </span><span class="n"><span class="bright">Intent</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">Intent</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">.</span></span><span class="bright"></span><span style="color: #354bb3"><span class="bright">ACTION_VIEW</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(251, 175, 175, .7);"> <span class="bright"> </span><span class="n"><span class="bright">intent</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">.</span></span><span class="bright"></span><span style="color: #354bb3"><span class="bright">setDataAndType</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">destinationUri</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">mimeType</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);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// Nougat requires share:// URIs instead of file:// URIs</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// TODO use FileProvider for >Nougat</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Good catch :)</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/D5876#inline-24779" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">TelephonyPlugin.java:291</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">public</span> <span class="n">String</span><span style="color: #aa2211">[]</span> <span style="color: #004012">getRequiredPermissions</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 style="color: #aa4000">return</span> <span style="color: #aa4000">new</span> <span class="n">String</span><span style="color: #aa2211">[]{</span><span class="n">Manifest</span><span style="color: #aa2211">.</span><span style="color: #354bb3">permission</span><span style="color: #aa2211">.</span><span style="color: #354bb3">READ_PHONE_STATE</span><span style="color: #aa2211">,</span> <span class="n">Manifest</span><span style="color: #aa2211">.</span><span style="color: #354bb3">permission</span><span style="color: #aa2211">.</span><span style="color: #354bb3">READ_CONTACTS</span><span style="color: #aa2211">,</span> <span class="n">Manifest</span><span style="color: #aa2211">.</span><span style="color: #354bb3">permission</span><span style="color: #aa2211">.</span><span style="color: #354bb3">SEND_SMS</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></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This plugin only needs to RECEIVE_SMS, but not send them.</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/D5876" rel="noreferrer">https://phabricator.kde.org/D5876</a></div></div><br /><div><strong>To: </strong>nicolasfella, KDE Connect<br /><strong>Cc: </strong>albertvaka, aboudhar, seebauer, progwolff, MayeulC, menasshock, ach, apol, hkaelberer<br /></div>