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

use Chaquopy syntax directly in the android folder #2129

Merged
merged 46 commits into from
Sep 27, 2023

Conversation

dgmouris
Copy link
Contributor

@dgmouris dgmouris commented Sep 21, 2023

Trying to remove references to toga_android.libs by importing from chaquopy directly.

Fixes #1980

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@dgmouris
Copy link
Contributor Author

dgmouris commented Sep 22, 2023

Tests run so far for reference:

briefcase run android -r --test -- tests/widgets/test_base.py
briefcase run android -r --test -- tests/widgets/test_box.py
briefcase run android -r --test -- tests/widgets/test_button.py
briefcase run android -r --test -- tests/widgets/test_dateinput.py
briefcase run android -r --test -- tests/test_images.py
briefcase run android -r --test -- tests/test_icons.py
briefcase run android -r --test -- tests/test_fonts.py
briefcase run android -r --test -- tests/test_paths.py

briefcase run android -r --test -- tests/widgets/test_detailedlist.py
briefcase run android -r --test -- tests/widgets/test_imageview.py
briefcase run android -r --test -- tests/widgets/test_label.py
briefcase run android -r --test -- tests/widgets/test_multilinetextinput.py
briefcase run android -r --test -- tests/widgets/test_numberinput.py
briefcase run android -r --test -- tests/widgets/test_passwordinput.py
briefcase run android -r --test -- tests/widgets/test_progressbar.py
briefcase run android -r --test -- tests/widgets/test_scrollcontainer.py
briefcase run android -r --test -- tests/widgets/test_selection.py
briefcase run android -r --test -- tests/widgets/test_slider.py
briefcase run android -r --test -- tests/widgets/test_switch.py
briefcase run android -r --test -- tests/widgets/test_table.py
briefcase run android -r --test -- tests/widgets/test_timeinput.py

@freakboy3742
Copy link
Member

Looking good; as a quick note, if you add a change note file to your PR, CI will run the full testbed suite when you push your code to GitHub. Right now, it's failing tests early because there's no change note.

@mhsmith
Copy link
Member

mhsmith commented Sep 23, 2023

Also, I see there's a couple of CI failures in test_button (follow the "testbed (android)" link below):

I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/requirements/toga_android/widgets/base.py", line 162, in set_background_filter
I/python.stdout:     native_color(value), dynamic_proxy(PorterDuff.Mode).SRC_IN
I/python.stdout:   File "proxy.pxi", line 50, in java.chaquopy.dynamic_proxy
I/python.stdout: TypeError: <class 'android.graphics.PorterDuff$Mode'> is not a Java interface

dynamic_proxy should only be used in class headers, and then only on classes which were previously imported using JavaInterface as opposed to JavaClass.

@dgmouris
Copy link
Contributor Author

Also, I see there's a couple of CI failures in test_button (follow the "testbed (android)" link below):

I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/requirements/toga_android/widgets/base.py", line 162, in set_background_filter
I/python.stdout:     native_color(value), dynamic_proxy(PorterDuff.Mode).SRC_IN
I/python.stdout:   File "proxy.pxi", line 50, in java.chaquopy.dynamic_proxy
I/python.stdout: TypeError: <class 'android.graphics.PorterDuff$Mode'> is not a Java interface

dynamic_proxy should only be used in class headers, and then only on classes which were previously imported using JavaInterface as opposed to JavaClass.

Thank you! I was fumbling my way around until I had a bit of deeper understanding later on as I was testing it.

I'll update this shortly with both updates (updating this dynamic_proxy, and removing libs).

I really appreciate the help again folks @freakboy3742 and @mhsmith !

@dgmouris
Copy link
Contributor Author

@freakboy3742 @mhsmith I'm sure you folks are aware of this and I think it was discussed in the discord but there is no testbed/test_canvas.py. I have changed this file, although there are no tests at this current moment in time.

It seems to be in the works, based on the discord discussion.

@dgmouris
Copy link
Contributor Author

The only errors that I get with this PR is

I/python.stdout: tests/test_paths.py::test_app_paths[config] 
I/python.stdout: ERROR                        [  5%]
I/python.stdout: 
I/python.stdout: tests/test_paths.py::test_app_paths[data] 
I/python.stdout: ERROR                          [  5%]
I/python.stdout: 
I/python.stdout: tests/test_paths.py::test_app_paths[cache] 
I/python.stdout: ERROR                         [  6%]
I/python.stdout: 
I/python.stdout: tests/test_paths.py::test_app_paths[logs] 
I/python.stdout: ERROR                          [  6%]
I/python.stdout: 

I get these tests to pass locally... I can't trigger this action to run again here unless I add another commit.

I've removed the libs and everything here is exclusively using chaquopy. Let em know if there's anything I'm missing for issue #1980 but it seems like it covers it :)

Thanks again for the help.

@dgmouris
Copy link
Contributor Author

On a separate note this PR affects #2130

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've flagged a couple of minor things inline, but otherwise, this looks like a thorough set of changes. Nice work!

The test failures you're seeing look like they've been caused by the final removal of the libs directory. There's one last reference to toga_android.libs in the tests_backend folder. I'm guessing it's passing for you locally because of a stale .pyc file or resource in your build. A clean build (including deleting the app from your phone/emulator) should reproduce the bug.

There's one closely related change that might also be possible as part of this - removing rubicon-java as a package requirement. Now that we're not using Rubicon syntax, I think we may be able to finish the job and remove Rubicon as a package requirement. I'm not 100% certain about this - there might be one or two subtle uses that are lingering - but if we're in a position to remove it, we should. However, if it turns out things start exploding when you remove this requirement, and the fix isn't immediately obvious, that's not a blocker to merging what you've got here.

android/src/toga_android/widgets/progressbar.py Outdated Show resolved Hide resolved
changes/1980.misc.rst Outdated Show resolved Hide resolved
@mhsmith
Copy link
Member

mhsmith commented Sep 26, 2023

There's one closely related change that might also be possible as part of this - removing rubicon-java as a package requirement. Now that we're not using Rubicon syntax, I think we may be able to finish the job and remove Rubicon as a package requirement. I'm not 100% certain about this - there might be one or two subtle uses that are lingering - but if we're in a position to remove it, we should.

I've already removed the calls to __global__, since Chaquopy ensures all Java objects exposed to Python code use global JNI references. So I think the only remaining use of Rubicon (probably the only remaining appearance of the word "rubicon" on this PR's branch) is the import of android_events in toga_android/app.py.

This file currently lives in the Briefcase Android template, so to make it easier to update, it should be copied into toga_android. The copy in the template should be left alone for now, because it's needed by older versions of Toga.

@dgmouris
Copy link
Contributor Author

The test failures you're seeing look like they've been caused by the final removal of the libs directory. There's one last reference to toga_android.libs in the tests_backend folder. I'm guessing it's passing for you locally because of a stale .pyc file or resource in your build. A clean build (including deleting the app from your phone/emulator) should reproduce the bug.

Just removed that reference in the toga_android/tests_backend folder.

PS. This project has a very impressive test suite folks, I'm glad I got to use and see it in full force here.

@mhsmith
Copy link
Member

mhsmith commented Sep 26, 2023

Thanks, it's been our main focus for the last 6 months.

Do you want to do the last couple of things mentioned above?

  • Removing rubicon_java from setup.py
  • Copying android_events.py into toga_android, and updating it to use the new import syntax

I think those are the only remaining references to "rubicon" in the android folder, so it would make sense to remove them in this PR. But if not, I'm happy to merge it as it is.

@freakboy3742
Copy link
Member

  • Copying android_events.py into toga_android, and updating it to use the new import syntax

Ironically, this is a module that will need to be in toga_android.libs, similar to the proactor.py module in Winforms. The reference to the original rubicon version in app.py will need to be updated as well.

@dgmouris
Copy link
Contributor Author

  • Copying android_events.py into toga_android, and updating it to use the new import syntax

Ironically, this is a module that will need to be in toga_android.libs, similar to the proactor.py module in Winforms. The reference to the original Rubicon version in app.py will need to be updated as well.

I didn't read this before I did my most recent push to remove the Rubicon requirement. I can remove it if needed, no problem there.

If I'm not mistaken you folks are discussing using a similar approach to

class WinformsProactorEventLoop(asyncio.ProactorEventLoop):
in the toga repo except using the briefcase android event loop to use chaquopy in toga_android.libs

IMO the easiest path forward for this would be to:

  1. merge this as it's getting without removing Rubicon (I'll remove that commit)
  2. create a new PR to add the event loop (as we'll probably add toga_android.libs for the event loop. (I'd be happy to hack away on this as well and learn a bit about briefcase) and remove Rubicon for the Android template
  3. create a new PR in briefcase to remove it (but I'm not sure about the repercussions tbh.

I'm good with either way:) Let me know what you folks think @mhsmith and @freakboy3742

@mhsmith
Copy link
Member

mhsmith commented Sep 27, 2023

Moving over android_events.py should be pretty simple, so I'll do it myself now.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks like Malcolm has pushed the ball the last couple of inches - so I think this is done! Thanks for the contribution - there's a lot of work here, and it significantly improves the readability of the Android codebase.

@freakboy3742 freakboy3742 merged commit 38b2831 into beeware:main Sep 27, 2023
41 checks passed
@t-arn
Copy link
Contributor

t-arn commented Sep 29, 2023

On a separate note this PR affects #2130

Noted, thanks. I'll do a merge when I finalize the PR.

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.

Android: use Chaquopy syntax directly
4 participants