ericcornelissen / NervousFish

An app for your :iphone: to exchange public-keys in a secure manner.
GNU Lesser General Public License v3.0
2 stars 4 forks source link

Add SLF4J and Logback as logging facilities, closes #38 #85

Closed jverbraeken closed 7 years ago

jverbraeken commented 7 years ago

What

This PR adds logging facilities to the project by adding SLF4J, the most popular logging facade for Java, and Logback, the successor of the very popular Log4J and arguably the best free industry-grade logger for Java.

Why

This PR is needed because we want to be able to trace down where bugs are when the program suddenly crashes and because we want to be able to be notified of warnings that don't crash the program or are useful to the user.

How

This feature can be tested by just running the app and inspecting the logcat (for example by opening Android Monitor)

Alternative implementation

SLF4J and Logback are both industry-standard and the most popular logging tools at the moment. People are happy about it and I didn't see any complains about in on the internet. Because logging is a relatively unimportant aspect of our program I don't think it's worth the time to look for even better alternatives as these are a safe bet already.

Notes

-

jverbraeken commented 7 years ago

@TheBonheurs @ericcornelissen Thanks for the suggestion! I made a document containing some guidelines here: https://docs.google.com/document/d/1g62VM3IgYDweTrvEwqNFFspnIivzrymEIT7UpBHnMeA However, I think that part of the logging is at places where your intuition thinks it should be. For example, I would log it when a button is pressed, but I wouldn't log it when the user scrolls thought a list of items.

jverbraeken commented 7 years ago

@ericcornelissen I'm aware that such changes are not preferred in a logging PR, but I had to fix them because otherwise Gradle check would fail each time because of Android Lint. I don't know how the failing code could appear on the master, maybe Travis skips Android Lint, but these issues had to be fixed to let my local check succeed.

ericcornelissen commented 7 years ago

@jverbraeken I'm pretty sure it is because you removed this

jverbraeken commented 7 years ago

@ericcornelissen Actually not, that line of code was duplicate, because it was also (correctly) in quality.gradle

ericcornelissen commented 7 years ago

@jverbraeken are you aware that you added it?

jverbraeken commented 7 years ago

@ericcornelissen Ah, I see, thought you were talking about a different line of code. I added it because both issues were not relevant, but although I suppressed both warnings I still got quite some Android Lint warnings that I had to solve before my local build succeeded

ericcornelissen commented 7 years ago

@jverbraeken no problem 🙂 still, it would have been better to put it in a different pull request...

jverbraeken commented 7 years ago

@ericcornelissen Next time I'll create a different PR to merge into this branch. Is the rest of the PR OK for now?