-
Notifications
You must be signed in to change notification settings - Fork 96
[Android] Use PersistableBundle to persist the whole ShortCutItem object #54
Conversation
Thanks @ItsNoHax! I've skimmed through the code and nothing jumps out as immediately concerning. I need to fire up my old copy of Android Studio and play around with this to make sure it works as expected. If you don't hear from me in a week or so feel free to ping me again. |
@jordanbyron Managed to have a look at this? |
Hey Alberto. I still haven’t had a chance to get Android studio installed.
I’ll do my best to take a look at this over the weekend. Thanks for your
patience.
On Mon, Jan 8, 2018 at 3:19 AM Alberto Blanco ***@***.***> wrote:
@jordanbyron <https://github.com/jordanbyron> Managed to have a look at
this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACe1nxlta4Q6t8_VQSup_TXihImg_AWks5tIc-KgaJpZM4RFmbA>
.
--
Jordan
|
Hey man, I would very much like to close this issue at work. Could you please take 5 minutes of your time to look at this? |
Sadly this is not just a 5 minute check. I have to get my Android Studio
app setup and working again so I can run your code. This has been on my
radar but I just haven’t had time to check it out.
Additionally someone pointed out some potential issues with your
implementation. Not sure where the comment went but I’ll paste the text
below:
---
I don't see the benefit of changing to PersistableBundle if you don't
actually use it to persist to a store. It is only a wrapper for easy
serialization.
Starting in API 21, there is a new PersistableBundle class that is a
variant of Bundle with a stable data format that supports serialization as
XML. It only accepts a subset of the data types supported by Bundle. In
particular, it does not support Parcelable objects.
https://medium.com/google-developers/developing-for-android-v-f6b8038b42f5
However, you have pointed out that the existing code can be simplified a
bit.
References:
https://developer.android.com/reference/android/os/PersistableBundle.html
https://medium.com/google-developers/developing-for-android-v-f6b8038b42f5
On Tue, Feb 20, 2018 at 4:02 AM Alberto Blanco ***@***.***> wrote:
Hey man, I would very much like to close this issue at work. Could you
please take 5 minutes of your time to look at this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACe1tldJ_hvW-ZBbiuv3CWhhTF6sdpVks5tWooJgaJpZM4RFmbA>
.
--
Jordan
|
@jordanbyron I deleted the comment. I didn't read the diff right. The code looks fine to me. I'll try and test this PR over the next few days. Links are still useful to understand PersistableBundle: |
Just answering some points in the comment deleted. If I remember correctly, the reason I used PersistableBundle is because it restricts the data types it can hold to make sure it is always persistable since ShortCutManager saves the shortcutinfo on disk. |
@ItsNoHax I finally had a chance to update my Android Studio so I could test out this PR and it seems like there is a bit of a problem. Any ideas? |
Hey @jordanbyron, I'm not sure what that error means but I don't think it's related to my code? |
Hi all, I tried the fork and can't use it straight away. I believe it's because I need to lift the API (still using lvl 16, and have users at lower than 21). Is there a way to only use the native code if it is supported by the API level? I am not really a Java developer and already tried, but couldn't get it working. |
Any updates on this? |
I’ve been using my fork in production for months now without a problem
…On Wed, 12 Dec 2018 at 12:30, Diego Fernandes ***@***.***> wrote:
Any updates on this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEBxyhOA4sZRsMk6ndhUJZiH3btdRc3sks5u4Oi_gaJpZM4RFmbA>
.
|
@ItsNoHax Can you publish your fork to NPM registry? |
🤠 I'll just merge this. If something goes wrong with the android side of the house I'll expect ya'll to chip in and help fix it. As I've stated before the Android code was "gifted" to me and I don't use it. |
Let me know if something goes wrong or some edge case appears and I'll gladly get it fixed. |
Fixes: #51
Pro's:
Cons:
This should cause no regression but I am not 100% sure whether iOS returns the whole object like this.
Let me know what you think,
Alberto Blanco