Add defaults to AppSettingsDialog and string res #74

SUPERCILEX · 2017-01-18T04:12:31Z

This change depends on #73 because I forgot to switch branches. I could try to cherry pick my commit if necessary.

Hey @samtstern my app is in the process of getting an Export as CSV option so I was like I'm definately going to use @samtstern's easypermissions lib and I'm already loving it!

Anyway, back to the PR. I noticed that you can't pass in a string res in AppSettingsDialog whereas you can in AlertDialog. Also, I think AppSettingsDialog should provide default values for the title and rationale because those are usually good enough. @samtstern If you don't agree with any of my statements above, please tell me and I can file to issue to discuss it more.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

# Conflicts: # app/build.gradle # build.gradle # constants.gradle # easypermissions/build.gradle # gradle/wrapper/gradle-wrapper.properties

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

… Also allow use of string res instead of just strings Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2017-01-18T04:12:59Z

app/src/main/java/pub/devrel/easypermissions/sample/MainActivity.java

-            new AppSettingsDialog.Builder(this, getString(R.string.rationale_ask_again))
-                    .setTitle(getString(R.string.title_settings_dialog))
-                    .setPositiveButton(getString(R.string.setting))
-                    .setNegativeButton(getString(R.string.cancel), null /* click listener */)


Do we want to keep these with the default values just for the demo?

SUPERCILEX · 2017-01-18T04:14:02Z

easypermissions/src/main/AndroidManifest.xml

@@ -1,3 +1,3 @@
-<manifest package="com.google.example.easypermissions">
+<manifest package="pub.devrel.easypermissions">


This seems to be a mistake: I couldn't get the project to find the R class with the old package name.

Good catch.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2017-01-18T04:35:03Z

@samtstern I had to make lint happy in 041f92f which is weird because #73 didn't fail. I guess this lib is a little cleaner now! 😄

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

…nd-string-res

SUPERCILEX · 2017-01-18T16:25:23Z

@samtstern merged!

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2017-01-18T17:55:41Z

@samtstern if this PR is good, could you hold off on merging? Thanks!

samtstern · 2017-01-18T20:08:04Z

app/src/main/res/layout/activity_basic.xml

    tools:context=".BasicActivity">

    <Button
        android:id="@+id/button_request"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
-        android:text="Request" />
+        android:text="Request"
+        tools:ignore="HardcodedText"/>


If we are going to add @string resources in other places let's do it here as well rather than ignoring.

I didn't do that because BasicActivity isn't part of the sample so it shouldn't pollute the strings.xml file. Do you agree with this or should I add it to the strings file anyway?

I see, that's fine either way then.

samtstern · 2017-01-18T20:08:29Z

easypermissions/src/main/AndroidManifest.xml

@@ -1,3 +1,3 @@
-<manifest package="com.google.example.easypermissions">
+<manifest package="pub.devrel.easypermissions">


Good catch.

samtstern · 2017-01-18T20:09:28Z

easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java


        // Positive button text, or default
        String positiveButtonText = TextUtils.isEmpty(positiveButton) ?
-                context.getString(android.R.string.ok) : positiveButton;
+                context.getString(R.string.settings) : positiveButton;


By changing this to be a string in the library but still using android.R.string.cancel for the negative, we could end up in a strange semi-translated state for non-english locales.

@samtstern What do you suggest doing? Should we keep android.R.string.ok or add a string for cancel so it isn't translated?

I think let's keep android.R.string.ok for now.

samtstern · 2017-01-18T20:10:54Z

easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java

         * @param rationale text explaining why the user should launch the app settings screen.
+         * @deprecated Use {@link #Builder(android.support.v4.app.Fragment)} with {@link
+         * #setRationale(String)} or {@link #setRationale(int)}.


The reason to include rationale in the builder constructor is that it implies that rationale is the only required option, the rest of the setters are optional.

I see that you're adding a default message. That should be OK.

Yeah, I removed it because of that reason and also because have 6 different builder constructors would be crazy (a pair of two for the int and string overloads).

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

samtstern · 2017-01-20T17:23:39Z

@SUPERCILEX can I merge this now? #73 was merged, any other dependencies?

SUPERCILEX · 2017-01-20T17:58:32Z

@samtstern I wanted to wait for #75 because it deprecates setNegativeButton. We could merge this one, but I'd have to either remove the int varient of setNegativeButton in #75 or immediately deprecate it. What do you think is the best option?

…nd-string-res # Conflicts: # easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java # easypermissions/src/main/java/pub/devrel/easypermissions/EasyPermissions.java

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

samtstern · 2017-01-24T19:52:10Z

@SUPERCILEX conflicts, obviously. Happy to merge this once fixed though.

…nd-string-res # Conflicts: # app/src/main/java/pub/devrel/easypermissions/sample/MainActivity.java # easypermissions/src/main/AndroidManifest.xml # easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2017-01-24T23:57:59Z

@samtstern Thanks for waiting on this! Could you just wait a tad bit longer? (I want to make sure I didn't make any stupid mistakes) Thanks!

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2017-01-25T00:07:14Z

@samtstern I fixed some mistakes so it should be good now, but feel free to take another look at it!

SUPERCILEX added 8 commits January 17, 2017 19:11

Update dependencies and extract constants.gradle

c89a2dc

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Merge remote-tracking branch 'easypermissions/master'

9be3f11

# Conflicts: # app/build.gradle # build.gradle # constants.gradle # easypermissions/build.gradle # gradle/wrapper/gradle-wrapper.properties

Fix merge mistakes

0aeeca7

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

More dep updates

9416367

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Add default values for rationale and title to AppSettingsDialog.java.…

d51702e

… Also allow use of string res instead of just strings Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Update .travis.yml

70958eb

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Merge branch 'master' into defaults-and-string-res

7d857e7

Cleanup

db9ad8c

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX commented Jan 18, 2017

View reviewed changes

I guess lint turns itself in android gradle tools 2.3 so make it happy

041f92f

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX added 2 commits January 17, 2017 22:22

Update documentation

e564758

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Merge remote-tracking branch 'easypermissions/master' into defaults-a…

8089cd6

…nd-string-res

Cleanup

14b2e6b

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

samtstern reviewed Jan 18, 2017

View reviewed changes

Change string rsc

84d7dd8

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX added 3 commits January 23, 2017 16:23

Merge remote-tracking branch 'easypermissions/master' into defaults-a…

13ab4e6

…nd-string-res # Conflicts: # easypermissions/src/main/java/pub/devrel/easypermissions/AppSettingsDialog.java # easypermissions/src/main/java/pub/devrel/easypermissions/EasyPermissions.java

Fix merge mistakes

d8dd887

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Fix Travis

8f89c7e

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX added 2 commits January 24, 2017 15:52

Fix merge mistakes

3913483

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Fix mistakes

571000f

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

samtstern merged commit a11c288 into googlesamples:master Jan 25, 2017

SUPERCILEX deleted the defaults-and-string-res branch January 25, 2017 00:20

		@@ -1,3 +1,3 @@
		<manifest package="com.google.example.easypermissions">
		<manifest package="pub.devrel.easypermissions">

Add defaults to AppSettingsDialog and string res #74

Add defaults to AppSettingsDialog and string res #74

Uh oh!

Conversation

SUPERCILEX commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Jan 18, 2017

Uh oh!

SUPERCILEX commented Jan 18, 2017

Uh oh!

SUPERCILEX commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jan 20, 2017

Uh oh!

SUPERCILEX commented Jan 20, 2017

Uh oh!

samtstern commented Jan 24, 2017

Uh oh!

SUPERCILEX commented Jan 24, 2017

Uh oh!

SUPERCILEX commented Jan 25, 2017

Uh oh!

Uh oh!