Merge branch 'fix_1460' into 'master'

Icon fixes

Closes #1686, #1772, #1771, and #1460

See merge request fdroid/fdroidclient!817

app/build.gradle

@@ -169,7 +169,8 @@ dependencies {
     testImplementation 'org.robolectric:robolectric:3.8'
     testImplementation "com.android.support.test:monitor:1.0.2"
     testImplementation 'org.bouncycastle:bcprov-jdk15on:1.60'
-    testImplementation 'junit:junit:4.12'
+    testImplementation 'junit:junit:4.13'
+    testImplementation 'org.hamcrest:hamcrest:2.2'
     testImplementation 'org.mockito:mockito-core:2.7.22'
 
     androidTestImplementation 'com.android.support:support-annotations:27.1.1'

app/src/full/java/org/fdroid/fdroid/nearby/LocalRepoManager.java

@@ -414,7 +414,7 @@ public final class LocalRepoManager {
             tag("lastupdated", app.lastUpdated);
             tag("name", app.name);
             tag("summary", app.summary);
-            tag("icon", app.icon);
+            tag("icon", app.iconFromApk);
             tag("desc", app.description);
             tag("license", "Unknown");
             tag("categories", "LocalRepo," + Preferences.get().getLocalRepoName());

app/src/full/java/org/fdroid/fdroid/nearby/SwapSuccessView.java

@@ -309,7 +309,7 @@ public class SwapSuccessView extends SwapView implements LoaderManager.LoaderCal
                     nameView.setText(app.name);
                 }
 
-                ImageLoader.getInstance().displayImage(app.iconUrl, iconView, Utils.getRepoAppDisplayImageOptions());
+                ImageLoader.getInstance().displayImage(app.getIconUrl(iconView.getContext()), iconView, Utils.getRepoAppDisplayImageOptions());
 
                 if (app.hasUpdates()) {
                     btnInstall.setText(R.string.menu_upgrade);

app/src/main/java/org/fdroid/fdroid/NotificationHelper.java

@@ -493,7 +493,7 @@ class NotificationHelper {
     private Bitmap getLargeIconForEntry(AppUpdateStatusManager.AppUpdateStatus entry) {
         final Point largeIconSize = getLargeIconSize();
         Bitmap iconLarge = null;
-        if (TextUtils.isEmpty(entry.app.iconUrl)) {
+        if (TextUtils.isEmpty(entry.app.getIconUrl(context))) {
             return null;
         } else if (entry.status == AppUpdateStatusManager.Status.Downloading
                 || entry.status == AppUpdateStatusManager.Status.Installing) {
@@ -505,12 +505,12 @@ class NotificationHelper {
                 downloadIcon.draw(canvas);
             }
             return bitmap;
-        } else if (DiskCacheUtils.findInCache(entry.app.iconUrl, ImageLoader.getInstance().getDiskCache()) != null) {
+        } else if (DiskCacheUtils.findInCache(entry.app.getIconUrl(context), ImageLoader.getInstance().getDiskCache()) != null) {
             iconLarge = ImageLoader.getInstance().loadImageSync(
-                    entry.app.iconUrl, new ImageSize(largeIconSize.x, largeIconSize.y));
+                    entry.app.getIconUrl(context), new ImageSize(largeIconSize.x, largeIconSize.y));
         } else {
             // Load it for later!
-            ImageLoader.getInstance().loadImage(entry.app.iconUrl, new ImageSize(largeIconSize.x, largeIconSize.y), new ImageLoadingListener() {
+            ImageLoader.getInstance().loadImage(entry.app.getIconUrl(context), new ImageSize(largeIconSize.x, largeIconSize.y), new ImageLoadingListener() {
 
                 AppUpdateStatusManager.AppUpdateStatus entry;
 
@@ -536,8 +536,8 @@ class NotificationHelper {
                     AppUpdateStatusManager.AppUpdateStatus oldEntry = appUpdateStatusManager.get(entry.getCanonicalUrl());
                     if (oldEntry != null
                             && oldEntry.app != null
-                            && oldEntry.app.iconUrl != null
+                            && oldEntry.app.getIconUrl(context) != null
-                            && DiskCacheUtils.findInCache(oldEntry.app.iconUrl, ImageLoader.getInstance().getDiskCache()) != null) {
+                            && DiskCacheUtils.findInCache(oldEntry.app.getIconUrl(context), ImageLoader.getInstance().getDiskCache()) != null) {
                         createNotification(oldEntry); // Update with new image!
                     }
                 }

app/src/main/java/org/fdroid/fdroid/Utils.java

@@ -49,12 +49,15 @@ import android.util.Log;
 import android.util.TypedValue;
 import android.view.View;
 import android.view.ViewTreeObserver;
+import android.widget.ImageView;
 import android.widget.Toast;
 import com.nostra13.universalimageloader.core.DisplayImageOptions;
+import com.nostra13.universalimageloader.core.ImageLoader;
 import com.nostra13.universalimageloader.core.assist.ImageScaleType;
 import com.nostra13.universalimageloader.core.display.FadeInBitmapDisplayer;
 import com.nostra13.universalimageloader.utils.StorageUtils;
 import org.fdroid.fdroid.compat.FileCompat;
+import org.fdroid.fdroid.data.App;
 import org.fdroid.fdroid.data.Repo;
 import org.fdroid.fdroid.data.SanitizedFile;
 import org.xml.sax.XMLReader;
@@ -486,6 +489,26 @@ public final class Utils {
         return repoAppDisplayImageOptions;
     }
 
+    /**
+     * If app has an iconUrl we feed that to UIL, otherwise we ask the PackageManager which will
+     * return the app's icon directly when the app is installed.
+     * We fall back to the placeholder icon otherwise.
+     */
+    public static void setIconFromRepoOrPM(@NonNull App app, ImageView iv, Context context) {
+        if (app.getIconUrl(iv.getContext()) == null) {
+            try {
+                iv.setImageDrawable(context.getPackageManager().getApplicationIcon(app.packageName));
+            } catch (PackageManager.NameNotFoundException e) {
+                DisplayImageOptions options = Utils.getRepoAppDisplayImageOptions();
+                iv.setImageDrawable(options.shouldShowImageForEmptyUri()
+                        ? options.getImageForEmptyUri(FDroidApp.getInstance().getResources())
+                        : null);
+            }
+        } else {
+            ImageLoader.getInstance().displayImage(app.getIconUrl(iv.getContext()), iv, Utils.getRepoAppDisplayImageOptions());
+        }
+    }
+
     // this is all new stuff being added
     public static String hashBytes(byte[] input, String algo) {
         try {

app/src/main/java/org/fdroid/fdroid/data/App.java

@@ -19,9 +19,11 @@ import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
 import android.text.TextUtils;
 import android.util.Log;
+
 import com.fasterxml.jackson.annotation.JacksonInject;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
+
 import org.apache.commons.io.filefilter.RegexFileFilter;
 import org.fdroid.fdroid.FDroidApp;
 import org.fdroid.fdroid.Preferences;
@@ -116,7 +118,8 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
     public String name = "Unknown";
 
     public String summary = "Unknown application";
-    public String icon;
+    @JsonProperty("icon")
+    public String iconFromApk;
 
     public String description;
 
@@ -215,9 +218,10 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
     public String[] requirements;
 
     /**
-     * URL to download the app's icon.
+     * URL to download the app's icon. (Set only from localized block, see also
+     * {@link #iconFromApk} and {@link #getIconUrl(Context)}
      */
-    public String iconUrl;
+    private String iconUrl;
 
     public static String getIconName(String packageName, int versionCode) {
         return packageName + "_" + versionCode + ".png";
@@ -258,7 +262,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
                     summary = cursor.getString(i);
                     break;
                 case Cols.ICON:
-                    icon = cursor.getString(i);
+                    iconFromApk = cursor.getString(i);
                     break;
                 case Cols.DESCRIPTION:
                     description = cursor.getString(i);
@@ -567,6 +571,10 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         if (!TextUtils.isEmpty(value)) {
             description = formatDescription(value);
         }
+        value = getLocalizedGraphicsEntry(localized, localesToUse, "icon");
+        if (!TextUtils.isEmpty(value)) {
+            iconUrl = value;
+        }
 
         featureGraphic = getLocalizedGraphicsEntry(localized, localesToUse, "featureGraphic");
         promoGraphic = getLocalizedGraphicsEntry(localized, localesToUse, "promoGraphic");
@@ -656,6 +664,28 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         return description.replace("\n", "<br>");
     }
 
+    public String getIconUrl(Context context) {
+        Repo repo = RepoProvider.Helper.findById(context, repoId);
+        if (TextUtils.isEmpty(iconUrl)) {
+            if (TextUtils.isEmpty(iconFromApk)){
+                return null;
+            }
+            if (iconFromApk.endsWith(".xml")){
+                // We cannot use xml ressources as icons. F-Droid server should not include them
+                // https://gitlab.com/fdroid/fdroidserver/issues/344
+                return null;
+            }
+            String iconsDir;
+            if (repo.version >= Repo.VERSION_DENSITY_SPECIFIC_ICONS) {
+                iconsDir = Utils.getIconsDir(context, 1.0);
+            } else {
+                iconsDir = Utils.FALLBACK_ICONS_DIR;
+            }
+            return repo.address + iconsDir + iconFromApk;
+        }
+        return repo.address + "/" + packageName + "/" + iconUrl;
+    }
+
     public String getFeatureGraphicUrl(Context context) {
         if (TextUtils.isEmpty(featureGraphic)) {
             return null;
@@ -756,7 +786,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
                 + ", last updated on " + this.lastUpdated + ")</p>";
 
         this.name = (String) appInfo.loadLabel(pm);
-        this.icon = getIconName(packageName, packageInfo.versionCode);
+        this.iconFromApk = getIconName(packageName, packageInfo.versionCode);
         this.installedVersionName = packageInfo.versionName;
         this.installedVersionCode = packageInfo.versionCode;
         this.compatible = true;
@@ -943,7 +973,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         values.put(Cols.NAME, name);
         values.put(Cols.REPO_ID, repoId);
         values.put(Cols.SUMMARY, summary);
-        values.put(Cols.ICON, icon);
+        values.put(Cols.ICON, iconFromApk);
         values.put(Cols.ICON_URL, iconUrl);
         values.put(Cols.DESCRIPTION, description);
         values.put(Cols.WHATSNEW, whatsNew);
@@ -1173,7 +1203,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         dest.writeString(this.name);
         dest.writeLong(this.repoId);
         dest.writeString(this.summary);
-        dest.writeString(this.icon);
+        dest.writeString(this.iconFromApk);
         dest.writeString(this.description);
         dest.writeString(this.whatsNew);
         dest.writeString(this.license);
@@ -1224,7 +1254,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         this.name = in.readString();
         this.repoId = in.readLong();
         this.summary = in.readString();
-        this.icon = in.readString();
+        this.iconFromApk = in.readString();
         this.description = in.readString();
         this.whatsNew = in.readString();
         this.license = in.readString();

app/src/main/java/org/fdroid/fdroid/data/AppProvider.java

@@ -1007,8 +1007,6 @@ public class AppProvider extends FDroidProvider {
         updatePreferredMetadata();
         updateCompatibleFlags();
         updateSuggestedFromUpstream(null);
-        updateSuggestedFromLatest(null);
-        updateIconUrls();
     }
 
     /**
@@ -1182,51 +1180,4 @@ public class AppProvider extends FDroidProvider {
 
         LoggingQuery.execSQL(db(), updateSql, args);
     }
-
-    private void updateIconUrls() {
-        final String appTable = getTableName();
-        final String iconsDir = Utils.getIconsDir(getContext(), 1.0);
-        String repoVersion = Integer.toString(Repo.VERSION_DENSITY_SPECIFIC_ICONS);
-        Utils.debugLog(TAG, "Updating icon paths for apps belonging to repos with version >= " + repoVersion);
-        Utils.debugLog(TAG, "Using icons dir '" + iconsDir + "'");
-        String query = getIconUpdateQuery(appTable);
-        final String[] params = {
-            repoVersion, iconsDir, Utils.FALLBACK_ICONS_DIR,
-        };
-        db().execSQL(query, params);
-    }
-
-    /**
-     * Returns a query which requires two parameters to be bound. These are (in order):
-     *  1) The repo version that introduced density specific icons
-     *  2) The dir to density specific icons for the current device.
-     */
-    private static String getIconUpdateQuery(String app) {
-
-        final String repo = RepoTable.NAME;
-
-        final String iconUrlQuery =
-                "SELECT " +
-
-                // Concatenate (using the "||" operator) the address, the
-                // icons directory (bound to the ? as the second parameter
-                // when executing the query) and the icon path.
-                "( " +
-                    repo + "." + RepoTable.Cols.ADDRESS +
-                    " || " +
-
-                    // If the repo has the relevant version, then use a more
-                    // intelligent icons dir, otherwise revert to the default
-                    // one
-                    " CASE WHEN " + repo + "." + RepoTable.Cols.VERSION + " >= ? THEN ? ELSE ? END " +
-
-                    " || " +
-                    app + "." + Cols.ICON +
-                ") " +
-                " FROM " +
-                repo + " WHERE " + repo + "." + RepoTable.Cols._ID + " = " + app + "." + Cols.REPO_ID;
-        return "UPDATE " + app + " SET "
-            + Cols.ICON_URL + " = ( " + iconUrlQuery + " )";
-    }
-
 }

app/src/main/java/org/fdroid/fdroid/data/DBHelper.java

@@ -227,7 +227,7 @@ public class DBHelper extends SQLiteOpenHelper {
             + "primary key(" + ApkAntiFeatureJoinTable.Cols.APK_ID + ", " + ApkAntiFeatureJoinTable.Cols.ANTI_FEATURE_ID + ") "
             + " );";
 
-    protected static final int DB_VERSION = 83;
+    protected static final int DB_VERSION = 84;
 
     private final Context context;
 
@@ -456,6 +456,15 @@ public class DBHelper extends SQLiteOpenHelper {
         addIsLocalized(db, oldVersion);
         addTranslation(db, oldVersion);
         switchRepoArchivePriorities(db, oldVersion);
+        deleteOldIconUrls(db, oldVersion);
+    }
+
+    private void deleteOldIconUrls(SQLiteDatabase db, int oldVersion) {
+        if (oldVersion >= 84) {
+            return;
+        }
+        Utils.debugLog(TAG, "Clearing iconUrl field to enable localized icons on next update");
+        db.execSQL("UPDATE " + AppMetadataTable.NAME + " SET " + AppMetadataTable.Cols.ICON_URL  + "= NULL");
     }
 
     private void switchRepoArchivePriorities(SQLiteDatabase db, int oldVersion) {

app/src/main/java/org/fdroid/fdroid/data/RepoXMLHandler.java

@@ -214,7 +214,7 @@ public class RepoXMLHandler extends DefaultHandler {
                     curapp.name = str;
                     break;
                 case "icon":
-                    curapp.icon = str;
+                    curapp.iconFromApk = str;
                     break;
                 case "description":
                     // This is the old-style description. We'll read it

app/src/main/java/org/fdroid/fdroid/privileged/views/InstallConfirmActivity.java

@@ -76,7 +76,7 @@ public class InstallConfirmActivity extends FragmentActivity implements OnCancel
         TabHost tabHost = (TabHost) findViewById(android.R.id.tabhost);
 
         appName.setText(app.name);
-        ImageLoader.getInstance().displayImage(app.iconUrl, appIcon,
+        ImageLoader.getInstance().displayImage(app.getIconUrl(this), appIcon,
                 Utils.getRepoAppDisplayImageOptions());
 
         tabHost.setup();

app/src/main/java/org/fdroid/fdroid/views/AppDetailsActivity.java

@@ -149,7 +149,7 @@ public class AppDetailsActivity extends AppCompatActivity
         DisplayImageOptions displayImageOptions = Utils.getRepoAppDisplayImageOptions();
         String featureGraphicUrl = app.getFeatureGraphicUrl(this);
         featureImage.loadImageAndDisplay(ImageLoader.getInstance(), displayImageOptions,
-                featureGraphicUrl, app.iconUrl);
+                featureGraphicUrl, app.getIconUrl(this));
     }
 
     private String getPackageNameFromIntent(Intent intent) {

app/src/main/java/org/fdroid/fdroid/views/AppDetailsRecyclerViewAdapter.java

@@ -480,7 +480,7 @@ public class AppDetailsRecyclerViewAdapter
         }
 
         public void bindModel() {
-            ImageLoader.getInstance().displayImage(app.iconUrl, iconView, Utils.getRepoAppDisplayImageOptions());
+            Utils.setIconFromRepoOrPM(app, iconView, iconView.getContext());
             titleView.setText(app.name);
             if (!TextUtils.isEmpty(app.authorName)) {
                 authorView.setText(context.getString(R.string.by_author_format, app.authorName));

app/src/main/java/org/fdroid/fdroid/views/apps/AppListItemController.java

@@ -191,18 +191,7 @@ public abstract class AppListItemController extends RecyclerView.ViewHolder {
 
         if (actionButton != null) actionButton.setEnabled(true);
 
-        if (app.iconUrl == null) {
+        Utils.setIconFromRepoOrPM(app, icon, activity);
-            try {
-                icon.setImageDrawable(activity.getPackageManager().getApplicationIcon(app.packageName));
-            } catch (PackageManager.NameNotFoundException e) {
-                DisplayImageOptions options = Utils.getRepoAppDisplayImageOptions();
-                icon.setImageDrawable(options.shouldShowImageForEmptyUri()
-                        ? options.getImageForEmptyUri(FDroidApp.getInstance().getResources())
-                        : null);
-            }
-        } else {
-            ImageLoader.getInstance().displayImage(app.iconUrl, icon, Utils.getRepoAppDisplayImageOptions());
-        }
 
         // Figures out the current install/update/download/etc status for the app we are viewing.
         // Then, asks the view to update itself to reflect this status.

app/src/main/java/org/fdroid/fdroid/views/categories/AppCardController.java

@@ -95,8 +95,7 @@ public class AppCardController extends RecyclerView.ViewHolder
                 newTag.setVisibility(View.GONE);
             }
         }
-
+        Utils.setIconFromRepoOrPM(app, icon, icon.getContext());
-        ImageLoader.getInstance().displayImage(app.iconUrl, icon, Utils.getRepoAppDisplayImageOptions());
     }
 
     private boolean isConsideredNew(@NonNull App app) {

app/src/main/java/org/fdroid/fdroid/views/categories/CategoryController.java

@@ -154,6 +154,8 @@ public class CategoryController extends RecyclerView.ViewHolder implements Loade
                             Schema.AppMetadataTable.Cols.Package.PACKAGE_NAME,
                             Schema.AppMetadataTable.Cols.SUMMARY,
                             Schema.AppMetadataTable.Cols.ICON_URL,
+                            Schema.AppMetadataTable.Cols.ICON,
+                            Schema.AppMetadataTable.Cols.REPO_ID,
                     },
                     null,
                     null,
@@ -167,7 +169,7 @@ public class CategoryController extends RecyclerView.ViewHolder implements Loade
         int topAppsId = currentCategory.hashCode();
         int countAllAppsId = topAppsId + 1;
 
-        // Anything other than these IDs indicates that the loader which just finished finished
+        // Anything other than these IDs indicates that the loader which just finished
         // is no longer the one this view holder is interested in, due to the user having
         // scrolled away already during the asynchronous query being run.
         if (loader.getId() == topAppsId) {

app/src/test/java/org/fdroid/fdroid/updater/AppIconsTest.java

@@ -1,6 +1,7 @@
 package org.fdroid.fdroid.updater;
 
 import android.content.ContentValues;
+
 import org.fdroid.fdroid.BuildConfig;
 import org.fdroid.fdroid.IndexUpdater;
 import org.fdroid.fdroid.data.App;
@@ -8,13 +9,14 @@ import org.fdroid.fdroid.data.AppProvider;
 import org.fdroid.fdroid.data.Repo;
 import org.fdroid.fdroid.data.RepoProvider;
 import org.fdroid.fdroid.data.Schema;
+import org.hamcrest.text.MatchesPattern;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
-import static org.junit.Assert.assertEquals;
+import static org.hamcrest.MatcherAssert.assertThat;
 
 /**
  * Check whether app icons are loaded from the correct repository. The repository with the
@@ -42,7 +44,7 @@ public class AppIconsTest extends MultiIndexUpdaterTest {
         updateMain();
         updateArchive();
 
-        assertIconUrl("https://f-droid.org/repo/icons/org.adaway.54.png");
+        assertIconUrl("^https://f-droid\\.org/repo/icons-[0-9]{3}/org\\.adaway\\.54\\.png$");
     }
 
     @Test
@@ -53,7 +55,7 @@ public class AppIconsTest extends MultiIndexUpdaterTest {
         updateMain();
         updateArchive();
 
-        assertIconUrl("https://f-droid.org/archive/icons/org.adaway.54.png");
+        assertIconUrl("^https://f-droid\\.org/archive/icons-[0-9]{3}/org\\.adaway\\.54.png$");
     }
 
     private void setRepoPriority(String repoUri, int priority) {
@@ -66,9 +68,10 @@ public class AppIconsTest extends MultiIndexUpdaterTest {
 
     private void assertIconUrl(String expectedUrl) {
         App app = AppProvider.Helper.findHighestPriorityMetadata(context.getContentResolver(),
-                "org.adaway", new String[]{Schema.AppMetadataTable.Cols.ICON_URL});
+                "org.adaway", new String[]{
-
+                        Schema.AppMetadataTable.Cols.ICON_URL,
-        assertEquals(app.iconUrl, expectedUrl);
+                        Schema.AppMetadataTable.Cols.ICON,
+                        Schema.AppMetadataTable.Cols.REPO_ID});
+        assertThat(app.getIconUrl(context), MatchesPattern.matchesPattern(expectedUrl));
     }
-
 }

app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java

@@ -27,6 +27,7 @@ import org.fdroid.fdroid.data.Repo;
 import org.fdroid.fdroid.data.RepoProvider;
 import org.fdroid.fdroid.data.RepoPushRequest;
 import org.fdroid.fdroid.data.RepoXMLHandlerTest;
+import org.fdroid.fdroid.data.Schema;
 import org.fdroid.fdroid.mock.RepoDetails;
 import org.junit.Before;
 import org.junit.Test;
@@ -143,6 +144,14 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
         assertTrue(requestedPermissions.contains(android.Manifest.permission.READ_EXTERNAL_STORAGE));
         assertTrue(requestedPermissions.contains(android.Manifest.permission.WRITE_EXTERNAL_STORAGE));
         assertFalse(requestedPermissions.contains(android.Manifest.permission.READ_CALENDAR));
+        App app = AppProvider.Helper.findHighestPriorityMetadata(context.getContentResolver(),
+                "com.autonavi.minimap", new String[]{
+                        Schema.AppMetadataTable.Cols.ICON_URL,
+                        Schema.AppMetadataTable.Cols.ICON,
+                        Schema.AppMetadataTable.Cols.REPO_ID,
+                        Schema.AppMetadataTable.Cols.Package.PACKAGE_NAME});
+        assertEquals("localized icon takes precedence", TESTY_CANONICAL_URL + "/"
+                + app.packageName +  "/en-US/icon.png", app.getIconUrl(context));
     }
 
     @Test(expected = IndexUpdater.SigningException.class)
@@ -312,7 +321,7 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
                 "donate",
                 "featureGraphic",
                 "flattrID",
-                "icon",
+                "iconFromApk",
                 "iconUrl",
                 "issueTracker",
                 "lastUpdated",