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

Provider generics interface to handle non-png icons #32

Merged
merged 5 commits into from
Oct 24, 2022

Conversation

jmatsu
Copy link

@jmatsu jmatsu commented Oct 18, 2022

Waiting for

Breaking changes

  • backward_compatible_adaptive_icon? now returns true whenever it's configured properly.
  • Drop setters of icon, adaptive_icon, backward_compatible_adaptive_icon`

New itnerface

  • AppIcon class: this provides IO interface to get app icons
  • AndroidApk#app_icons: this returns an array sorted by resolution priority
  • Xmltree: this is not a XML document but tree provided by aapt/aapt2

@jmatsu jmatsu force-pushed the jmatsu/correct_xml_icon_handling branch from 161d70f to e168af5 Compare October 20, 2022 14:50
@jmatsu jmatsu changed the base branch from master to feat/use_ruby26 October 20, 2022 14:51
@@ -168,3 +168,6 @@ Style/TrailingCommaInArrayLiteral:

Style/TrailingCommaInHashLiteral:
Enabled: false

Style/AccessModifierDeclarations:
Copy link
Author

Choose a reason for hiding this comment

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

end

# Check whether or not this apk's icon is an adaptive icon
# @return [Boolean] Return true if this apk has an *correct* adaptive icon, otherwise false.
Copy link
Author

@jmatsu jmatsu Oct 20, 2022

Choose a reason for hiding this comment

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

*correct* means Technically correct.

# invalid xml file may throw an error
Zip::File.open(filepath) do |zip_file|
apk.adaptive_icon = zip_file.find_entry(adaptive_icon_path)&.get_input_stream do |entry|
!!entry.read.include?("adaptive-icon")
Copy link
Author

@jmatsu jmatsu Oct 20, 2022

Choose a reason for hiding this comment

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

this logic worked even if the xml is decoded but the result included false-positives. That's why I delegate the content look-up to aapt dump xmltree.

@@ -16,7 +16,6 @@ Gem::Specification.new do |spec|
spec.required_ruby_version = ">= 2.6.0"

spec.files = `git ls-files | grep -v 'spec/fixture'`.split($/)
spec.test_files = spec.files.grep(%r{^(test|spec)/})
Copy link
Author

Choose a reason for hiding this comment

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

self.verified = false
self.test_only = false
end

# @return [Array<AndroidApk::AppIcon>]
def app_icons
Copy link
Author

Choose a reason for hiding this comment

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

We will check this array from 0 to last to look up icons.

content&.split(/\n/) do |line|
line.strip!

if line.start_with?("E: ")
Copy link
Author

Choose a reason for hiding this comment

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

The first E: -prefixed line indicates the root element of the XML.

@root_element = nil

line_num = 1
content&.split(/\n/) do |line|
Copy link
Author

Choose a reason for hiding this comment

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

tip: split with block requires Ruby 2.6

@jmatsu jmatsu marked this pull request as ready for review October 20, 2022 15:10
Base automatically changed from feat/use_ruby26 to master October 21, 2022 04:27
@anoworl anoworl self-requested a review October 21, 2022 09:10
Copy link

@anoworl anoworl left a comment

Choose a reason for hiding this comment

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

LGTM!

@jmatsu
Copy link
Author

jmatsu commented Oct 24, 2022

Thank you so much~~~!

@jmatsu jmatsu merged commit 1f5cb65 into master Oct 24, 2022
@jmatsu jmatsu deleted the jmatsu/correct_xml_icon_handling branch October 24, 2022 06:13
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.

2 participants