Review Request 121104: Manual IP/host specification for pairing through VPN or separate netwoks

[Achilleas Koutsou]

> On Nov. 17, 2014, 4:54 a.m., Albert Vaca Cintora wrote:
> > 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?

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.

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:

- Create empty list of IP addresses/hosts
- Get the custom IP/host list from the PreferenceManager (if any are specified/stored)
- Append 255.255.255.255 to the list
- Send a packet to each address in the list


> On Nov. 17, 2014, 4:54 a.m., Albert Vaca Cintora wrote:
> > src/main/java/org/kde/kdeconnect/UserInterface/CustomDevicesActivity.java, line 86
> > <https://git.reviewboard.kde.org/r/121104/diff/1/?file=327745#file327745line86>
> >
> >     Do we really need to specify a port? If it is needed, how can it be working without it?

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.


For now, I'll simply leave it as is and remove the TODO item.


> On Nov. 17, 2014, 4:54 a.m., Albert Vaca Cintora wrote:
> > src/main/java/org/kde/kdeconnect/UserInterface/CustomDevicesActivity.java, line 111
> > <https://git.reviewboard.kde.org/r/121104/diff/1/?file=327745#file327745line111>
> >
> >     Why is this code commented? If it's not needed we should just remove it.

Indeed it's unnecessary. Sorry for the clutter. It was left over from when I was copying code from the MainSettingsActivity.


- Achilleas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121104/#review70495
-----------------------------------------------------------


On Nov. 11, 2014, 4:01 p.m., Achilleas Koutsou wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121104/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2014, 4:01 p.m.)
> 
> 
> Review request for kdeconnect.
> 
> 
> Bugs: 326509
>     http://bugs.kde.org/show_bug.cgi?id=326509
> 
> 
> Repository: kdeconnect-android
> 
> 
> Description
> -------
> 
> 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).
> 
> This resolves [bug 326509](https://bugs.kde.org/show_bug.cgi?id=326509).
> 
> 
> Diffs
> -----
> 
>   src/main/AndroidManifest.xml 886e27c 
>   src/main/java/org/kde/kdeconnect/Backends/CustomLinkBackend/CustomLink.java PRE-CREATION 
>   src/main/java/org/kde/kdeconnect/Backends/CustomLinkBackend/CustomLinkProvider.java PRE-CREATION 
>   src/main/java/org/kde/kdeconnect/BackgroundService.java c8d4e36 
>   src/main/java/org/kde/kdeconnect/UserInterface/CustomDevicesActivity.java PRE-CREATION 
>   src/main/java/org/kde/kdeconnect/UserInterface/MainActivity.java aba1336 
>   src/main/res/layout/custom_ip_list.xml PRE-CREATION 
>   src/main/res/menu/main.xml 189b036 
>   src/main/res/values/strings.xml 5801b31 
> 
> Diff: https://git.reviewboard.kde.org/r/121104/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Achilleas Koutsou
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20141120/99f290bd/attachment.html>