plyer icon indicating copy to clipboard operation
plyer copied to clipboard

Choose a reasonable and valid number for the intent code

Open matan-h opened this issue 1 year ago • 3 comments

This PR makes the Android file chooser use a reasonable and valid number in the intent code. As this Stack Overflow post explains, the maximum value for the requestCode is 2^16 (65536).

Previously, the code used arbitrary numbers (123456 and 654321), which is making some phones encounter a Java Exception: java.lang.IllegalArgumentException: Can only use lower 16 bits for requestCode.

matan-h avatar Dec 11 '23 19:12 matan-h

I am not 100% sure I understand the context on this code, so let me explain what I think:

  • The save_code and result_code are meant to be unique identifiers, that show that the callback is related to this particular call.
  • The Pythonic way would be to use an object() call to guarantee uniqueness within the Python application. I believe the other callers are not coming from Python, so this technique isn't guaranteed.
  • Using a uuid would be another standard approach to avoid clashes. I am not clear whether there is a hardware limit that makes this impossible.
  • The old code just generates two random 6 digit numbers and just hopes there isn't a clash.
  • The old code just generates two random 5 digit numbers and just hopes there isn't a clash.

My comments:

  • 5 digits doesn't feel rare enough. There will be clashes in the field.
  • Your edit says 65536 is the maximum, but doesn't explain what is imposing this maximum. Given it is 2^16, it sounds like it must be an 16-bit OS limitation. If so, you should explain (link?) to why it is so limited. If not, how about 2^32 instead?
  • save_code and result_code can randomly be generated as the same number, which will cause problems. How about make the top end of the range of the first one one smaller, and then define the second one as the first one + 1? That way they never clash.

Julian-O avatar Dec 11 '23 23:12 Julian-O

Thank you, @Julian-O. I've added an explanation to the code and edited the description of this pull request to make it cleaner.

I've also removed the other minor change as that would probably require a design change. Here is the android code which creating this error in the new API:

matan-h avatar Dec 12 '23 07:12 matan-h

Thanks. I understand now.

[Just to be clear, I am not an official reviewer with merge rights. We'll need to wait for one of them.]

Julian-O avatar Dec 12 '23 13:12 Julian-O