From a452060b41aa141058963836027a063970ea35c1 Mon Sep 17 00:00:00 2001
From: Torsten Grote <t@grobox.de>
Date: Mon, 1 Aug 2016 18:04:17 -0300
Subject: [PATCH] Address issues found in code review

---
 .../res/layout/activity_dev_report.xml        | 202 ++++++++----------
 briar-android/res/values/strings.xml          |  13 +-
 .../android/report/DevReportActivity.java     |  26 ++-
 3 files changed, 121 insertions(+), 120 deletions(-)

diff --git a/briar-android/res/layout/activity_dev_report.xml b/briar-android/res/layout/activity_dev_report.xml
index 7f0a304d6..cdc892c9e 100644
--- a/briar-android/res/layout/activity_dev_report.xml
+++ b/briar-android/res/layout/activity_dev_report.xml
@@ -19,183 +19,171 @@
 
 		<RelativeLayout
 			android:layout_width="match_parent"
-			android:layout_height="match_parent">
+			android:layout_height="match_parent"
+			android:layout_marginEnd="@dimen/margin_large"
+			android:layout_marginLeft="@dimen/margin_large"
+			android:layout_marginRight="@dimen/margin_large"
+			android:layout_marginStart="@dimen/margin_large">
 
-			<LinearLayout
+			<android.support.design.widget.TextInputLayout
+				android:id="@+id/user_comment_layout"
 				android:layout_width="match_parent"
-				android:layout_height="wrap_content"
-				android:orientation="vertical"
-				android:paddingTop="@dimen/margin_medium">
+				android:layout_height="wrap_content">
 
-				<EditText
+				<android.support.design.widget.TextInputEditText
 					android:id="@+id/user_comment"
 					android:layout_width="match_parent"
 					android:layout_height="wrap_content"
-					android:layout_marginEnd="@dimen/margin_large"
-					android:layout_marginLeft="@dimen/margin_large"
-					android:layout_marginRight="@dimen/margin_large"
-					android:layout_marginStart="@dimen/margin_large"
 					android:inputType="textMultiLine|textCapSentences"
 					tools:hint="@string/describe_crash"/>
 
-				<EditText
+			</android.support.design.widget.TextInputLayout>
+
+			<android.support.design.widget.TextInputLayout
+				android:id="@+id/user_email_layout"
+				android:layout_width="match_parent"
+				android:layout_height="wrap_content"
+				android:layout_below="@+id/user_comment_layout"
+				android:layout_marginTop="@dimen/margin_small">
+
+				<android.support.design.widget.TextInputEditText
 					android:id="@+id/user_email"
 					android:layout_width="match_parent"
 					android:layout_height="wrap_content"
-					android:layout_marginEnd="@dimen/margin_large"
-					android:layout_marginLeft="@dimen/margin_large"
-					android:layout_marginRight="@dimen/margin_large"
-					android:layout_marginStart="@dimen/margin_large"
-					android:layout_marginTop="@dimen/margin_small"
 					android:hint="@string/optional_contact_email"
 					android:inputType="textEmailAddress"
 					android:maxLines="1"/>
 
+			</android.support.design.widget.TextInputLayout>
+
+			<CheckBox
+				android:id="@+id/include_debug_report"
+				android:layout_width="wrap_content"
+				android:layout_height="wrap_content"
+				android:layout_alignParentLeft="true"
+				android:layout_below="@+id/user_email_layout"
+				android:layout_marginTop="@dimen/margin_small"
+				android:layout_toLeftOf="@+id/chevron"
+				android:checked="false"
+				android:text="@string/include_debug_report_crash"/>
+
+			<Button
+				android:id="@+id/chevron"
+				style="@style/BriarButtonFlat.Positive"
+				android:layout_width="wrap_content"
+				android:layout_height="wrap_content"
+				android:layout_alignParentRight="true"
+				android:layout_alignTop="@+id/include_debug_report"
+				android:text="@string/show"/>
+
+			<ScrollView
+				android:layout_width="match_parent"
+				android:layout_height="wrap_content"
+				android:layout_below="@id/include_debug_report"
+				android:layout_marginTop="@dimen/margin_small">
+
 				<LinearLayout
+					android:id="@+id/report_content"
 					android:layout_width="match_parent"
 					android:layout_height="wrap_content"
-					android:layout_marginEnd="@dimen/margin_large"
-					android:layout_marginLeft="@dimen/margin_large"
-					android:layout_marginRight="@dimen/margin_large"
-					android:layout_marginStart="@dimen/margin_large"
-					android:layout_marginTop="@dimen/margin_small"
-					android:gravity="center_vertical"
-					android:orientation="horizontal">
+					android:gravity="center_horizontal"
+					android:orientation="vertical"
+					android:paddingBottom="@dimen/listitem_height_one_line_avatar"
+					android:paddingEnd="@dimen/margin_large"
+					android:paddingLeft="@dimen/margin_large"
+					android:paddingRight="@dimen/margin_large"
+					android:paddingStart="@dimen/margin_large"
+					android:paddingTop="@dimen/margin_small"
+					android:visibility="gone"/>
 
-					<TextView
-						android:id="@+id/debug_report"
-						android:layout_width="0dp"
-						android:layout_height="wrap_content"
-						android:layout_weight="1"
-						android:text="@string/debug_report"
-						android:textColor="@color/briar_text_primary"/>
-
-					<CheckBox
-						android:id="@+id/include_debug_report"
-						android:layout_width="0dp"
-						android:layout_height="wrap_content"
-						android:layout_weight="1"
-						android:checked="false"
-						android:text="@string/include_debug_report"
-						android:visibility="gone"/>
-
-					<ImageView
-						android:id="@+id/chevron"
-						android:layout_width="wrap_content"
-						android:layout_height="wrap_content"
-						android:layout_marginLeft="@dimen/margin_large"
-						android:layout_marginStart="@dimen/margin_large"
-						android:clickable="true"
-						android:src="@drawable/selector_chevron"
-						android:tint="@color/briar_button_positive"/>
-
-				</LinearLayout>
-
-				<ScrollView
-					android:layout_width="match_parent"
-					android:layout_height="wrap_content"
-					android:layout_marginTop="@dimen/margin_small">
-
-					<LinearLayout
-						android:id="@+id/report_content"
-						android:layout_width="match_parent"
-						android:layout_height="wrap_content"
-						android:gravity="center_horizontal"
-						android:orientation="vertical"
-						android:paddingBottom="@dimen/listitem_height_one_line_avatar"
-						android:paddingEnd="@dimen/margin_large"
-						android:paddingLeft="@dimen/margin_large"
-						android:paddingRight="@dimen/margin_large"
-						android:paddingStart="@dimen/margin_large"
-						android:paddingTop="@dimen/margin_small"
-						android:visibility="gone"/>
-
-				</ScrollView>
-			</LinearLayout>
+			</ScrollView>
 
 			<ProgressBar
 				android:id="@+id/progress_wheel"
 				style="?android:attr/progressBarStyleLarge"
 				android:layout_width="wrap_content"
 				android:layout_height="wrap_content"
-				android:layout_centerInParent="true"
+				android:layout_alignParentBottom="true"
+				android:layout_below="@+id/include_debug_report"
+				android:layout_centerHorizontal="true"
 				android:indeterminate="true"
 				android:visibility="gone"/>
 
 		</RelativeLayout>
 	</LinearLayout>
 
-	<LinearLayout
+	<RelativeLayout
 		android:id="@+id/request_report"
 		android:layout_width="match_parent"
 		android:layout_height="match_parent"
-		android:background="@android:color/background_light"
+		android:background="@color/window_background"
 		android:clickable="true"
 		android:gravity="center"
-		android:orientation="vertical"
 		android:padding="@dimen/margin_large"
 		android:visibility="gone">
 
 		<TextView
-			android:layout_width="match_parent"
-			android:layout_height="wrap_content"
-			android:gravity="center"
-			android:text="@string/sad_face"
-			android:textColor="@color/briar_text_primary"
-			android:textSize="@dimen/text_size_xlarge"/>
-
-		<TextView
+			android:id="@+id/crashed"
 			android:layout_width="match_parent"
 			android:layout_height="wrap_content"
 			android:layout_marginTop="@dimen/margin_large"
 			android:gravity="center"
 			android:text="@string/briar_crashed"
-			android:textColor="@color/briar_text_primary"
+			android:textColor="@color/briar_text_secondary"
 			android:textSize="@dimen/text_size_large"/>
 
 		<TextView
+			android:id="@+id/fault"
 			android:layout_width="match_parent"
 			android:layout_height="wrap_content"
+			android:layout_below="@+id/crashed"
 			android:layout_marginTop="@dimen/margin_large"
 			android:gravity="center"
 			android:text="@string/not_your_fault"
-			android:textColor="@color/briar_text_secondary"/>
+			android:textColor="@color/briar_text_secondary"
+			android:textSize="@dimen/text_size_large"/>
 
 		<TextView
+			android:id="@+id/pleaseSend"
 			android:layout_width="match_parent"
 			android:layout_height="wrap_content"
+			android:layout_below="@+id/fault"
 			android:layout_marginTop="@dimen/margin_large"
 			android:gravity="center"
 			android:text="@string/please_send_report"
-			android:textColor="@color/briar_text_secondary"/>
+			android:textColor="@color/briar_text_secondary"
+			android:textSize="@dimen/text_size_large"/>
 
 		<TextView
+			android:id="@+id/encrypted"
 			android:layout_width="match_parent"
 			android:layout_height="wrap_content"
+			android:layout_below="@+id/pleaseSend"
+			android:layout_marginBottom="@dimen/margin_large"
 			android:layout_marginTop="@dimen/margin_large"
 			android:gravity="center"
 			android:text="@string/report_is_encrypted"
-			android:textColor="@color/briar_text_secondary"/>
+			android:textColor="@color/briar_text_secondary"
+			android:textSize="@dimen/text_size_large"/>
 
-		<LinearLayout
-			android:layout_width="match_parent"
+		<Button
+			android:id="@+id/declineButton"
+			style="@style/BriarButtonFlat.Negative"
+			android:layout_width="wrap_content"
 			android:layout_height="wrap_content"
-			android:layout_marginTop="@dimen/margin_large"
-			android:gravity="center_horizontal">
+			android:layout_alignParentLeft="true"
+			android:layout_below="@+id/encrypted"
+			android:text="@string/close"/>
 
-			<Button
-				android:id="@+id/declineButton"
-				style="@style/BriarButton.Default"
-				android:layout_width="wrap_content"
-				android:layout_height="wrap_content"
-				android:text="@string/dialog_button_decline"/>
+		<Button
+			android:id="@+id/acceptButton"
+			style="@style/BriarButtonFlat.Positive"
+			android:layout_width="wrap_content"
+			android:layout_height="wrap_content"
+			android:layout_alignParentRight="true"
+			android:layout_below="@+id/encrypted"
+			android:text="@string/send_report"/>
+
+	</RelativeLayout>
 
-			<Button
-				android:id="@+id/acceptButton"
-				style="@style/BriarButton.Default"
-				android:layout_width="wrap_content"
-				android:layout_height="wrap_content"
-				android:text="@string/dialog_button_accept"/>
-		</LinearLayout>
-	</LinearLayout>
 </FrameLayout>
\ No newline at end of file
diff --git a/briar-android/res/values/strings.xml b/briar-android/res/values/strings.xml
index c28639ac0..c80a5d7b7 100644
--- a/briar-android/res/values/strings.xml
+++ b/briar-android/res/values/strings.xml
@@ -3,21 +3,25 @@
 	<string name="nav_drawer_open_description">Open the navigation drawer</string>
 	<string name="nav_drawer_close_description">Close the navigation drawer</string>
 	<string name="app_name">Briar</string>
+
+	<!-- Crash Reporter -->
 	<string name="crash_report_title">Briar Crash Report</string>
-	<string name="sad_face" translatable="false">:-(</string>
 	<string name="briar_crashed">Sorry, Briar has crashed.</string>
 	<string name="not_your_fault">This is not your fault.</string>
 	<string name="please_send_report">Please help us build a better Briar by sending us a crash report.</string>
 	<string name="report_is_encrypted">We promise that the report is encrypted and sent securely.</string>
 	<string name="feedback_title">Feedback</string>
-	<string name="describe_crash">Describe what happened</string>
+	<string name="describe_crash">Describe what happened (optional)</string>
 	<string name="enter_feedback">Enter your feedback</string>
-	<string name="optional_contact_email">Optional contact email</string>
+	<string name="optional_contact_email">Your email address (optional)</string>
 	<string name="debug_report">Debug report:</string>
-	<string name="include_debug_report">Include debug report?</string>
+	<string name="include_debug_report_crash">Include anonymous data about the crash</string>
+	<string name="include_debug_report_feedback">Include anonymous data about this device</string>
 	<string name="could_not_load_report_data">Could not load report data.</string>
 	<string name="send_report">Send report</string>
+	<string name="close">Close</string>
 	<string name="dev_report_saved">Report saved. It will be sent the next time you log into Briar.</string>
+
 	<string name="ongoing_notification_title">Signed into Briar</string>
 	<string name="ongoing_notification_text">Touch to open Briar.</string>
 	<string name="setup_title">Briar Setup</string>
@@ -87,6 +91,7 @@
 		<item quantity="other">%d forums shared by contacts</item>
 	</plurals>
 	<string name="show">Show</string>
+	<string name="hide">Hide</string>
 	<string name="show_forums">Show</string>
 	<string name="forum_leave">Leave Forum</string>
 	<string name="forum_left_toast">Left Forum</string>
diff --git a/briar-android/src/org/briarproject/android/report/DevReportActivity.java b/briar-android/src/org/briarproject/android/report/DevReportActivity.java
index b938cdadf..e7077e514 100644
--- a/briar-android/src/org/briarproject/android/report/DevReportActivity.java
+++ b/briar-android/src/org/briarproject/android/report/DevReportActivity.java
@@ -14,6 +14,7 @@ import android.view.MenuInflater;
 import android.view.MenuItem;
 import android.view.View;
 import android.view.inputmethod.InputMethodManager;
+import android.widget.Button;
 import android.widget.CheckBox;
 import android.widget.CompoundButton;
 import android.widget.EditText;
@@ -74,7 +75,7 @@ public class DevReportActivity extends BaseCrashReportDialog
 	private EditText userCommentView = null;
 	private EditText userEmailView = null;
 	private CheckBox includeDebugReport = null;
-	private View chevron = null;
+	private Button chevron = null;
 	private LinearLayout report = null;
 	private View progress = null;
 	private MenuItem sendReport = null;
@@ -113,9 +114,8 @@ public class DevReportActivity extends BaseCrashReportDialog
 		final View requestReport = findViewById(R.id.request_report);
 		userCommentView = (EditText) findViewById(R.id.user_comment);
 		userEmailView = (EditText) findViewById(R.id.user_email);
-		TextView debugReport = (TextView) findViewById(R.id.debug_report);
 		includeDebugReport = (CheckBox) findViewById(R.id.include_debug_report);
-		chevron = findViewById(R.id.chevron);
+		chevron = (Button) findViewById(R.id.chevron);
 		report = (LinearLayout) findViewById(R.id.report_content);
 		progress = findViewById(R.id.progress_wheel);
 
@@ -126,8 +126,12 @@ public class DevReportActivity extends BaseCrashReportDialog
 		userCommentView.setHint(isFeedback() ? R.string.enter_feedback :
 				R.string.describe_crash);
 
-		debugReport.setVisibility(isFeedback() ? GONE : VISIBLE);
-		includeDebugReport.setVisibility(isFeedback() ? VISIBLE : GONE);
+		if (isFeedback()) {
+			includeDebugReport
+					.setText(getString(R.string.include_debug_report_feedback));
+		} else {
+			includeDebugReport.setChecked(true);
+		}
 
 		findViewById(R.id.acceptButton).setOnClickListener(
 				new View.OnClickListener() {
@@ -151,11 +155,15 @@ public class DevReportActivity extends BaseCrashReportDialog
 		chevron.setOnClickListener(new View.OnClickListener() {
 			@Override
 			public void onClick(View v) {
-				chevron.setSelected(!chevron.isSelected());
-				if (chevron.isSelected())
+				boolean show =
+						chevron.getText().equals(getString(R.string.show));
+				if (show) {
+					chevron.setText(R.string.hide);
 					refresh();
-				else
+				} else {
+					chevron.setText(R.string.show);
 					report.setVisibility(GONE);
+				}
 			}
 		});
 
@@ -367,7 +375,7 @@ public class DevReportActivity extends BaseCrashReportDialog
 						fields.add(field.name());
 					}
 					prefEditor.putStringSet(PREF_EXCLUDED_FIELDS, fields);
-					prefEditor.commit();
+					prefEditor.apply();
 				}
 
 				if (success) {