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

refactor(android): simplify window & activity #249

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

erisu
Copy link
Member

@erisu erisu commented Oct 2, 2022

Platforms affected

Android

Motivation and Context

Declare class properties activity and window to shorten lines and remove in method defines.

Description

Created class properties and defined them plugin initialize.

Testing

build with AS

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@erisu erisu force-pushed the refactor/activity-window branch 2 times, most recently from 111c269 to 261baa4 Compare October 3, 2022 00:14
@erisu erisu marked this pull request as draft October 3, 2022 01:14
Copy link
Member

@jcesarmobile jcesarmobile 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 this PR should be the first one instead of the last one, since on the PR that changes the if to a switch is already using activity instead of this.cordova.getActivity().

Also, I wouldn't remove the array just yet, because we are going to need a new dark theme or system theme or similar, at the moment we have a PR to add dark theme, but not sure if it's the correct approach.
The problem is we use default = dark and light = light. But on iOS default = match the system theme (switches automatically based on the user theme)
So we should probably add a dark theme, a "system" theme or similar naming and deprecate default (in this case we will still need the array since it would do the same as dark mode)
Or add dark theme and change the default behavior back to the native "default" value.

@erisu erisu force-pushed the refactor/activity-window branch from 261baa4 to f7969af Compare October 5, 2022 07:35
@erisu erisu force-pushed the refactor/activity-window branch from f7969af to 3a393e3 Compare October 5, 2022 07:36
@erisu erisu marked this pull request as ready for review October 5, 2022 07:43
@erisu
Copy link
Member Author

erisu commented Oct 5, 2022

I think this PR should be the first one instead of the last one, since on the PR that changes the if to a switch is already using activity instead of this.cordova.getActivity().

I did forget to revert all changes from the switch case PR, but I had defined above the switch case statement this:

final Activity activity = this.cordova.getActivity();
final Window window = activity.getWindow();

This was a bit of a step towards the final goal of this PR.

Since I refactored the PRs for readability and remove stacking of PRs, this ended up becoming last.

Also, I wouldn't remove the array just yet, because we are going to need a new dark theme or system theme or similar, at the moment we have a PR to add dark theme, but not sure if it's the correct approach.
The problem is we use default = dark and light = light. But on iOS default = match the system theme (switches automatically based on the user theme)
So we should probably add a dark theme, a "system" theme or similar naming and deprecate default (in this case we will still need the array since it would do the same as dark mode)
Or add dark theme and change the default behavior back to the native "default" value.

I think we can add back the array if necessary.

IMO, clean up when possible is good, even if part of it is readded.
I am unaware of the progress of the development of that feature or ETA. Since it might take time, and maybe even the development becomes stale, I thought it would be better to remove clutter when possible.

Also based on the comment that the default value can be either dark or light, depending on the setting of the system's theme, I think the value default would not be in either array. That means there might be still only one value in each array if we add back STYLE_DARK_CONTENT and treat default as using system setting.

Seems the id condition would be better in that case, avoid looping an array, and remove extra import.

I feel like the code would become something like this:

private void setStatusBarStyle(final String style) {
  if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M && !style.isEmpty()) {
    View decorView = window.getDecorView();
    WindowInsetsControllerCompat windowInsetsControllerCompat = WindowCompat.getInsetsController(window, decorView);
    
    if (style.equals(STYLE_DEFAULT)) {
      windowInsetsControllerCompat.setAppearanceLightStatusBars(getIsNightModeActive());
    } else if (style.equals(STYLE_DARK_CONTENT)) {
      windowInsetsControllerCompat.setAppearanceLightStatusBars(true);
    } else if (style.equals(STYLE_LIGHT_CONTENT)) {
      windowInsetsControllerCompat.setAppearanceLightStatusBars(false);
    } else {
      LOG.e(TAG, "Invalid style, must be either 'default' or 'lightcontent'");
    }
  }
}

private boolean getIsNightModeActive() {
  Configuration configuration = activity.getResources().getConfiguration();
  
  switch (configuration.uiMode & Configuration.UI_MODE_NIGHT_MASK) {
    case Configuration.UI_MODE_NIGHT_YES:
    return true;
    case Configuration.UI_MODE_NIGHT_NO:
    default:
    return false;
  }
}

resources.configuration.isNightModeActive may also be a better method to use, but it requires minSDK of 30.

Anyways, I do not know the end goal for the style values.

As for this PR, it has been rebased and now focus on the creating and hoisting the class variable for window and activity.

@erisu erisu requested a review from jcesarmobile October 5, 2022 08:19
@erisu
Copy link
Member Author

erisu commented Oct 5, 2022

@jcesarmobile sorry, I accidentally click on the request re-review button without knowing that you already approved it 25 minutes ago. I thought it was still in the request-change mode.

@erisu erisu merged commit d4dcd71 into apache:master Oct 5, 2022
@erisu erisu deleted the refactor/activity-window branch October 5, 2022 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants