Added swipe refresh layout to notify user of repository update

See #341 (closed) , #425

Title changed from Added swipe refresh layout to notify user of repository update in to Added swipe refresh layout to notify user of repository update

Added 1 commit:

• 59f6017e - Added swipe refresh layout to notify user of repository update in

Added 1 commit:

• cc7bade5 - Added swipe refresh layout to notify user of repository update in

Added 1 commit:

• 02d734c7 - Added swipe refresh layout to notify user of repository update in

Thanks @paresh, I think many people will be happy with this change. I had one minor nitpick in my comment above, but that shouldn't stop this feature going in because it is more an opinion rather than an objective "the code is wrong" thing. In general code looks great. TIL about sticky broadcasts :)

This isn't fixing #341 (closed), right? But it should be fixing #425.

I think it will fix both of them. As for whether the pull to refresh should show more feedback in the future (i.e. a progress bar with some additional feedback ala the curren tnotification) is probably a new issue if somebody wants to raise it.

@paresh, @mvdan: Do you think we can remove the "update" action bar item now? Do you think swipe to refresh is known enough to make this redundant? I think it is redundant having it in the action bar as well as supported by swipe. It will just be a matter of having a changelog entry that we can point people to when they ask "Where is the update menu item?".

@paresh: As per IRC, this is going to go into master, but we've branched of stable-v0.100 which means this will end up in v0.101 as it is a new feature. We are in the process of ironing out bugs for the upcoming stable.

Milestone changed to 0.101

Sounds good to me.

Also, to clarify my comment:

Do you think we can remove the "update" action bar item now?

I don't mean in this MR, but as a subsequent issue if you guys agree.

Removing the menu option sounds fine.

As I understand, the comment above still needs to be addressed, right?

I understand @paresh you have exams on now if I remember correctly, and I don't think we are in any particular rush. But yes, I would like to see that comment addressed before merging some time in the future if people are alright with that.

OK, no hurry. Just checking :)

Added 1 commit:

• def70021 - Added swipe refresh layout to notify user of repository update in

Added 1 commit:

• 85f12325 - Added swipe refresh layout to notify user of repository update in

humm, I might need few more days to see If we need to call removeStickyBroadcast in case sticky broadcast persists which would result in weird behavior, due to sendStickyBroadcast being called multiple times.

Sticky broadcasts should not be used in F-Droid at all. They have security issues and are slow.

• https://groups.google.com/forum/?_escaped_fragment_=topic/android-developers/8341SaXhvmY#!topic/android-developers/8341SaXhvmY

Use LocalBroadcastManager.

I think sticky broadcasts might even be deprecated https://stackoverflow.com/questions/3490913/what-is-a-sticky-broadcast

@eighthave, It's more of a prevention deprecation as that would prevent (new)developers from using it. if you are passing sensitive[1] information you should refrain from using it. but in our case we aren't.

Sticky broadcasts cannot be secured with a permission and therefore are accessible to any receiver. If these broadcasts contain sensitive data or reach a malicious receiver, the application may be compromised.

The android OS uses sticky broadcast for various functionalities like telling apps that battery is low.

Information exposure is the biggest threat if we are using sticky broadcast (exposure from intents)[2] Though I can find a workaround using LocalBroadcastManager and SharedPreferences (by saving state) but sticky broadcast should be used (not abused) whenever they can be. Anyhow I do agree that the security issue is a concern (or might be in near future)

[1] http://www.hpenterprisesecurity.com/vulncat/en/vulncat/java/android_bad_practices_sticky_broadcast.html

[2] https://www.appvigil.co/blog/2015/03/sticky-broadcast-security-vulnerabilities-in-android-apps/

This should use a LocalBroadcastManager for the broadcast, and to save state, why not use the standard Android way of saving and restoring GUI state: https://developer.android.com/training/basics/activity-lifecycle/recreating.html

I really do not want to see any Broadcasts outside of LocalBroadcastManager. It is too easy to mess up security-wise, and its not always apparent what can be a security issue. This is not just some app, it is the app store, so we need to be especially careful.

Status changed to closed

I understand your security concerns, and yes we as of now have no clue what other security issues might arrive, anyhow I am going to close this MR and rewrite another one which implements this using LocalBroadcastManager and saveInstanceState.

thanks for working on this, looking forward to the update. I'm glad you're working on modernizing the GUI :)

@eighthave , my pleasure to help you guys out with as much as I am capable of, It's good learning experience. Not only I love FOSS apps but an FOSS appstore which makes them available to everyone is a dream come true for FOSS app users, the effort required to maintain such application store is overwhelming and the F-Droid team is doing a very good job at it.

Not only is F-Droid one stop for all(almost) the FOSS apps out there but also a good place for newbies(like me) to learn about and to contribute to open source project, through F-Droid I found various interesting projects to contribute to.

I want to put more work into UI (as soon as I have time) mostly animations and such, the only sad thing about animations is backward compatibility, As the material design was introduced in android-21 and higher, the UI changes would be most vibrantly visible on these devices. :(