saros-project / saros

Open Source IDE plugin for distributed collaborative software development
https://www.saros-project.org
GNU General Public License v2.0
158 stars 52 forks source link

[Refactor] Replace weupnp library and UPnP classes #1122

Closed linusb20 closed 3 years ago

linusb20 commented 3 years ago

Removes local patch of weupnp library from saros and replaces it with an internal solution to provide port mapping using upnp. Resolves issues of saros classes directly importing weupnp classes by adding interfaces to abstract over upnp logic.

This was done to simplify the code and logic behind upnp port mapping.

Your checklist for this pull request

Please, check that:

Description

Please describe your pull request.

Thank you!

krzok commented 3 years ago

So instead of just using the latest official release which includes our custom patch you

  1. reinvent the wheel and also introducing the same bug again (no timeout on the http connections)
  2. refactor dozens line of codes that are actually working fine

For the first point we think that the new library we used is both smaller and clearer compared to the old WeUPnP implementation, additionally there was the old Gateway Class which was implemented in several spots directly into Saros instead of being implemented via an Interface. The second point is based on the first due to us implementing the Gateway class to fullfill Saros coding guidelines we had to refactor other classes to use the new Interface

srossbach commented 3 years ago

Your "internal solution" is just a copy of https://github.com/adolfintel/WaifUPnP

All you did was to remove any hints / copyright notices.

krzok commented 3 years ago

It is obviously true that its based on WaifUPnP however it is not just a copy, we wanted to originally use the Waif library however that was not possible due to Saros specific implementation of upnp including the use of USNs which is not a common feature to waif, or any other upnp implementation

linusb20 commented 3 years ago

I know this PR is closed. I just wanted to add that it wasn't our intention to conceal the fact that our code is based on WaifUPnP. As @krzok said, we had modified WaifUPnP to fit the needs of saros. I carelessly removed any copyright notices and failed to think about it further before making a pull request.

srossbach commented 3 years ago

The task was (as already mentioned) to simply update the existing library. There was no request for using another library nor a refactor / rework of the existing code. I do not know who has assigned you this task.