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

Add icon-bg and icon-fg to fix_args #2633

Merged
merged 3 commits into from
Aug 4, 2022

Conversation

danigm
Copy link
Contributor

@danigm danigm commented Jul 7, 2022

Fix the icon-bg and icon-fg arguments and also create the folder
mipmap-anydpi-v26 if it doesn't exists before trying to create a file
inside.

Fix the icon-bg and icon-fg arguments and also create the folder
mipmap-anydpi-v26 if it doesn't exists before trying to create a file
inside.
Comment on lines 352 to 355
mipmap_anydpi = join(res_dir, 'mipmap-anydpi-v26')
if not exists(mipmap_anydpi):
os.mkdir(mipmap_anydpi)
with open(join(mipmap_anydpi, 'icon.xml'), "w") as fd:
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 already defined function for checking path existence

        anydpi_dir = join(res_dir, 'mipmap-anydpi-v26')
        ensure_dir(anydpi_dir)
        with open(join(anydpi_dir, 'icon.xml'), "w") as fd:

Copy link

Choose a reason for hiding this comment

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

@danigm would you like to update the PR to use the ensure_dir utility function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a new commit to use ensure_dir.

@interlark
Copy link
Contributor

Also noticed that issue FileNotFoundError: [Errno 2] No such file or directory: 'src/main/res/mipmap-anydpi-v26/icon.xml' when you define --icon-fg and --icon-bg, was about to duplicate this PR, good that I checked first.

@@ -349,7 +349,10 @@ def make_package(args):
if args.icon_fg and args.icon_bg:
shutil.copy(args.icon_fg, join(res_dir, 'mipmap/icon_foreground.png'))
shutil.copy(args.icon_bg, join(res_dir, 'mipmap/icon_background.png'))
with open(join(res_dir, 'mipmap-anydpi-v26/icon.xml'), "w") as fd:
mipmap_anydpi = join(res_dir, 'mipmap-anydpi-v26')
Copy link

Choose a reason for hiding this comment

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

I agree with @interlark, anydpi_dir is a better name for this variable.

Suggested change
mipmap_anydpi = join(res_dir, 'mipmap-anydpi-v26')
anydpi_dir = join(res_dir, 'mipmap-anydpi-v26')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the new commit I've used the anydpi_dir variable name.

@danigm danigm requested review from manuq and interlark and removed request for manuq and interlark August 2, 2022 06:21
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

Hi @danigm!

Thank you for the PR, I've left a comment regarding one of the two proposed changes 😄

Comment on lines 352 to 353
with open(join(res_dir, 'mipmap-anydpi-v26/icon.xml'), "w") as fd:
anydpi_dir = join(res_dir, 'mipmap-anydpi-v26')
ensure_dir(anydpi_dir)
Copy link
Member

Choose a reason for hiding this comment

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

mipmap-anydpi-v26 is already present inside our template, and should be copied.

Can you better explain why that change is needed? Isn't the template working for your use-case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @misl6. When you specify --icon-fg and --icon-bg for --bootstrap=webview you end up with error FileNotFoundError: [Errno 2] No such file or directory: 'src/main/res/mipmap-anydpi-v26/icon.xml'

Manually creating mipmap-anydpi-v26 dir in build.py fixes this error. Could you, please, point where this dir is supposed to be created/copied?

Copy link

@manuq manuq Aug 2, 2022

Choose a reason for hiding this comment

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

Exactly. This is explained in the commit message. I guess this is one of those things that work from Buildozer but not if python-for-android is used directly.

Copy link
Member

@misl6 misl6 Aug 2, 2022

Choose a reason for hiding this comment

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

prepare_build_dir copies the content from the templates (which are placed here: bootstraps)

for --bootstrap=webview

Ouch, that makes sense.

The mipmap-anydpi-v26 folder seems to only be available in sdl2 boostrap: https://github.com/kivy/python-for-android/tree/develop/pythonforandroid/bootstraps/sdl2/build/src/main/res, but not in the webview one: https://github.com/kivy/python-for-android/tree/develop/pythonforandroid/bootstraps/webview/build/src/main/res

The logic should be cleaned up and unified, in order to avoid this kind of issues in future 🧐

Meanwhile (unless you would like to clean up and unify the logic), would be nice to add the missing folders to webview, in order to avoid extra checks in code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the folder to the bootstrap template and removed the check code in the last commit.

@misl6 misl6 self-assigned this Aug 2, 2022
This makes available the directory in the final template so it's not
needed to check the directory exists.
@danigm danigm requested a review from misl6 August 3, 2022 06:10
Copy link
Member

@misl6 misl6 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@misl6 misl6 merged commit e8686e2 into kivy:develop Aug 4, 2022
misl6 added a commit that referenced this pull request Sep 5, 2022
* Add option for copying arbitratry xml files to src/main/res/xml without touching manifest

* handle the case of getting list as res_xml

* Remove stray - in output file name

The final component in the package name is just the extension, so
prefixing it with a - means the output file ends in -.<ext>.

* Resolve absolute path to local recipes (#2640)

This is necessary when using patches in a local recipe since
`Recipe.apply_patch` assumes the recipe directory is absolute and uses
`patch -d` to change directories. It could just be fixed there, but this
ensures that recipe directories are always absolute.

Closes: #2623

* Fix webview Back button behaviour (#2636)

* Fixes TypeError: str.join() takes exactly one argument (2 given) in hostpython3/__init__.py", line 69 (#2642)

* Update __init__.py

.buildozer/android/platform/python-for-android/pythonforandroid/recipes/hostpython3/__init__.py", line 69, in get_recipe_env
    env["PKG_CONFIG_PATH"] = os.pathsep.join(
TypeError: str.join() takes exactly one argument (2 given)

* according of 

#2642 (review)

* RTSP support for ffmpeg

Added UDP and TCP protocols to "--enable-procol" to supporting RTSP streams over each.

Added RTSP to "--enable-demuxer" to supporting RTSP with ffmpeg.

* Fixes some E275 - assert is a keyword. (#2647)

* Add icon-bg and icon-fg to fix_args + adds a missing folder in webview dir template (#2633)

* Add icon-bg and icon-fg to fix_args

Fix the icon-bg and icon-fg arguments and also create the folder
mipmap-anydpi-v26 if it doesn't exists before trying to create a file
inside.

* Use existing ensure_dir function to create anydpi_dir

* Add mipmap-anydpi-v26 to bootstrap template

This makes available the directory in the final template so it's not
needed to check the directory exists.

* Updates matplotlib, fixes an issue related to shared libc++ (#2645)

* Updates matplotlib, fixes an issue related to shared libc++

* Cleanup

* Update supported Python versions (#2656)

* Remove six and enum34 dependency (#2657)

* Force --platform=linux/amd64 in Dockerfile (#2660)

* Use p4a_install instead of install, as a file named INSTALL is already present. (#2663)

* Update CHANGELOG.md & change version for release 2022.09.04

Co-authored-by: Eero af Heurlin <[email protected]>
Co-authored-by: Dan Nicholson <[email protected]>
Co-authored-by: Andre Miras <[email protected]>
Co-authored-by: Andy Trofimov <[email protected]>
Co-authored-by: Akshay Arora <[email protected]>
Co-authored-by: --=FurtiF™=-- <[email protected]>
Co-authored-by: alickc <[email protected]>
Co-authored-by: danigm <[email protected]>
@dbnicholson dbnicholson deleted the fix-icon-bg-arg-upstream branch September 19, 2022 17:24
shyamnathp pushed a commit to shyamnathp/python-for-android that referenced this pull request Feb 17, 2023
…w dir template (kivy#2633)

* Add icon-bg and icon-fg to fix_args

Fix the icon-bg and icon-fg arguments and also create the folder
mipmap-anydpi-v26 if it doesn't exists before trying to create a file
inside.

* Use existing ensure_dir function to create anydpi_dir

* Add mipmap-anydpi-v26 to bootstrap template

This makes available the directory in the final template so it's not
needed to check the directory exists.
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