Maintenance: Create a helper function to check version of Kodi and JSON API

[wutschel]

Currently there a checks for version of Kodi server (e.g. 19.0) or the JSON API (e.g. 12.7.0) implemented in many places. It will improve code readability and maintainability to implement helper functions for this.

[wutschel]

I implemented a solution which reads quite nice when using it, examples:

[Utilities isKodiVersion:laterThan version:@"11"]
[Utilities isKodiVersion:earlierThan version:@"13"]
[Utilities isAPIVersion:earlierThan version:@"12.7.0"]
[Utilities isAPIVersion:laterOrSameAs version:@"6.32.4"]

But I feel the parsing to evaluate the string and the logic/math to compare seem quite expensive.

+ (BOOL)checkAbstractedVersion:(int)version compare:(VersionComparisonType)compare targetVersion:(int)targetVersion {
    BOOL result;
    switch (compare) {
        case earlierThan:
            result = version < targetVersion;
            break;
        case earlierOrSameAs:
            result = version <= targetVersion;
            break;
        case sameAs:
            result = version == targetVersion;
            break;
        case laterOrSameAs:
            result = version >= targetVersion;
            break;
        case laterThan:
            result = version > targetVersion;
            break;
        default:
            NSAssert(NO, @"checkAbstractedVersion: unknown comparison mode %d", compare);
            break;
    }
    return result;
}

+ (BOOL)checkVersion:(int*)currentVersion compare:(VersionComparisonType)compare targetVersion:(NSString*)targetVersion {
    int targetValue = 0;
    int currentValue = 0;
    NSMutableString *string = [targetVersion mutableCopy];
    for (int i = 0; i < 3; ++i) {
        currentValue = currentValue * 1000 + currentVersion[i];
        targetValue = targetValue * 1000 + [string intValue];
        NSString *checker = [NSString stringWithFormat:@"%d.", [string intValue]];
        NSRange range = [string rangeOfString:checker];
        if (range.location == NSNotFound || range.location + range.length == string.length) {
            break;
        }
        [string deleteCharactersInRange:range];
    }
    return [Utilities checkAbstractedVersion:currentValue compare:compare targetVersion:targetValue];
}

+ (BOOL)isAPIVersion:(VersionComparisonType)compare version:(NSString*)versionString {
    int APIVersion[3] = {AppDelegate.instance.APImajorVersion,
                         AppDelegate.instance.APIminorVersion,
                         AppDelegate.instance.APIpatchVersion};
    return [Utilities checkVersion:APIVersion compare:compare targetVersion:versionString];
}

+ (BOOL)isKodiVersion:(VersionComparisonType)compare version:(NSString*)versionString {
    int KodiVersion[3] = {AppDelegate.instance.serverVersion,
                          AppDelegate.instance.serverMinorVersion,
                          0};
    return [Utilities checkVersion:KodiVersion compare:compare targetVersion:versionString];
}

[kambala-decapitator]

I strongly suggest to create a dedicated struct/class that holds version number and avoid strings, see NSOperatingSystemVersion for an example (you could even simply typedef it).

3 should be a constant of course.

[wutschel]

You mean instead of the human readable comparison to "10.2.44", we compare to something like {10, 2, 44}?

[kambala-decapitator]

You mean instead of the human readable comparison to "10.2.44", we compare to something like {10, 2, 44}?

exactly

[wutschel]

Ok, will look into this.

[wutschel]

I could not find a good way to handle this when still trying to maintain the possibility to check for incomplete versions. E.g. we are checking for Kodi server version == N. This is true for all subversions of N (N.0, N.1, ...). Without using NSArray or NSString I came back to create three methods which allow to compare with a dedicated depth. Not as user friendly as just using a version string, but a lot less complex to compute (mainly a condition tree) and still human readable.

[Utilities isAPIVersion:sameAs major:12 minor:4 patch:0];
[Utilities isAPIVersion:laterOrSameAs major:12 minor:4 patch:0];
[Utilities isAPIVersion:sameAs major:12 minor:4];
[Utilities isAPIVersion:laterThan major:11];

[kambala-decapitator]

you could introduce enum Component with 4 values: 'all' and each dedicated component, and 2 methods: one with Component parameter and one without (defaults to all).

[wutschel]

Just to get the complete picture on the solution with three methods for API and two methods for Kodi: https://github.com/wutschel/Official-Kodi-Remote-iOS/tree/rework_version_2

[kambala-decapitator]

array manipulation looks really ugly :) that's why I suggest a struct/class.

You could also check if NSComparisonResult fits here.

[wutschel]

Not sure, if this is worth the hassle. I will see, if I can find a string based less complex solution. If this is not working out well, I will focus on something else.

Edit: Back to string based input, but less complex logic and using NSComparisonResult and caseInsensitiveCompare at the core. Still need to cut the length of the system version (API or Kodi) to the length of the targeted version in a pre-processing. https://github.com/wutschel/Official-Kodi-Remote-iOS/tree/rework_version_3

[kambala-decapitator]

this one looks better!

• Kodi version can also be x.y.z, can't it?
• I think you can refactor both methods to a single helper method that accepts version triple
• better compare strings using NSNumericSearch instead, I don't think that sensitivity matters here

[wutschel]

Yes, the code can be compressed a bit further and the Kodi check can be enhanced to also accept x.y.z. I can also change the comparison to use NSNumericSearch.

Edit: Pushed a commit to refactor the code.

[wutschel]

Closed as even though this provides an easy-to-read version check, this seems a complex solution (also at runtime) for overall few places where a complicated outline check is done. Instead, I will create a PR to move some of these checks to dedicated helper functions (e.g. hasPvrSupport()).

To keep the code I am adding the patch of the version checker here.

diff --git a/XBMC Remote/VersionCheck.h b/XBMC Remote/VersionCheck.h
index 5c61ad86..33c67589 100644
--- a/XBMC Remote/VersionCheck.h	
+++ b/XBMC Remote/VersionCheck.h	
@@ -9,8 +9,18 @@
 #import <Foundation/Foundation.h>
 #import "AppDelegate.h"
 
+typedef enum {
+    earlierThan,
+    earlierOrSameAs,
+    sameAs,
+    laterOrSameAs,
+    laterThan
+} VersionComparisonType;
+
 @interface VersionCheck : NSObject
 
++ (BOOL)isAPIVersion:(VersionComparisonType)compare version:(NSString*)targetVersion;
++ (BOOL)isKodiVersion:(VersionComparisonType)compare version:(NSString*)targetVersion;
 + (BOOL)hasRecordingIdPlaylistSupport;
 
 @end
diff --git a/XBMC Remote/VersionCheck.m b/XBMC Remote/VersionCheck.m
index 80707adc..4cad2257 100644
--- a/XBMC Remote/VersionCheck.m	
+++ b/XBMC Remote/VersionCheck.m	
@@ -12,6 +12,66 @@
 
 @implementation VersionCheck
 
++ (BOOL)evaluateComparisonRequest:(VersionComparisonType)compare againstComparison:(NSComparisonResult)comparison {
+    BOOL result;
+    switch (compare) {
+        case earlierThan:
+            result = comparison == NSOrderedAscending;
+            break;
+        case earlierOrSameAs:
+            result = comparison != NSOrderedDescending;
+            break;
+        case sameAs:
+            result = comparison == NSOrderedSame;
+            break;
+        case laterOrSameAs:
+            result = comparison != NSOrderedAscending;
+            break;
+        case laterThan:
+            result = comparison == NSOrderedDescending;
+            break;
+        default:
+            NSAssert(NO, @"evaluateComparisonRequest: unknown comparison mode %d", compare);
+            break;
+    }
+    return result;
+}
+
++ (BOOL)compareVersion:(int)major minor:(int)minor patch:(int)patch compare:(VersionComparisonType)compare targetVersion:(NSString*)targetVersion {
+    NSUInteger num = [targetVersion componentsSeparatedByString:@"."].count;
+    NSString *systemVersion = @"";
+    switch (num) {
+        case 3:
+            systemVersion = [NSString stringWithFormat:@".%d", patch];
+        case 2:
+            systemVersion = [NSString stringWithFormat:@".%d%@", minor, systemVersion];
+        case 1:
+            systemVersion = [NSString stringWithFormat:@"%d%@", major, systemVersion];
+            break;
+        default:
+            NSAssert(NO, @"compareVersion: called with non supported version string.");
+        break;
+    }
+    NSComparisonResult comparisonResult = [systemVersion compare:targetVersion options:NSNumericSearch];
+    return [VersionCheck evaluateComparisonRequest:compare againstComparison:comparisonResult];
+}
+
++ (BOOL)isAPIVersion:(VersionComparisonType)compare version:(NSString*)targetVersion {
+    return [self compareVersion:AppDelegate.instance.APImajorVersion
+                          minor:AppDelegate.instance.APIminorVersion
+                          patch:AppDelegate.instance.APIpatchVersion
+                        compare:compare
+                  targetVersion:targetVersion];
+}
+
++ (BOOL)isKodiVersion:(VersionComparisonType)compare version:(NSString*)targetVersion {
+    return [self compareVersion:AppDelegate.instance.serverVersion
+                          minor:AppDelegate.instance.serverMinorVersion
+                          patch:0
+                        compare:compare
+                  targetVersion:targetVersion];
+}
+
 + (BOOL)hasRecordingIdPlaylistSupport {
     // Since API 12.7.0 Kodi server can handle Playlist.Insert and Playlist.Add for recordingid.
     return (AppDelegate.instance.APImajorVersion == 12 && AppDelegate.instance.APIminorVersion >= 7) || AppDelegate.instance.APImajorVersion > 12;