Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kotlin DSL #81

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

lutsenko-yuriy
Copy link

  • Adding Kotlin support to project
  • Building Kotlin DSL which in theory should make code more readable
  • Some changes in PermissionBuilder like turning String[] permissions into final List<String> permissions which should make code less error-prone
  • Some changes in ObjectUtils which also should make code less error-prone

Copy link
Contributor

@JSpiner JSpiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should pass the Travis build and modify the following.

@Override
protected void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no reason to change indent.
Please revert it!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad! Reverted it in last commit.


@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any lint error about this?
I don't know why this should be added.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now there is no lint error about it. I just don't like compile warnings. Removed it anyway.

The only thing lint complains about right now is an absence of a korean translation. I gave the one "powered" by Google Translate. Probably I will need your assist in translating text because I'm not sure that the current translation is correct.

@@ -64,30 +79,63 @@ protected void checkPermissions() {
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
intent.addFlags(Intent.FLAG_ACTIVITY_NO_USER_ACTION);
TedPermissionActivity.startActivity(context, intent, listener);
TedPermissionBase.setFirstRequest(context,permissions);
TedPermissionBase.setFirstRequest(context, permissionsAsStringArray());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about changing setFirstRequest(Context, String[]) to setFirstRequest(Context, List)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Also I decided to move stringListToStringArray to ObjectUtils class.

}

protected void checkPermissions() {
if (listener == null) {
throw new IllegalArgumentException("You must setPermissionListener() on TedPermission");
} else if (ObjectUtils.isEmpty(permissions)) {
} else if (permissions.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks changed to ObjectUtils.isEmpty(permissions).
It can be null.

return (T) this;
}

public T addSinglePermission(String permission) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about change naming to addPermission?
(It's just a recommendation. This is also good.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment. I thought about it but I guess addPermissions and addPermission look too similar and it would be easy to mix them up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Thanks

SCREEN_ORIENTATION_SENSOR_PORTRAIT,
SCREEN_ORIENTATION_REVERSE_LANDSCAPE,
SCREEN_ORIENTATION_REVERSE_PORTRAIT,
SCREEN_ORIENTATION_FULL_SENSOR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks need to add followings.

  • SCREEN_ORIENTATION_FULL_USER
  • SCREEN_ORIENTATION_LOCKED
  • SCREEN_ORIENTATION_USER_LANDSCAPE
  • SCREEN_ORIENTATION_USER_PORTRAIT

@lutsenko-yuriy lutsenko-yuriy force-pushed the kotlin-dsl branch 2 times, most recently from ee4b8a1 to 445fb1f Compare March 5, 2018 21:15
@JSpiner
Copy link
Contributor

JSpiner commented Mar 6, 2018

Because of there is no test code in this project, I'll run code and reply. Thanks

@ParkSangGwon
Copy link
Owner

@lutsenko-yuriy
Sorry, Nowadays I start study kotlin language.
When I finish study, I will review your code.
Or If many other people approve this PR, I will approve too.

@lutsenko-yuriy lutsenko-yuriy changed the title Kotlin DSL [WIP] Kotlin DSL Jun 3, 2018
@lutsenko-yuriy
Copy link
Author

@ParkSangGwon
That's ok. There is actually some room for improvements so I marked this pull request as Work In Progress. Thank you for your reply anyway

@lutsenko-yuriy lutsenko-yuriy changed the title [WIP] Kotlin DSL Kotlin DSL Jun 30, 2018

public abstract class PermissionBuilder<T extends PermissionBuilder> {

private static final String PREFS_NAME_PERMISSION = "PREFS_NAME_PERMISSION";
private static final String PREFS_IS_FIRST_REQUEST = "PREFS_IS_FIRST_REQUEST";

private PermissionListener listener;
private String[] permissions;

private final List<String> permissions = new ArrayList<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should resolve crash reported on issue #114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants