jordanbyron / react-native-quick-actions

A react-native interface for Touch 3D home screen quick actions
MIT License
1.06k stars 94 forks source link

[Android] Use PersistableBundle to persist the whole ShortCutItem object #54

Closed ItsNoHax closed 5 years ago

ItsNoHax commented 6 years ago

Fixes: #51

Pro's:

  1. In memory list mShortcutItems is no longer needed.
  2. 'Type' is technically useless now but I think we should leave it to distinguish each shortcut.
  3. Method: getShortcutItem is no longer needed since the whole object is persisted, not just type.
  4. The whole object persists on COLD APP launch, now consistent with iOS version.

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

jordanbyron commented 6 years ago

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.

ItsNoHax commented 6 years ago

@jordanbyron Managed to have a look at this?

jordanbyron commented 6 years ago

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 notifications@github.com 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 https://github.com/jordanbyron/react-native-quick-actions/pull/54#issuecomment-355904919, or mute the thread https://github.com/notifications/unsubscribe-auth/AACe1nxlta4Q6t8_VQSup_TXihImg_AWks5tIc-KgaJpZM4RFmbA .

-- Jordan

ItsNoHax commented 6 years ago

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?

jordanbyron commented 6 years ago

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 notifications@github.com 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 https://github.com/jordanbyron/react-native-quick-actions/pull/54#issuecomment-366911551, or mute the thread https://github.com/notifications/unsubscribe-auth/AACe1tldJ_hvW-ZBbiuv3CWhhTF6sdpVks5tWooJgaJpZM4RFmbA .

-- Jordan

AndrewJack commented 6 years ago

@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:

ItsNoHax commented 6 years ago

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.

jordanbyron commented 6 years ago

@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?

screen shot 2018-03-03 at 1 37 10 pm

ItsNoHax commented 6 years ago

Hey @jordanbyron, I'm not sure what that error means but I don't think it's related to my code? By the sounds of it I'm guessing you linked your project twice. Check your MainApplication/MainActivity for duplicates.

jsdario commented 6 years ago

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.

diego3g commented 5 years ago

Any updates on this?

ItsNoHax commented 5 years ago

I’ve been using my fork in production for months now without a problem

On Wed, 12 Dec 2018 at 12:30, Diego Fernandes notifications@github.com wrote:

Any updates on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jordanbyron/react-native-quick-actions/pull/54#issuecomment-446555664, or mute the thread https://github.com/notifications/unsubscribe-auth/AEBxyhOA4sZRsMk6ndhUJZiH3btdRc3sks5u4Oi_gaJpZM4RFmbA .

diego3g commented 5 years ago

@ItsNoHax Can you publish your fork to NPM registry?

jordanbyron commented 5 years ago

🤠 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.

ItsNoHax commented 5 years ago

Let me know if something goes wrong or some edge case appears and I'll gladly get it fixed.