Add second activity to example app #37

miguelandres · 2016-10-20T21:24:38Z

MainActivity contains a two textviews, title and freeform text, whose
text is controlled by a String list Variable and a String Variable,
respectively.

Box activity contains a title text and a box. The box has a color
controlled by a Color list Variable and it is hidden or shown based on
a Boolean variable.

Both activities’ title text share a Range Variable that dictates their
size, this is meant to showcase Remixer keeping values across different
contexts, but this depends on #36

MainActivity contains a two textviews, title and freeform text, whose text is controlled by a String list Variable and a String Variable, respectively. Box activity contains a title text and a box. The box has a color controlled by a Color list Variable and it is hidden or shown based on a Boolean variable. Both activities’ title text share a Range Variable that dictates their size, this is meant to showcase Remixer keeping values across different contexts, but this depends on #36

pingpongboss · 2016-10-22T02:30:45Z

remixer_example/src/main/java/com/google/android/apps/remixer/BoxActivity.java

+ * explicit (not-annotation-based) API for Remixer.
+ *
+ * <p>While this is supported we think most people would prefer to use the clearer, less verbose
+ * annotation-based API.


nit: I'm not convinced that you need to create showcase a non-annotation-based example. In fact, I would argue that since you want to encourage clients to use the annotation-based version, you should just showcase that.

Plus, you can still ensure support for the explicit API via tests.

I'll fix that

pingpongboss · 2016-10-22T02:31:17Z

remixer_example/src/main/java/com/google/android/apps/remixer/BoxActivity.java

+  // The remixer instance
+  private Remixer remixer;
+
+


formatting and blank lines for these fields

pingpongboss · 2016-10-22T02:43:44Z

remixer_example/src/main/java/com/google/android/apps/remixer/MainActivity.java

-    remixer.addItem(booleanVariable.buildAndInit());
+  @RangeVariableMethod(
+      minValue = 16, maxValue = 72, increment = 4, title = "(Shared) Title font size")
+  void setTitleTextSize(Integer size) {


if I'm reading this right, then the key of an annotated method is the simple method name? And this is how you ensure this Variable is shared across 2 activities?

This is really clever, but it's a really difficult to discover API. This is also prone to errors: imagine if a client has two different Activities, each with an annotated setTextSize() (maybe they're lazy) but meant to target completely different widgets. All of a sudden, these 2 variables are linked due to having the same key/name, and it's probably a huge pain trying to debug why it's happening (without your help).

I would suggest changing it, perhaps to something more explicit. I have some ideas, but can you first enumerate the reasons why we have keys? I'll start us off:

Two RemixerItems can have the same key in order to share the current value. The constraint is that they must both share the same Type and their parent Activity must differ.

A RemixerItem's key is used to connect it to the corresponding remixer Widget so updates in the widget are correctly routed to the right callback. (is this true?) The constraint is that all RemixerItems under the same parent Activity must have different keys.

Anything else?

The key can be set explicitly, remember a while ago we were trying to minimize the amount of boilerplate a user needed to write, key was one of them, so it takes a sensible default.

I can still change it to something more explicit, and maybe remove auto key generation if you think it's that problematic.

Also you're correct.

I think you should definitely keep an eye out for whether this trips anyone up. And/or think about when an unintended key collision happens, how you can help the client debug. All of this can be done after this PR.

pingpongboss · 2016-10-22T02:51:01Z

remixer_example/src/main/java/com/google/android/apps/remixer/MenuActivity.java

+    adapter.add(getResources().getString(R.string.boxDemoName));
+    listView.setOnItemClickListener(new AdapterView.OnItemClickListener() {
+      @Override
+      public void onItemClick(AdapterView<?> adapterView, View view, int i, long l) {


please rename i and l. position and id

pingpongboss · 2016-10-22T02:52:09Z

remixer_example/src/main/java/com/google/android/apps/remixer/MenuActivity.java

+
+public class MenuActivity extends AppCompatActivity {
+
+  private ListView listView;


suggest being a good example (get it?) and using RecyclerView. Since this is in the example project, clients won't unnecessarily inherit the RV dependency.

pingpongboss · 2016-10-22T02:52:32Z

remixer_example/src/main/java/com/google/android/apps/remixer/MenuActivity.java

+        Intent intent = new Intent();
+        switch (i) {
+          case 0:
+            intent.setClass(getApplicationContext(), MainActivity.class);


noooo don't use the application context unnecessarily. Or did you do it on purpose?

pingpongboss

woohoo

pingpongboss · 2016-10-25T19:43:47Z

remixer_example/src/main/java/com/google/android/apps/remixer/BoxActivity.java

+  void setBoxColor(Integer color) {
+    box.setBackgroundColor(color);
+  }
+


blank spaces - taylor swift

HAHAHAHAHAHA

pingpongboss · 2016-10-25T19:47:09Z

remixer_example/src/main/java/com/google/android/apps/remixer/MainActivity.java

 import com.google.android.libraries.remixer.ui.gesture.Direction;
 import com.google.android.libraries.remixer.ui.view.RemixerFragment;

 /**
- * Main activity with explicit instantiation of Remixer Objects.
+ * Annotated version of the BoxActivity.


is this javadoc right?

pingpongboss · 2016-10-25T19:47:29Z

remixer_example/src/main/java/com/google/android/apps/remixer/MainActivity.java

-  private TextView freeformText;
-  // The remixer instance
-  private Remixer remixer;
+  TextView titleText;


are these package-private for a reason

pingpongboss · 2016-10-25T19:54:23Z

remixer_example/src/main/java/com/google/android/apps/remixer/MainActivity.java

-        .setLayoutId(R.layout.color_list_variable_widget);
-    remixer.addItem(colorVariable.buildAndInit());
+    remixerButton = (Button) findViewById(R.id.button);
+    RemixerBinder.bind(this);


not for this PR, but: could this call be replaced with an @annotation on the Activity?

It generally can't, this is because of the way Annotation processing works, all such frameworks use a similar method to tie things together :(

pingpongboss · 2016-10-25T19:56:16Z

remixer_example/src/main/java/com/google/android/apps/remixer/MainActivity.java

+  @RangeVariableMethod(
+      key = "titleTextSize",
+      minValue = 16, maxValue = 72, increment = 4, title = "(Shared) Title font size")
+  void setTitleTextSize(Integer size) {


Curious if this param can be replaced with int? It may be better as an example for this to be int as well.

No, it can't :/ Java is very dumb about unboxing templated stuff. :/

Or rather, I'm checking that the parameter is of a specific class, and since templates don't work with primitives and primitives aren't classes, it's mandatory to use the boxed version, I will investigate whether there is a way to compare to the unboxed type, but I don't think that's likely.

whaaat really? I think this deserves another look (later). There must be a way to support this.

I think this is pretty important, or devs who add @RemixerAnnotations to existing code will find it to be a major PITA.

Opened #40 to investigate

…nActivity

miguelandres assigned pingpongboss Oct 20, 2016

Fix wrong Remixer key on shared Variable

0ea8303

pingpongboss suggested changes Oct 22, 2016

View reviewed changes

Miguel Yanez Barreto added 2 commits October 25, 2016 15:15

Use RecyclerView instead of ListView in MenuActivity

2268b2f

Make BoxActivity use the Annotation-based API, improve keys.

eede4ac

pingpongboss approved these changes Oct 25, 2016

View reviewed changes

Clear blank spaces, fix documentation and fix access modifiers in Mai…

30de063

…nActivity

miguelandres merged commit 267802b into develop Oct 25, 2016

miguelandres deleted the demo_app branch October 25, 2016 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add second activity to example app #37

Add second activity to example app #37

miguelandres commented Oct 20, 2016

pingpongboss Oct 22, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 22, 2016

pingpongboss Oct 22, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 25, 2016

pingpongboss Oct 22, 2016

pingpongboss Oct 22, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 22, 2016

miguelandres Oct 25, 2016

pingpongboss left a comment

pingpongboss Oct 25, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 25, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 25, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 25, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 25, 2016

miguelandres Oct 25, 2016

miguelandres Oct 25, 2016

pingpongboss Oct 25, 2016

miguelandres Oct 25, 2016


		public class MenuActivity extends AppCompatActivity {

		private ListView listView;

Add second activity to example app #37

Add second activity to example app #37

Conversation

miguelandres commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pingpongboss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment