<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121104/">https://git.reviewboard.kde.org/r/121104/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2014, 4:54 a.m. EET, <b>Albert Vaca Cintora</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see that most of the code from CustomLinkProvider is shared with LanLinkProvider, so my suggestion is to merge them into one class to avoid having duplicate code to maintain. If you think there is a good reason to keep this in a separate plugin, then we should remove all the UDP-related code from here, because it's not used anymore. What do you think?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You are right. There's a lot of unnecessary duplication, which would make maintaining future changes tedious. I'm now merging the two classes and I see that I can simply add the custom IP list code before or after the broadcast packet is sent. In fact, I am now considering simply adding the broadcast address to the list of IP addresses after they are read from the preference manager. Therefore, the onStart() method in LanLinkProvider would simply read in the list of saved IP addresses (or hosts), create an empty arraylist if the list is empty, and then append the broadcast address to this list.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If I understand the code in the LanLinkProvider correctly, it creates a packet addressed to "-1.-1.-1.-1" which always (?) results in 255.255.255.255, the broadcast address of the zero network. So in merging my code into the LanLinkProvider, I changed onStart() in the way described above:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Create empty list of IP addresses/hosts</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Get the custom IP/host list from the PreferenceManager (if any are specified/stored)</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Append 255.255.255.255 to the list</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Send a packet to each address in the list</li>
</ul></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2014, 4:54 a.m. EET, <b>Albert Vaca Cintora</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/121104/diff/1/?file=327745#file327745line86" style="color: black; font-weight: bold; text-decoration: underline;">src/main/java/org/kde/kdeconnect/UserInterface/CustomDevicesActivity.java</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">86</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// </span><span class="cs">TODO</span><span class="c1">: allow specifying port as well</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do we really need to specify a port? If it is needed, how can it be working without it?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If no port is specified, it simply uses the default KDE Connect ports (1714-1764). The initial plan was to be able to optionally specify a port along with the IP to be used in cases where the default ports are blocked/unavailable. I might add this at a later time if necessary. It might require also having an option to specify a port number on the desktop side. Even without that, I can imagine a situation where one needs to go through a firewall and could specify a port number on the Android side and have the firewall receive on that port and forward to the default port for the desktop side.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For now, I'll simply leave it as is and remove the TODO item.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2014, 4:54 a.m. EET, <b>Albert Vaca Cintora</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/121104/diff/1/?file=327745#file327745line111" style="color: black; font-weight: bold; text-decoration: underline;">src/main/java/org/kde/kdeconnect/UserInterface/CustomDevicesActivity.java</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">111</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> private void initPreferences(final ListPreference ipListPref) {</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why is this code commented? If it's not needed we should just remove it.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Indeed it's unnecessary. Sorry for the clutter. It was left over from when I was copying code from the MainSettingsActivity.</p></pre>
<br />
<p>- Achilleas</p>
<br />
<p>On November 11th, 2014, 4:01 p.m. EET, Achilleas Koutsou wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for kdeconnect.</div>
<div>By Achilleas Koutsou.</div>
<p style="color: grey;"><i>Updated Nov. 11, 2014, 4:01 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=326509">326509</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdeconnect-android
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Created new backend (CustomLinkBackend) which reads a list of user-specified IP addresses or host names and attempts to send KDE-connect packets to them, bypassing the autodiscovery which usually happens via broadcast packets. This allows for the pairing and communication between two devices which are connected via a gateway (or route) that does not allow broadcasts (e.g., OpenVPN in tunnel mode, separate networks). This patch also adds a new menu and activity for adding and removing IP addresses and hostnames (Custom device list).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This resolves <a href="https://bugs.kde.org/show_bug.cgi?id=326509" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">bug 326509</a>.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/main/AndroidManifest.xml <span style="color: grey">(886e27c)</span></li>
<li>src/main/java/org/kde/kdeconnect/Backends/CustomLinkBackend/CustomLink.java <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/main/java/org/kde/kdeconnect/Backends/CustomLinkBackend/CustomLinkProvider.java <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/main/java/org/kde/kdeconnect/BackgroundService.java <span style="color: grey">(c8d4e36)</span></li>
<li>src/main/java/org/kde/kdeconnect/UserInterface/CustomDevicesActivity.java <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/main/java/org/kde/kdeconnect/UserInterface/MainActivity.java <span style="color: grey">(aba1336)</span></li>
<li>src/main/res/layout/custom_ip_list.xml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/main/res/menu/main.xml <span style="color: grey">(189b036)</span></li>
<li>src/main/res/values/strings.xml <span style="color: grey">(5801b31)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121104/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>