Plugin classes refactor #3

desyncr · 2021-10-18T00:14:29Z

Picking up from branch #2 with original attribution. I'll be working on the comments left at https://github.com/freenet/plugin-UPnP2/pull/2/files:

Add some Todos.

interrupt thread for InterruptedException

desyncr · 2021-10-18T13:34:46Z

Few notes:

In 909858d:

    Service connectionService = discoverConnectionService(device);
    if (commonService == null) {
        return;
    }

The IDE indicates that commonService == null is always false. Not sure how it comes up with that as discoverConnectionService clearly returns a NULL under certain conditions.

In 646ead5:

    Map<String, StateVariableValue<?>> values = sub.getCurrentValues();

The IDE indicates that it's an "unchecked assignment: Reason: 'sub' has raw type, so result of getCurrentValues is erased".

desyncr · 2021-11-05T09:22:19Z

@ArneBab @Bombe Let me know how it looks!

ArneBab · 2022-06-18T16:29:00Z

@Bombe is this good to go now?

ArneBab · 2022-11-28T00:06:57Z

@Bombe is this good to go now?

Bombe · 2022-12-11T18:38:41Z

Few notes:

In 909858d:
    Service connectionService = discoverConnectionService(device);
    if (commonService == null) {
        return;
    }
The IDE indicates that commonService == null is always false. Not sure how it comes up with that as discoverConnectionService clearly returns a NULL under certain conditions.

Sure, but connectionService is not actually checked for null here. commonService, however, was already found to be non-null a couple of lines earlier so your IDE is quite right in this case. :)

Bombe · 2022-12-12T17:08:47Z

And for this one:

In 646ead5:
    Map<String, StateVariableValue<?>> values = sub.getCurrentValues();
The IDE indicates that it's an "unchecked assignment: Reason: 'sub' has raw type, so result of getCurrentValues is erased".

Yes, sub is a GENASubscription (as per the inherited method from SubscriptionCallback) and GENASubscription is supposed to have a type parameter. As a consequence of that sub.getCurrentValues() won’t have any type parameters which is far less problematic than the other issue. :)

We could check whether a newer version of cling (2.1.2 is available, we’re using 2.1.0) has type parameters in its method declarations; other than that there’s little we can do. Cast it, ignore it, either would be fine with me.

Bombe

I haven’t delved into the UPnP protocol at all, all the synchronous network communication seems really weird to me but I’m assuming it works. 🙂

Bombe · 2022-12-12T18:39:26Z

src/main/java/plugins/UPnP2/ClingTest.java

-			Service connectionService;
-			if ((connectionService = discoverConnectionService(device)) == null) return;
+			Service connectionService = discoverConnectionService(device);
+			if (commonService == null) {


As already mentioned in the other comment, this needs to be connectionService == null.

Bombe · 2022-12-12T18:39:42Z

src/main/java/plugins/UPnP2/IGDRegistryListener.java

-        Service connectionService;
-        if ((connectionService = discoverConnectionService(device)) == null) return;
+        Service connectionService = discoverConnectionService(device);
+        if (commonService == null) {


Bombe · 2022-12-12T18:42:37Z

src/main/java/plugins/UPnP2/IGDRegistryListener.java

+        });
+    }
+
+    private UpnpService upnpService;


This should be final.

Bombe · 2022-12-12T18:49:53Z

src/main/java/plugins/UPnP2/PortMappingTicker.java

+        });
+    }
+
+    private ServiceManager serviceManager;


Member variables initialized in the constructor should be final.

Bombe · 2022-12-12T18:52:50Z

src/main/java/plugins/UPnP2/PortMappingTicker.java

+
+        serviceManager.waitForBooting();
+
+        synchronized (this) {


Which of the members in this synchronized are actually protected by it? Every single one of them is also used without a synchronized block, so it’s either unnecessary or broken. :)

Bombe · 2022-12-12T18:58:14Z