Moved minimumApiVersion detection to be executed before any first request

Refactored RESTMusicService to Kotlin
Refactored OfflineMusicService not to be a subclass of RESTMusicService
Minor fixes

ultrasonic/src/main/java/org/moire/ultrasonic/activity/DownloadActivity.java

@@ -1218,6 +1218,7 @@ public class DownloadActivity extends SubsonicTabActivity implements OnGestureLi
 			@Override
 			protected void error(final Throwable error)
 			{
+				Timber.e(error, "Exception has occurred in savePlaylistInBackground");
 				final String msg = String.format("%s %s", getResources().getString(R.string.download_playlist_error), getErrorMessage(error));
 				Util.toast(DownloadActivity.this, msg);
 			}

ultrasonic/src/main/java/org/moire/ultrasonic/activity/MainActivity.java

@@ -36,12 +36,9 @@ import org.moire.ultrasonic.R;
 import org.moire.ultrasonic.data.ActiveServerProvider;
 import org.moire.ultrasonic.data.ServerSetting;
 import org.moire.ultrasonic.service.MediaPlayerLifecycleSupport;
-import org.moire.ultrasonic.service.MusicService;
-import org.moire.ultrasonic.service.MusicServiceFactory;
 import org.moire.ultrasonic.util.Constants;
 import org.moire.ultrasonic.util.FileUtil;
 import org.moire.ultrasonic.util.MergeAdapter;
-import org.moire.ultrasonic.util.TabActivityBackgroundTask;
 import org.moire.ultrasonic.util.Util;
 
 import java.util.Collections;
@@ -153,10 +150,6 @@ public class MainActivity extends SubsonicTabActivity
 
 			adapter.addView(videosTitle, false);
 			adapter.addViews(Collections.singletonList(videosButton), true);
-
-            if (Util.isNetworkConnected(this)) {
-                new PingTask(this, false).execute();
-            }
 		}
 
 		list.setAdapter(adapter);
@@ -250,7 +243,7 @@ public class MainActivity extends SubsonicTabActivity
 		{
 			final SharedPreferences.Editor editor = preferences.edit();
 			editor.putString(Constants.PREFERENCES_KEY_CACHE_LOCATION, FileUtil.getDefaultMusicDirectory(this).getPath());
-			editor.commit();
+			editor.apply();
 		}
 	}
 
@@ -386,23 +379,4 @@ public class MainActivity extends SubsonicTabActivity
 			currentSetting.getPassword(), currentSetting.getAllowSelfSignedCertificate(),
 			currentSetting.getLdapSupport(), currentSetting.getMinimumApiVersion());
 	}
-
-	/**
-     * Temporary task to make a ping to server to get it supported api version.
-     */
-    private static class PingTask extends TabActivityBackgroundTask<Void> {
-        PingTask(SubsonicTabActivity activity, boolean changeProgress) {
-            super(activity, changeProgress);
-        }
-
-        @Override
-        protected Void doInBackground() throws Throwable {
-            final MusicService service = MusicServiceFactory.getMusicService(getActivity());
-            service.ping(getActivity(), null);
-            return null;
-        }
-
-        @Override
-        protected void done(Void result) {}
-    }
 }

ultrasonic/src/main/java/org/moire/ultrasonic/service/OfflineMusicService.java

@@ -21,12 +21,14 @@ package org.moire.ultrasonic.service;
 import android.content.Context;
 import android.graphics.Bitmap;
 import android.media.MediaMetadataRetriever;
+
+import kotlin.Pair;
 import timber.log.Timber;
 
-import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient;
-import org.moire.ultrasonic.cache.PermanentFileStorage;
 import org.moire.ultrasonic.data.ActiveServerProvider;
 import org.moire.ultrasonic.domain.Artist;
+import org.moire.ultrasonic.domain.Bookmark;
+import org.moire.ultrasonic.domain.ChatMessage;
 import org.moire.ultrasonic.domain.Genre;
 import org.moire.ultrasonic.domain.Indexes;
 import org.moire.ultrasonic.domain.JukeboxStatus;
@@ -34,10 +36,12 @@ import org.moire.ultrasonic.domain.Lyrics;
 import org.moire.ultrasonic.domain.MusicDirectory;
 import org.moire.ultrasonic.domain.MusicFolder;
 import org.moire.ultrasonic.domain.Playlist;
+import org.moire.ultrasonic.domain.PodcastsChannel;
 import org.moire.ultrasonic.domain.SearchCriteria;
 import org.moire.ultrasonic.domain.SearchResult;
 import org.moire.ultrasonic.domain.Share;
 import org.moire.ultrasonic.domain.UserInfo;
+import org.moire.ultrasonic.util.CancellableTask;
 import org.moire.ultrasonic.util.Constants;
 import org.moire.ultrasonic.util.FileUtil;
 import org.moire.ultrasonic.util.ProgressListener;
@@ -48,6 +52,7 @@ import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileReader;
 import java.io.FileWriter;
+import java.io.InputStream;
 import java.io.Reader;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -68,22 +73,11 @@ import static org.koin.java.KoinJavaComponent.inject;
 /**
  * @author Sindre Mehus
  */
-public class OfflineMusicService extends RESTMusicService
+public class OfflineMusicService implements MusicService
 {
 	private static final Pattern COMPILE = Pattern.compile(" ");
-
 	private Lazy<ActiveServerProvider> activeServerProvider = inject(ActiveServerProvider.class);
 
-	public OfflineMusicService(SubsonicAPIClient subsonicAPIClient, PermanentFileStorage storage) {
-        super(subsonicAPIClient, storage);
-    }
-
-    @Override
-	public boolean isLicenseValid(Context context, ProgressListener progressListener) throws Exception
-	{
-		return true;
-	}
-
 	@Override
 	public Indexes getIndexes(String musicFolderId, boolean refresh, Context context, ProgressListener progressListener) throws Exception
 	{
@@ -150,7 +144,7 @@ public class OfflineMusicService extends RESTMusicService
 	}
 
 	@Override
-	public MusicDirectory getMusicDirectory(String id, String artistName, boolean refresh, Context context, ProgressListener progressListener) throws Exception
+	public MusicDirectory getMusicDirectory(String id, String artistName, boolean refresh, Context context, ProgressListener progressListener)
 	{
 		File dir = new File(id);
 		MusicDirectory result = new MusicDirectory();
@@ -341,7 +335,7 @@ public class OfflineMusicService extends RESTMusicService
 	}
 
 	@Override
-	public Bitmap getAvatar(Context context, String username, int size, boolean saveToFile, boolean highQuality, ProgressListener progressListener) throws Exception
+	public Bitmap getAvatar(Context context, String username, int size, boolean saveToFile, boolean highQuality, ProgressListener progressListener)
 	{
 		try
 		{
@@ -355,7 +349,7 @@ public class OfflineMusicService extends RESTMusicService
 	}
 
 	@Override
-	public Bitmap getCoverArt(Context context, MusicDirectory.Entry entry, int size, boolean saveToFile, boolean highQuality, ProgressListener progressListener) throws Exception
+	public Bitmap getCoverArt(Context context, MusicDirectory.Entry entry, int size, boolean saveToFile, boolean highQuality, ProgressListener progressListener)
 	{
 		try
 		{
@@ -369,25 +363,7 @@ public class OfflineMusicService extends RESTMusicService
 	}
 
 	@Override
-	public void star(String id, String albumId, String artistId, Context context, ProgressListener progressListener) throws Exception
+	public SearchResult search(SearchCriteria criteria, Context context, ProgressListener progressListener)
-	{
-		throw new OfflineException("Star not available in offline mode");
-	}
-
-	@Override
-	public void unstar(String id, String albumId, String artistId, Context context, ProgressListener progressListener) throws Exception
-	{
-		throw new OfflineException("UnStar not available in offline mode");
-	}
-
-	@Override
-	public List<MusicFolder> getMusicFolders(boolean refresh, Context context, ProgressListener progressListener) throws Exception
-	{
-		throw new OfflineException("Music folders not available in offline mode");
-	}
-
-	@Override
-	public SearchResult search(SearchCriteria criteria, Context context, ProgressListener progressListener) throws Exception
 	{
 		List<Artist> artists = new ArrayList<Artist>();
 		List<MusicDirectory.Entry> albums = new ArrayList<MusicDirectory.Entry>();
@@ -531,7 +507,7 @@ public class OfflineMusicService extends RESTMusicService
 	}
 
 	@Override
-	public List<Playlist> getPlaylists(boolean refresh, Context context, ProgressListener progressListener) throws Exception
+	public List<Playlist> getPlaylists(boolean refresh, Context context, ProgressListener progressListener)
 	{
 		List<Playlist> playlists = new ArrayList<Playlist>();
 		File root = FileUtil.getPlaylistDirectory(context);
@@ -661,6 +637,45 @@ public class OfflineMusicService extends RESTMusicService
 		}
 	}
 
+
+	@Override
+	public MusicDirectory getRandomSongs(int size, Context context, ProgressListener progressListener)
+	{
+		File root = FileUtil.getMusicDirectory(context);
+		List<File> children = new LinkedList<File>();
+		listFilesRecursively(root, children);
+		MusicDirectory result = new MusicDirectory();
+
+		if (children.isEmpty())
+		{
+			return result;
+		}
+
+		Random random = new java.security.SecureRandom();
+		for (int i = 0; i < size; i++)
+		{
+			File file = children.get(random.nextInt(children.size()));
+			result.addChild(createEntry(context, file, getName(file)));
+		}
+
+		return result;
+	}
+
+	private static void listFilesRecursively(File parent, List<File> children)
+	{
+		for (File file : FileUtil.listMediaFiles(parent))
+		{
+			if (file.isFile())
+			{
+				children.add(file);
+			}
+			else
+			{
+				listFilesRecursively(file, children);
+			}
+		}
+	}
+
 	@Override
 	public void deletePlaylist(String id, Context context, ProgressListener progressListener) throws Exception
 	{
@@ -691,12 +706,6 @@ public class OfflineMusicService extends RESTMusicService
 		throw new OfflineException("Album lists not available in offline mode");
 	}
 
-	@Override
-	public String getVideoUrl(Context context, String id, boolean useFlash)
-	{
-		return null;
-	}
-
 	@Override
 	public JukeboxStatus updateJukeboxPlaylist(List<String> ids, Context context, ProgressListener progressListener) throws Exception
 	{
@@ -739,29 +748,6 @@ public class OfflineMusicService extends RESTMusicService
 		throw new OfflineException("Starred not available in offline mode");
 	}
 
-	@Override
-	public MusicDirectory getRandomSongs(int size, Context context, ProgressListener progressListener) throws Exception
-	{
-		File root = FileUtil.getMusicDirectory(context);
-		List<File> children = new LinkedList<File>();
-		listFilesRecursively(root, children);
-		MusicDirectory result = new MusicDirectory();
-
-		if (children.isEmpty())
-		{
-			return result;
-		}
-
-		Random random = new java.security.SecureRandom();
-		for (int i = 0; i < size; i++)
-		{
-			File file = children.get(random.nextInt(children.size()));
-			result.addChild(createEntry(context, file, getName(file)));
-		}
-
-		return result;
-	}
-
 	@Override
 	public MusicDirectory getSongsByGenre(String genre, int count, int offset, Context context, ProgressListener progressListener) throws Exception
 	{
@@ -804,18 +790,121 @@ public class OfflineMusicService extends RESTMusicService
 		throw new OfflineException("Updating shares not available in offline mode");
 	}
 
-	private static void listFilesRecursively(File parent, List<File> children)
+	@Override
+	public void star(String id, String albumId, String artistId, Context context, ProgressListener progressListener) throws Exception
 	{
-		for (File file : FileUtil.listMediaFiles(parent))
+		throw new OfflineException("Star not available in offline mode");
-		{
+	}
-			if (file.isFile())
+
-			{
+	@Override
-				children.add(file);
+	public void unstar(String id, String albumId, String artistId, Context context, ProgressListener progressListener) throws Exception
-			}
+	{
-			else
+		throw new OfflineException("UnStar not available in offline mode");
-			{
+	}
-				listFilesRecursively(file, children);
+	@Override
-			}
+	public List<MusicFolder> getMusicFolders(boolean refresh, Context context, ProgressListener progressListener) throws Exception
-		}
+	{
+		throw new OfflineException("Music folders not available in offline mode");
+	}
+
+	@Override
+	public MusicDirectory getAlbumList2(String type, int size, int offset, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getAlbumList2 was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public String getVideoUrl(Context context, String id, boolean useFlash) {
+		Timber.w("OfflineMusicService.getVideoUrl was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public List<ChatMessage> getChatMessages(Long since, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getChatMessages was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public void addChatMessage(String message, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.addChatMessage was called but it isn't available");
+	}
+
+	@Override
+	public List<Bookmark> getBookmarks(Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getBookmarks was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public void deleteBookmark(String id, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.deleteBookmark was called but it isn't available");
+	}
+
+	@Override
+	public void createBookmark(String id, int position, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.createBookmark was called but it isn't available");
+	}
+
+	@Override
+	public MusicDirectory getVideos(boolean refresh, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getVideos was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public SearchResult getStarred2(Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getStarred2 was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public void ping(Context context, ProgressListener progressListener) {
+	}
+
+	@Override
+	public boolean isLicenseValid(Context context, ProgressListener progressListener) {
+		return true;
+	}
+
+	@Override
+	public Indexes getArtists(boolean refresh, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getArtists was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public MusicDirectory getArtist(String id, String name, boolean refresh, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getArtist was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public MusicDirectory getAlbum(String id, String name, boolean refresh, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getAlbum was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public MusicDirectory getPodcastEpisodes(String podcastChannelId, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getPodcastEpisodes was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public Pair<InputStream, Boolean> getDownloadInputStream(Context context, MusicDirectory.Entry song, long offset, int maxBitrate, CancellableTask task) {
+		Timber.w("OfflineMusicService.getDownloadInputStream was called but it isn't available");
+		return null;
+	}
+
+	@Override
+	public void setRating(String id, int rating, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.setRating was called but it isn't available");
+	}
+
+	@Override
+	public List<PodcastsChannel> getPodcastsChannels(boolean refresh, Context context, ProgressListener progressListener) {
+		Timber.w("OfflineMusicService.getPodcastsChannels was called but it isn't available");
+		return null;
 	}
 }

ultrasonic/src/main/kotlin/org/moire/ultrasonic/activity/EditServerActivity.kt

@@ -9,7 +9,6 @@ import androidx.appcompat.app.AppCompatActivity
 import androidx.lifecycle.Observer
 import com.google.android.material.switchmaterial.SwitchMaterial
 import com.google.android.material.textfield.TextInputLayout
-import java.io.IOException
 import java.net.MalformedURLException
 import java.net.URL
 import org.koin.android.ext.android.inject
@@ -19,16 +18,14 @@ import org.moire.ultrasonic.R
 import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient
 import org.moire.ultrasonic.api.subsonic.SubsonicAPIVersions
 import org.moire.ultrasonic.api.subsonic.SubsonicClientConfiguration
-import org.moire.ultrasonic.api.subsonic.response.SubsonicResponse
 import org.moire.ultrasonic.data.ActiveServerProvider
 import org.moire.ultrasonic.data.ServerSetting
+import org.moire.ultrasonic.service.ApiCallResponseChecker.Companion.checkResponseSuccessful
 import org.moire.ultrasonic.service.MusicServiceFactory
-import org.moire.ultrasonic.service.SubsonicRESTException
 import org.moire.ultrasonic.util.Constants
 import org.moire.ultrasonic.util.ErrorDialog
 import org.moire.ultrasonic.util.ModalBackgroundTask
 import org.moire.ultrasonic.util.Util
-import retrofit2.Response
 import timber.log.Timber
 
 /**
@@ -87,6 +84,11 @@ internal class EditServerActivity : AppCompatActivity() {
                     if (t != null) {
                         currentServerSetting = t
                         setFields()
+                        // Remove the minimum API version so it can be detected again
+                        if (currentServerSetting?.minimumApiVersion != null) {
+                            currentServerSetting!!.minimumApiVersion = null
+                            serverSettingsModel.updateItem(currentServerSetting)
+                        }
                     }
                 }
             )
@@ -261,7 +263,8 @@ internal class EditServerActivity : AppCompatActivity() {
                 )
                 val subsonicApiClient = SubsonicAPIClient(configuration)
 
-                // Execute a ping to retrieve the API version. This is accepted to fail if the authentication is incorrect yet.
+                // Execute a ping to retrieve the API version.
+                // This is accepted to fail if the authentication is incorrect yet.
                 var pingResponse = subsonicApiClient.api.ping().execute()
                 if (pingResponse?.body() != null) {
                     val restApiVersion = pingResponse.body()!!.version.restApiVersion
@@ -302,28 +305,6 @@ internal class EditServerActivity : AppCompatActivity() {
         task.execute()
     }
 
-    /**
-     * Checks the Subsonic Response for application specific errors
-     */
-    private fun checkResponseSuccessful(response: Response<out SubsonicResponse?>) {
-        if (
-            response.isSuccessful &&
-            response.body()!!.status === SubsonicResponse.Status.OK
-        ) {
-            return
-        }
-        if (!response.isSuccessful) {
-            throw IOException("Server error, code: " + response.code())
-        } else if (
-            response.body()!!.status === SubsonicResponse.Status.ERROR &&
-            response.body()!!.error != null
-        ) {
-            throw SubsonicRESTException(response.body()!!.error!!)
-        } else {
-            throw IOException("Failed to perform request: " + response.code())
-        }
-    }
-
     /**
      * Finishes the Activity, after confirmation from the user if needed
      */

ultrasonic/src/main/kotlin/org/moire/ultrasonic/di/MusicServiceModule.kt

@@ -13,6 +13,7 @@ import org.moire.ultrasonic.api.subsonic.SubsonicClientConfiguration
 import org.moire.ultrasonic.cache.PermanentFileStorage
 import org.moire.ultrasonic.data.ActiveServerProvider
 import org.moire.ultrasonic.log.TimberOkHttpLogger
+import org.moire.ultrasonic.service.ApiCallResponseChecker
 import org.moire.ultrasonic.service.CachedMusicService
 import org.moire.ultrasonic.service.MusicService
 import org.moire.ultrasonic.service.OfflineMusicService
@@ -59,13 +60,14 @@ val musicServiceModule = module {
 
     single<HttpLoggingInterceptor.Logger> { TimberOkHttpLogger() }
     single { SubsonicAPIClient(get(), get()) }
+    single { ApiCallResponseChecker(get(), get()) }
 
     single<MusicService>(named(ONLINE_MUSIC_SERVICE)) {
-        CachedMusicService(RESTMusicService(get(), get()))
+        CachedMusicService(RESTMusicService(get(), get(), get(), get()))
     }
 
     single<MusicService>(named(OFFLINE_MUSIC_SERVICE)) {
-        OfflineMusicService(get(), get())
+        OfflineMusicService()
     }
 
     single { SubsonicImageLoader(androidContext(), get()) }

ultrasonic/src/main/kotlin/org/moire/ultrasonic/service/ApiCallResponseChecker.kt

@@ -0,0 +1,66 @@
+package org.moire.ultrasonic.service
+
+import java.io.IOException
+import org.moire.ultrasonic.api.subsonic.SubsonicAPIClient
+import org.moire.ultrasonic.api.subsonic.SubsonicAPIDefinition
+import org.moire.ultrasonic.api.subsonic.response.SubsonicResponse
+import org.moire.ultrasonic.data.ActiveServerProvider
+import retrofit2.Response
+import timber.log.Timber
+
+/**
+ * This call wraps Subsonic API calls so their results can be checked for errors, API version, etc
+ */
+class ApiCallResponseChecker(
+    private val subsonicAPIClient: SubsonicAPIClient,
+    private val activeServerProvider: ActiveServerProvider
+) {
+    /**
+     * Executes a Subsonic API call with response check
+     */
+    @Throws(SubsonicRESTException::class, IOException::class)
+    fun <T : Response<out SubsonicResponse>> callWithResponseCheck(
+        call: (SubsonicAPIDefinition) -> T
+    ): T {
+        // Check for API version when first contacting the server
+        if (activeServerProvider.getActiveServer().minimumApiVersion == null) {
+            try {
+                val response = subsonicAPIClient.api.ping().execute()
+                if (response?.body() != null) {
+                    val restApiVersion = response.body()!!.version.restApiVersion
+                    Timber.i("Server minimum API version set to %s", restApiVersion)
+                    activeServerProvider.setMinimumApiVersion(restApiVersion)
+                }
+            } catch (ignored: Exception) {
+                // This Ping is only used to get the API Version, if it fails, that's no problem.
+            }
+        }
+
+        // This call will be now executed with the correct API Version, so it shouldn't fail
+        val result = call.invoke(subsonicAPIClient.api)
+        checkResponseSuccessful(result)
+        return result
+    }
+
+    /**
+     * Creates Exceptions from the results returned by the Subsonic API
+     */
+    companion object {
+        @Throws(SubsonicRESTException::class, IOException::class)
+        fun checkResponseSuccessful(response: Response<out SubsonicResponse>) {
+            if (response.isSuccessful && response.body()!!.status === SubsonicResponse.Status.OK) {
+                return
+            }
+            if (!response.isSuccessful) {
+                throw IOException("Server error, code: " + response.code())
+            } else if (
+                response.body()!!.status === SubsonicResponse.Status.ERROR &&
+                response.body()!!.error != null
+            ) {
+                throw SubsonicRESTException(response.body()!!.error!!)
+            } else {
+                throw IOException("Failed to perform request: " + response.code())
+            }
+        }
+    }
+}