Allow access to resource identifiers for drawables

CHANGES_UNRELEASED.md

@@ -53,3 +53,17 @@ Use the template below to make assigning a version number during the release cut
 
         assert(feature.sectionsEnabled[HomeScreenSection.topSites] == true)
         ```
+
+### ⚠️ Breaking Changes ⚠️
+
+  - **Android only**: Accessing drawables has changed to give access to the resource identifier.
+    - Migration path to the old behaviour is:
+
+    ```kotlin
+    let drawable: Drawable = MyNimbus.features.exampleFeature.demoDrawable
+    ```
+
+    becomes:
+    ```kotlin
+    let drawable: Drawable = MyNimbus.features.exampleFeature.demoDrawable.resource
+    ```

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/FeatureVariables.kt

@@ -185,19 +185,19 @@ interface Variables {
      * N.B. the `key` and type `Image` should be listed in the experiment manifest. The
      * names of the drawable resources should also be listed.
      */
-    fun getDrawable(key: String): Drawable? = null
+    fun getDrawable(key: String): Res<Drawable>? = null
 
     /**
      * Uses `getStringList(key: String)` to get a list of strings, then coerces the
      * strings in the list into Drawables. Values that cannot be coerced are omitted.
      */
-    fun getDrawableList(key: String): List<Drawable>? = null
+    fun getDrawableList(key: String): List<Res<Drawable>>? = null
 
     /**
      * Uses `getStringList(key: String)` to get a list of strings, then coerces the
      * values into Drawables. Values that cannot be coerced are omitted.
      */
-    fun getDrawableMap(key: String): Map<String, Drawable>? = null
+    fun getDrawableMap(key: String): Map<String, Res<Drawable>>? = null
 
     // Get a child configuration object.
 
@@ -271,15 +271,15 @@ interface Variables {
     /**
      * Synonym for [getDrawable(key: String)], for easier code generation.
      */
-    fun getImage(key: String): Drawable? = getDrawable(key)
+    fun getImage(key: String): Res<Drawable>? = getDrawable(key)
     /**
      * Synonym for [getDrawableList(key: String)], for easier code generation.
      */
-    fun getImageList(key: String): List<Drawable>? = getDrawableList(key)
+    fun getImageList(key: String): List<Res<Drawable>>? = getDrawableList(key)
     /**
      * Synonym for [getDrawableMap(key: String)], for easier code generation.
      */
-    fun getImageMap(key: String): Map<String, Drawable>? = getDrawableMap(key)
+    fun getImageMap(key: String): Map<String, Res<Drawable>>? = getDrawableMap(key)
 }
 
 inline fun <reified T : Enum<T>> String.asEnum(): T? = try {
@@ -367,13 +367,13 @@ interface VariablesWithContext : Variables {
     override fun getTextMap(key: String): Map<String, String>? = getStringMap(key)?.mapValues(this::asText)
     override fun getDrawable(key: String) = getDrawableResource(key)?.let(this::asDrawable)
     override fun getDrawableList(key: String) = getStringList(key)?.mapNotNull(this::asDrawableResource)?.mapNotNull(this::asDrawable)
-    override fun getDrawableMap(key: String): Map<String, Drawable>? = getStringMap(key)?.mapValues(this::asDrawableResource)?.mapValues(this::asDrawable)
+    override fun getDrawableMap(key: String) = getStringMap(key)?.mapValues(this::asDrawableResource)?.mapValues(this::asDrawable)
 
     // These `as*` methods become useful when transforming values found in JSON to actual values
     // the app will use. They're broken out here so they can be re-used by codegen generating
     // defaults from manifest information.
     fun asText(res: Int) = context.getString(res)
-    fun asDrawable(res: Int) = context.getDrawable(res)
+    fun asDrawable(res: Int): Res<Drawable> = DrawableRes(context, res)
     fun asText(string: String): String? = asStringResource(string)?.let(this::asText) ?: string
     fun asStringResource(string: String) = context.getResource(string, "string")
     fun asDrawableResource(string: String) = context.getResource(string, "drawable")
@@ -465,3 +465,35 @@ private inline fun <reified T> JSONObject.asMap(): Map<String, T>? {
 
 // Another implementation of `Variables` may just return null for everything.
 class NullVariables(override val context: Context) : Variables
+
+/**
+ * Accessor object to allow callers access to the resource identifier as well as the
+ * convenience of getting the underlying resource.
+ *
+ * It is intended as a uniform way of accessing different resource types
+ * bundled with the app, through Nimbus.
+ */
+interface Res<T> {
+    /**
+     * The resource identifier
+     */
+    val resourceId: Int
+
+    /**
+     * The actual resource.
+     */
+    val resource: T
+
+    companion object {
+        fun drawable(context: Context, resId: Int): Res<Drawable> =
+            DrawableRes(context, resId)
+    }
+}
+
+internal class DrawableRes(
+    private val context: Context,
+    override val resourceId: Int
+) : Res<Drawable> {
+    override val resource: Drawable
+        get() = context.resources.getDrawable(resourceId, context.theme)
+}

components/support/nimbus-fml/fixtures/android/runtime/Context.kt

@@ -16,6 +16,8 @@ class Context {
 
     val packageName = "dummy.package.name"
 
+    val theme = "a theme"
+
     fun getDrawable(res: Int): Drawable = Drawable(res)
 
     fun getString(res: Int): String = "res:$res"
@@ -24,4 +26,6 @@ class Context {
 @Suppress("UNUSED_PARAMETER", "PACKAGE_OR_CLASSIFIER_REDECLARATION", "FunctionOnlyReturningConstant")
 object Resources {
     fun getIdentifier(resName: String, defType: String, packageName: String): Int? = null
+
+    fun getDrawable(resId: Int, theme: String) = Drawable(resId)
 }

components/support/nimbus-fml/src/backends/kotlin/gen_structs/bundled.rs

@@ -67,6 +67,10 @@ impl CodeType for TextCodeType {
             _ => unreachable!("Expecting a string"),
         }
     }
+
+    fn imports(&self, _oracle: &dyn CodeOracle) -> Option<Vec<String>> {
+        Some(vec!["android.content.Context".to_string()])
+    }
 }
 
 pub(crate) struct ImageCodeType;
@@ -75,7 +79,7 @@ impl CodeType for ImageCodeType {
     /// The language specific label used to reference this type. This will be used in
     /// method signatures and property declarations.
     fn type_label(&self, _oracle: &dyn CodeOracle) -> String {
-        "Drawable".into()
+        "Res<Drawable>".into()
     }
 
     fn property_getter(
@@ -119,12 +123,19 @@ impl CodeType for ImageCodeType {
         match literal {
             serde_json::Value::String(v) => {
                 format!(
-                    r#"{context}.getDrawable(R.drawable.{id})"#,
+                    r#"Res.drawable({context}, R.drawable.{id})"#,
                     context = ctx,
                     id = v.to_snake_case()
                 )
             }
-            _ => unreachable!("Expecting a string"),
+            _ => unreachable!("Expecting a string matching an image/drawable resource"),
         }
     }
+
+    fn imports(&self, _oracle: &dyn CodeOracle) -> Option<Vec<String>> {
+        Some(vec![
+            "android.graphics.drawable.Drawable".to_string(),
+            "org.mozilla.experiments.nimbus.Res".to_string(),
+        ])
+    }
 }

components/support/nimbus-fml/src/backends/kotlin/gen_structs/feature.rs

@@ -42,6 +42,12 @@ impl CodeDeclaration for FeatureCodeDeclaration {
     fn definition_code(&self, _oracle: &dyn CodeOracle) -> Option<String> {
         Some(self.render().unwrap())
     }
+
+    fn imports(&self, _oracle: &dyn CodeOracle) -> Option<Vec<String>> {
+        Some(vec![
+            "org.mozilla.experiments.nimbus.internal.FeatureHolder".to_string(),
+        ])
+    }
 }
 
 impl LiteralRenderer for FeatureCodeDeclaration {

components/support/nimbus-fml/src/backends/kotlin/gen_structs/filters.rs

@@ -51,19 +51,20 @@ pub fn enum_variant_name(nm: &dyn fmt::Display) -> Result<String, askama::Error>
 pub fn comment(txt: &dyn fmt::Display, spaces: &str) -> Result<String, askama::Error> {
     use textwrap::{fill, Options};
 
-    let indent1 = "/** ".to_string();
-    let indent2 = format!("{} * ", spaces);
-    let indent3 = format!("{} */", spaces);
+    let indent_start = "/** ".to_string();
+    let indent_mid = format!("{} * ", spaces);
+    let indent_end = format!("{} */", spaces);
 
     let options = Options::new(80)
-        .initial_indent(&indent1)
-        .subsequent_indent(&indent2);
+        .initial_indent(&indent_mid)
+        .subsequent_indent(&indent_mid);
 
     let lines = fill(txt.to_string().as_str(), &options);
     Ok(format!(
-        "{lines}\n{indent}",
+        "{start}\n{lines}\n{indent}",
+        start = indent_start,
         lines = lines,
-        indent = indent3
+        indent = indent_end
     ))
 }
 

components/support/nimbus-fml/src/backends/kotlin/gen_structs/mod.rs

@@ -7,7 +7,7 @@ use std::collections::HashSet;
 
 use crate::{
     backends::{CodeDeclaration, CodeOracle, CodeType, TypeIdentifier},
-    intermediate_representation::{FeatureDef, FeatureManifest},
+    intermediate_representation::{FeatureDef, FeatureManifest, TypeFinder},
     Config,
 };
 
@@ -85,6 +85,18 @@ impl<'a> FeatureManifestDeclaration<'a> {
             .into_iter()
             .filter_map(|member| member.imports(oracle))
             .flatten()
+            .chain(
+                self.fm
+                    .all_types()
+                    .into_iter()
+                    .filter_map(|type_| self.oracle.find(&type_).imports(oracle))
+                    .flatten(),
+            )
+            .chain(vec![
+                "org.mozilla.experiments.nimbus.Variables".to_string(),
+                "org.mozilla.experiments.nimbus.FeaturesInterface".to_string(),
+                format!("{}.R", self.config.resource_package_name()),
+            ])
             .collect::<HashSet<String>>()
             .into_iter()
             .collect();

components/support/nimbus-fml/src/backends/kotlin/gen_structs/object.rs

@@ -115,6 +115,13 @@ impl CodeDeclaration for ObjectCodeDeclaration {
     fn definition_code(&self, _oracle: &dyn CodeOracle) -> Option<String> {
         Some(self.render().unwrap())
     }
+
+    fn imports(&self, _oracle: &dyn CodeOracle) -> Option<Vec<String>> {
+        Some(vec![
+            "org.mozilla.experiments.nimbus.NullVariables".to_string(),
+            "android.content.Context".to_string(),
+        ])
+    }
 }
 
 impl LiteralRenderer for ObjectCodeDeclaration {

components/support/nimbus-fml/src/backends/kotlin/gen_structs/structural.rs

@@ -251,6 +251,31 @@ impl CodeType for MapCodeType {
 
         format!("mapOf({})", src.join(", "))
     }
+
+    fn imports(&self, oracle: &dyn CodeOracle) -> Option<Vec<String>> {
+        let k_type = oracle.find(&self.k_type);
+        let v_type = oracle.find(&self.v_type);
+        let mapper = match (
+            k_type.create_transform(oracle),
+            v_type.create_transform(oracle),
+        ) {
+            (Some(_), Some(_)) => {
+                Some("org.mozilla.experiments.nimbus.internal.mapEntries".to_string())
+            }
+            (None, Some(_)) => {
+                Some("org.mozilla.experiments.nimbus.internal.mapValues".to_string())
+            }
+            (Some(_), None) => Some("org.mozilla.experiments.nimbus.internal.mapKeys".to_string()),
+            _ => None,
+        };
+
+        let merger = "org.mozilla.experiments.nimbus.internal.mergeWith".to_string();
+
+        Some(match mapper {
+            Some(mapper) => vec![mapper, merger],
+            _ => vec![merger],
+        })
+    }
 }
 
 // List type

components/support/nimbus-fml/src/backends/kotlin/templates/FeatureManifestTemplate.kt

@@ -4,27 +4,16 @@
 {%- match self.config.nimbus_package_name() %}
 {%- when Some with (package_name) %}
 package {{ package_name }}
-{%- else %}
-{%- endmatch %}
 
-import android.content.Context
-import android.graphics.drawable.Drawable
-import org.mozilla.experiments.nimbus.NullVariables
-import org.mozilla.experiments.nimbus.Variables
-import org.mozilla.experiments.nimbus.FeaturesInterface
-import org.mozilla.experiments.nimbus.internal.FeatureHolder
-import org.mozilla.experiments.nimbus.internal.mapValues
-import org.mozilla.experiments.nimbus.internal.mapKeys
-import org.mozilla.experiments.nimbus.internal.mapEntries
-import org.mozilla.experiments.nimbus.internal.mergeWith
+{% else -%}
+{% endmatch %}
 
 {%- for imported_class in self.imports() %}
 import {{ imported_class }}
 {%- endfor %}
 
-import {{ self.config.resource_package_name() }}.R
+{%- let nimbus_object = self.config.nimbus_object_name() %}
 
-{% let nimbus_object = self.config.nimbus_object_name() -%}
 /**
  * An object for safely accessing feature configuration from Nimbus.
  *
@@ -63,6 +52,10 @@ object {{ nimbus_object }} {
      */
     var api: FeaturesInterface? = null
 
+    /**
+     * Accessor object for generated configuration classes extracted from Nimbus, with built-in
+     * default values.
+     */
     val features = Features()
 }
 

components/support/nimbus-fml/src/backends/kotlin/templates/FeatureTemplate.kt

@@ -4,7 +4,7 @@
 {%- let raw_name = inner.name() %}
 
 {% let nimbus_object = self.nimbus_object_name() %}
-{% let class_name = inner.name()|class_name -%}
+{%- let class_name = inner.name()|class_name -%}
 
 {{ inner.doc()|comment("") }}
 public class {{class_name}}
@@ -23,7 +23,7 @@ specify all values needed for the  feature #}
 
     constructor(_variables: Variables, {% for p in inner.props() %}
     {%- let t = p.typ() %}
-    {{p.name()|var_name}}: {{ t|type_label }} = {{ t|literal(self, p.default(), "_variables.context") }}{% if !loop.last %},{% endif %}
+        {{p.name()|var_name}}: {{ t|type_label }} = {{ t|literal(self, p.default(), "_variables.context") }}{% if !loop.last %},{% endif %}
     {%- endfor %}
     ) : this(
         _variables = _variables,
@@ -40,5 +40,5 @@ specify all values needed for the  feature #}
         {%- let defaults = format!("_defaults.{}", prop_kt) %}
         {{ p.typ()|property(p.name(), "_variables", defaults)}}
     }
-    {%- endfor %}
+{% endfor %}
 }

components/support/nimbus-fml/src/backends/kotlin/templates/ObjectTemplate.kt

@@ -1,6 +1,7 @@
 {%- import "macros.kt" as kt %}
 {%- let inner = self.inner() %}
 {%- let raw_name = inner.name() %}
+
 {% let class_name = inner.name()|class_name -%}
 
 {{ inner.doc()|comment("") }}
@@ -18,7 +19,6 @@ specify all values needed for the  feature #}
 
 {#- A constructor for application to use.  #}
 
-
     constructor(
         _variables: Variables, {% for p in inner.props() %}
         {%- let t = p.typ() %}
@@ -32,6 +32,10 @@ specify all values needed for the  feature #}
     )
 
 {#- A constructor for application tests to use.  #}
+
+    /**
+     * A convenience constructor for testing/mocking purposes
+     */
     constructor(
         _context: Context, {% for p in inner.props() %}
         {%- let t = p.typ() %}
@@ -53,7 +57,7 @@ specify all values needed for the  feature #}
         {%- let defaults = format!("_defaults.{}", prop_kt) %}
         {{ p.typ()|property(p.name(), "_variables", defaults)}}
     }
-    {%- endfor %}
+{% endfor %}
 
     internal fun _mergeWith(defaults: {{class_name}}): {{class_name}} =
         {{class_name}}(_variables = this._variables, _defaults = defaults._defaults)