lemberg / connfa-android

Open Source Android app for Conferences and Events
http://connfa.com
Apache License 2.0
100 stars 111 forks source link

Lock the version of the io.fabric.tools to 1.27.1 #37

Open idimopoulos opened 5 years ago

idimopoulos commented 5 years ago

Disclaimer: I am an amateur developer when it comes to android but I think this is quite basic to get it wrong. I disregarded all proposed updated from the latest version of AndroidStudio but in the very basic build I was getting the error

ERROR: No signature of method: com.crashlytics.tools.gradle.CrashlyticsPlugin.findObfuscationTransformTask() is applicable for argument types: (java.lang.String) values: [ConnfaDebug]

According to https://stackoverflow.com/questions/55214993/error-no-signature-of-method-com-crashlytics-tools-gradle-crashlyticsplugin-fi there is an issue with setting the dependency as io.fabric.tools:gradle:1.+ because the .+ will install a version that apparently breaks it.

I applied the fix from the path above.

Also, I see the v2_dev version already has major changes since v1. Is 1 going to be abandoned? Is it redundant to provide PRs?

TarasKunyk commented 5 years ago

Thanks for your request. Fabric tools version is controlled by fabric plugin and is injected and updated automatically. This code isn't supposed to be updated by developer but you are right - you may lock fabric plugin version however, please, note that your version support might be dropped quite soon (fabric versions aren't usually long living) that's why we left plugin version open.

Regarding your second and third question - branch v2 was a result of crowd development attempt (in fact- PR's) resulting in unstable and hard-to-support code. Unfortunately, PRs require too much resources to keep app stable so it was abandoned.

idimopoulos commented 5 years ago

No I get it actually. The thing with open source is that if there is not any clear goal, it can get messy. Now, when it comes to fabric (from what I understood), usually a newer version, apart from fixes and new features, also include changes in order to adapt to newer versions of packages they work with, e.g. the com.android.tools.build:gradle which is heavily outdated. So a more proper PR would be to actually update all dependencies or at least a group that ties together. However, I am not certain if I would qualify for this since if many people with my experience would show up, it would probably end up in another v2_dev version. I am certain that a dependency update is a more tricky task as it requires also changes in syntax throughout the project as well as possibly changes in methods so I don't know if I will fix it or ruin it. However, since the project is based on older versions, having fabric update to v1.29 currently (the problem exists since v1.28), I think that locking it to v1.27.1 is a bearable solution for now.

Again, as a disclaimer, I have not searched it more than the temple of google and stackoverflow so I do not know if that poses any security threat or not. I will try to generally update to the latest versions but I am pretty sure I will fail miserably :P

idimopoulos commented 5 years ago

As I thought, attempting to move to the latest version of dependencies cause a lot of requested changes. Android studio does provide some very good suggestions and most of the changes feel quite safe, but in the long run, I don't feel confident applying them given my experience on Android.

brittyazel commented 5 years ago

I too had this error and had to hardcode 1.27.1 to get it to build. What is this fabric tools that you are talking about? Is there a simple way to get that set up so I don't have to add this patch?

idimopoulos commented 5 years ago

@brittyazel the repo is quite outdated and normally needs updating multiple packages. These packages also include the android sdk tools. If you try you will see that it also needs to refactor parts of the code and also update some deprecated methods. However, currently the branch is broken due to this so the best and fastest way to deal with this is to lock this version.

brittyazel commented 5 years ago

I've edited quite heavily this package for our purposed for our GUADEC event. I was actually able to completely remove the fabric dependency by moving the Twitter window to an embeded webview rather than a native sdk implementation.

idimopoulos commented 5 years ago

@brittyazel the problem is that in your repo you worked directly in the connfa flavor instead of creating a new one. I was able to cherry pick some of the commits but from a point on, it gets hard to distinguish between personal business decisions. I do not think that changing to a web view however is a change that can be simply taken up in the PR as a change for everyone.

idimopoulos commented 5 years ago

BTW I am also building this for an event in Greece (I read the next GUADEC will take place in Greece), the CEST2019.

brittyazel commented 5 years ago

Hrm, your comment about working in the Connfa flavor isn't correct. We definitely created a GUADEC flavor, and have been working there. We did modify a crap ton in the main directory, though we tried to be as agnostic as possible to make those changes not GUADEC specific.

In our case we had to change the Twitter to a webview, as the current implementation just wouldn't work. I spent a crap ton of time on it, and just couldn't get it to load the tweets, even going so far as to update to TwitterCore 3.3 and rewriting the whole implementation. It was just a total failure

brittyazel commented 5 years ago

I think the most controversial change we made is removing the Sponsors from the app. I REALLY didn't like the way that it was implemented, and we felt it detracted from the UI. Also, I think the best option moving forward for sponsors is to integrate them into the Connfa server instead of hardcoding them into the app itself

idimopoulos commented 5 years ago

Hrm, your comment about working in the Connfa flavor isn't correct.

What do you mean? I see in https://gitlab.gnome.org/Teams/Engagement/Events/connfa/connfa-android/tree/GUADEC/app/src the connfa folder was renamed to guadec. I know you created a separate branch, however, many of these changes are now in renamed directories. Still, some of the patches applied cleanly to my connfa directory regardless of your changes. If you see on my repo, I did not touch the connfa folder apart from the fixes. I simply copied it over to a new directory, cest2019, and moved on from there. I am not saying that there is anything wrong with the approach, just that it is harder to backport.

I think the most controversial change we made is removing the Sponsors from the app.

I actually agree with this. I had to hardcode them but I do agree that it would be much better to be one of the automatic features, even if their presentation remained the same. However, I am going to stick to my previous comment, that for a company, this is a major business decision that needs planning, time, and money. I don't think that it is easy to change. However, I would enjoy a more attractive approach on that, especially since it is a major part of the conferences.

In our case we had to change the Twitter to a webview, as the current implementation just wouldn't work.

I simply removed the menu as I cannot afford the time right now to check it.

brittyazel commented 5 years ago

Right yes, I did just rename the Connfa folder to guadec. It seemed easier than copying it and leaving the other in there. Maybe for the ease of merging changes I shouldn't have done that. Whoopsies!

idimopoulos commented 5 years ago

Actually, false, I did spend some time now to check it. I am not an expert but I can make a couple of assumptions. From what I see in the connfa integration server, there is a setting for the Twitter widget id in the conference settings. If you search a bit in Google you will quickly find that this is not the way to go any more and indeed, the best way is to embed a view in your site/view. The generation can be done through the https://publish.twitter.com/. That leads me to think that indeed, this is also a deprecated thing that should be fixed. I also copied over your work (really thanks as it would take me days since I don't really develop Android) but split off the URL and the heading off to the strings.xml in order to have an abstraction of some kind :)

brittyazel commented 5 years ago

Hrm, we did add in the abstraction. You must have pulled only the first commit. We added the abstraction in the subsequent commits. Sorry! Our commits are super messy!