diff --git a/app/src/main/java/org/thoughtcrime/securesms/logsubmit/SubmitLogFragment.java b/app/src/main/java/org/thoughtcrime/securesms/logsubmit/SubmitLogFragment.java index 4e2d82f334..36d1db1777 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/logsubmit/SubmitLogFragment.java +++ b/app/src/main/java/org/thoughtcrime/securesms/logsubmit/SubmitLogFragment.java @@ -78,7 +78,6 @@ import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.Comparator; import java.util.Iterator; import java.util.LinkedList; import java.util.List; @@ -630,21 +629,32 @@ public class SubmitLogFragment extends Fragment { private static CharSequence buildFlags() { StringBuilder out = new StringBuilder(); - Map remote = FeatureFlags.getRemoteValues(); + Map memory = FeatureFlags.getMemoryValues(); + Map disk = FeatureFlags.getDiskValues(); Map forced = FeatureFlags.getForcedValues(); - int remoteLength = Stream.of(remote.keySet()).map(String::length).max(Integer::compareTo).orElse(0); - int forcedLength = Stream.of(forced.keySet()).map(String::length).max(Integer::compareTo).orElse(0); + int remoteLength = Stream.of(memory.keySet()).map(String::length).max(Integer::compareTo).orElse(0); + int forcedLength = Stream.of(disk.keySet()).map(String::length).max(Integer::compareTo).orElse(0); - out.append("-- Remote\n"); - for (Map.Entry entry : remote.entrySet()) { + out.append("-- Memory\n"); + for (Map.Entry entry : memory.entrySet()) { out.append(Util.rightPad(entry.getKey(), remoteLength)).append(": ").append(entry.getValue()).append("\n"); } out.append("\n"); - out.append("-- Forced\n"); - for (Map.Entry entry : forced.entrySet()) { + out.append("-- Disk\n"); + for (Map.Entry entry : disk.entrySet()) { out.append(Util.rightPad(entry.getKey(), forcedLength)).append(": ").append(entry.getValue()).append("\n"); } + out.append("\n"); + + out.append("-- Forced\n"); + if (forced.isEmpty()) { + out.append("None\n"); + } else { + for (Map.Entry entry : forced.entrySet()) { + out.append(Util.rightPad(entry.getKey(), forcedLength)).append(": ").append(entry.getValue()).append("\n"); + } + } return out; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index 9b9f732608..9963f7e851 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -31,9 +31,9 @@ import java.util.concurrent.TimeUnit; * - Create a new string constant using {@link #generateKey(String)}) * - Add a method to retrieve the value using {@link #getValue(String, boolean)}. You can also add * other checks here, like requiring other flags. - * - If you would like to force a value for testing, place an entry in {@link #FORCED_VALUES}. When - * launching a feature that is planned to be updated via a remote config, do not forget to - * remove the entry! + * - If you want to be able to change a flag remotely, place it in {@link #REMOTE_CAPABLE}. + * - If you would like to force a value for testing, place an entry in {@link #FORCED_VALUES}. + * Do not commit changes to this map! * * Other interesting things you can do: * - Make a flag {@link #HOT_SWAPPABLE} @@ -55,16 +55,21 @@ public final class FeatureFlags { private static final String PINS_MEGAPHONE_KILL_SWITCH = generateKey("pinsMegaphoneKillSwitch"); /** - * Values in this map will take precedence over any value. If you do not wish to have any sort of - * override, simply don't put a value in this map. You should never commit additions to this map - * for flags that you plan on updating remotely. + * We will only store remote values for flags in this set. If you want a flag to be controllable + * remotely, place it in here. */ + private static final Set REMOTE_CAPABLE = Sets.newHashSet( + PINS_MEGAPHONE_KILL_SWITCH + ); + + /** + * Values in this map will take precedence over any value. This should only be used for local + * development. Given that you specify a default when retrieving a value, and that we only store + * remote values for things in {@link #REMOTE_CAPABLE}, there should be no need to ever *commit* + * an addition to this map. + */ + @SuppressWarnings("MismatchedQueryAndUpdateOfCollection") private static final Map FORCED_VALUES = new HashMap() {{ - put(UUIDS, false); - put(PROFILE_DISPLAY, false); - put(MESSAGE_REQUESTS, false); - put(USERNAMES, false); - put(STORAGE_SERVICE, false); }}; /** @@ -108,7 +113,7 @@ public final class FeatureFlags { public static synchronized void update(@NonNull Map config) { Map memory = REMOTE_VALUES; Map disk = parseStoredConfig(); - UpdateResult result = updateInternal(config, memory, disk, HOT_SWAPPABLE, STICKY); + UpdateResult result = updateInternal(config, memory, disk, REMOTE_CAPABLE, HOT_SWAPPABLE, STICKY); SignalStore.setRemoteConfig(mapToJson(result.getDisk()).toString()); REMOTE_VALUES.clear(); @@ -158,10 +163,15 @@ public final class FeatureFlags { } /** Only for rendering debug info. */ - public static synchronized @NonNull Map getRemoteValues() { + public static synchronized @NonNull Map getMemoryValues() { return new TreeMap<>(REMOTE_VALUES); } + /** Only for rendering debug info. */ + public static synchronized @NonNull Map getDiskValues() { + return new TreeMap<>(parseStoredConfig()); + } + /** Only for rendering debug info. */ public static synchronized @NonNull Map getForcedValues() { return new TreeMap<>(FORCED_VALUES); @@ -171,6 +181,7 @@ public final class FeatureFlags { static @NonNull UpdateResult updateInternal(@NonNull Map remote, @NonNull Map localMemory, @NonNull Map localDisk, + @NonNull Set remoteCapable, @NonNull Set hotSwap, @NonNull Set sticky) { @@ -183,7 +194,7 @@ public final class FeatureFlags { allKeys.addAll(localMemory.keySet()); Stream.of(allKeys) - .filter(k -> k.startsWith(PREFIX)) + .filter(remoteCapable::contains) .forEach(key -> { Boolean remoteValue = remote.get(key); Boolean diskValue = localDisk.get(key); diff --git a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java index d3761329ec..6a5014ace9 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/util/FeatureFlagsTest.java @@ -17,15 +17,17 @@ public class FeatureFlagsTest { private static final String B = key("b"); @Test - public void updateInternal_newValue_ignoreMissingPrefix() { - UpdateResult result = FeatureFlags.updateInternal(mapOf("noprefix", true), + public void updateInternal_newValue_ignoreNotInRemoteCapable() { + UpdateResult result = FeatureFlags.updateInternal(mapOf("A", true, + "B", true), mapOf(), mapOf(), + setOf("A"), setOf(), setOf()); assertEquals(mapOf(), result.getMemory()); - assertEquals(mapOf(), result.getDisk()); + assertEquals(mapOf("A", true), result.getDisk()); } @Test @@ -33,6 +35,7 @@ public class FeatureFlagsTest { UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), mapOf(), mapOf(), + setOf(A), setOf(), setOf()); @@ -46,6 +49,7 @@ public class FeatureFlagsTest { mapOf(), mapOf(), setOf(A), + setOf(A), setOf()); assertEquals(mapOf(A, true), result.getMemory()); @@ -57,6 +61,7 @@ public class FeatureFlagsTest { UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), mapOf(), mapOf(), + setOf(A), setOf(), setOf(A)); @@ -70,6 +75,7 @@ public class FeatureFlagsTest { mapOf(), mapOf(), setOf(A), + setOf(A), setOf(A)); assertEquals(mapOf(A, true), result.getMemory()); @@ -79,8 +85,9 @@ public class FeatureFlagsTest { @Test public void updateInternal_replaceValue() { UpdateResult result = FeatureFlags.updateInternal(mapOf(A, true), - mapOf(A, false), - mapOf(A, false), + mapOf(A, false), + mapOf(A, false), + setOf(A), setOf(), setOf()); @@ -94,6 +101,7 @@ public class FeatureFlagsTest { mapOf(A, false), mapOf(A, false), setOf(A), + setOf(A), setOf()); assertEquals(mapOf(A, true), result.getMemory()); @@ -106,6 +114,7 @@ public class FeatureFlagsTest { mapOf(A, false), mapOf(A, false), setOf(A), + setOf(A), setOf(A)); assertEquals(mapOf(A, true), result.getMemory()); @@ -118,6 +127,7 @@ public class FeatureFlagsTest { mapOf(A, true), mapOf(A, true), setOf(A), + setOf(A), setOf(A)); assertEquals(mapOf(A, true), result.getMemory()); @@ -127,8 +137,9 @@ public class FeatureFlagsTest { @Test public void updateInternal_removeValue() { UpdateResult result = FeatureFlags.updateInternal(mapOf(), - mapOf(A, true), mapOf(A, true), + mapOf(A, true), + setOf(A), setOf(), setOf()); @@ -142,6 +153,7 @@ public class FeatureFlagsTest { mapOf(A, true), mapOf(A, true), setOf(A), + setOf(A), setOf()); assertEquals(mapOf(), result.getMemory()); @@ -151,8 +163,9 @@ public class FeatureFlagsTest { @Test public void updateInternal_removeValue_stickyAlreadyEnabled() { UpdateResult result = FeatureFlags.updateInternal(mapOf(), - mapOf(A, true), mapOf(A, true), + mapOf(A, true), + setOf(A), setOf(), setOf(A)); @@ -165,6 +178,7 @@ public class FeatureFlagsTest { UpdateResult result = FeatureFlags.updateInternal(mapOf(), mapOf(A, false), mapOf(A, false), + setOf(A), setOf(), setOf(A)); @@ -178,6 +192,7 @@ public class FeatureFlagsTest { mapOf(A, true), mapOf(A, true), setOf(A), + setOf(A), setOf(A)); assertEquals(mapOf(A, true), result.getMemory()); @@ -190,6 +205,7 @@ public class FeatureFlagsTest { mapOf(A, false), mapOf(A, false), setOf(A), + setOf(A), setOf(A)); assertEquals(mapOf(), result.getMemory()); @@ -202,6 +218,7 @@ public class FeatureFlagsTest { B, false), mapOf(), mapOf(), + setOf(A, B), setOf(), setOf()); @@ -217,6 +234,7 @@ public class FeatureFlagsTest { B, true), mapOf(A, true, B, true), + setOf(A, B), setOf(), setOf());