Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better logging GUI #2694

Closed
wants to merge 4 commits into from
Closed

Conversation

hannesa2
Copy link
Contributor

@hannesa2 hannesa2 commented Nov 1, 2019

It's about to change logging from self-made to Timber.
This PR is the GUI part from owncloud/android-library#275 and it relies on it.

It removes the logic to display log and simply uses a Fragment from logging-lib.

It keeps all previous functions like

  • searching

image

and adds

  • a tab for ownCloud Filelogging view
  • a tab for Logcat view
    • to see what happens on other processes too
    • to log what ownCloud forgot to log
    • within release build type !
  • share log
  • adds filter for Verbose, Debug, Info, Warning, Error

Eg, when you select Info you will see only Info, Warning, Error entries and filters out Verbose, Debug

image

So it helps to investigate, what's wrong with ownCloud.

Btw, I recommend to add Firebase Crashlytics in ownCloud. If you want, I can help you with this task

@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 2, 2019

I found an issue

@hannesa2 hannesa2 closed this Nov 2, 2019
@hannesa2 hannesa2 reopened this Nov 2, 2019
@hannesa2 hannesa2 force-pushed the BetterLoggingGUI branch 2 times, most recently from e4d6994 to 28d76f6 Compare November 4, 2019 06:41
@michaelstingl
Copy link
Contributor

Timber's Apache License 2.0 would work 👍

@hannesa2
Copy link
Contributor Author

hannesa2 commented Nov 4, 2019

@jesmrec jesmrec added this to the 2.14.0 milestone Nov 5, 2019
@davigonz davigonz added the Sprint label Nov 7, 2019
@abelgardep
Copy link
Contributor

abelgardep commented Nov 11, 2019

I have been testing this and looks good, but i found some problems.

  1. If you don't have permissions, it gets some errors, maybe it should check if it has access before start logging to a file.
    Screenshot 2019-11-11 at 08 40 20

  2. I got an error trying to share [FIXED]
    2019-11-11 08:41:46.953 16372-16372/com.owncloud.android.debug E/(LogBaseFragment.kt:203) .sendLogContent(): java.lang.NoSuchFieldException: mail_logger

  3. I got also some
    2019-11-11 08:41:31.522 16372-16372/com.owncloud.android.debug E/RecyclerView: No adapter attached; skipping layout

Pixel 2 v10

@abelgardep
Copy link
Contributor

Btw, I recommend to add Firebase Crashlytics in ownCloud. If you want, I can help you with this task

This could be interesting. You can create an issue to discuss it there 👍

@davigonz
Copy link
Contributor

Btw, I recommend to add Firebase Crashlytics in ownCloud. If you want, I can help you with this task

We're wondering if the integration of ownCloud with this sort of third-party solutions could trigger privacy issues, you know, my ownCloud, my info. We pay much attention to the info we put in the logs but is something to consider anyway, what do you think @michaelstingl ?

Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes requested @hannesa2 👍

Comment on lines 54 to 57
"Owncloud.log",
"search logfile",
"search logcat",
getString(R.string.mail_logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you include this as constants?

Copy link
Contributor

@abelgardep abelgardep Nov 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant hardcoded strings

@michaelstingl
Copy link
Contributor

We're wondering if the integration of ownCloud with this sort of third-party solutions could trigger privacy issues, you know, my ownCloud, my info. We pay much attention to the info we put in the logs but is something to consider anyway, what do you think @michaelstingl ?

Is this SaaS only? Or can we host this ourself? Better open new issue for discussion, and we also should involve @theScrabi – I remember he also proposed a solution.

@hannesa2
Copy link
Contributor Author

One alternative could be https://github.com/Countly I only know the Android part, it's very similar to Firebase. At least, there is a server in GitHub too

@hannesa2 hannesa2 mentioned this pull request Nov 15, 2019
6 tasks
@hannesa2
Copy link
Contributor Author

continue here #2725

@hannesa2 hannesa2 closed this Nov 15, 2019
@hannesa2 hannesa2 deleted the BetterLoggingGUI branch November 15, 2019 20:12
@hannesa2
Copy link
Contributor Author

I have been testing this and looks good, but i found some problems.

1. If you don't have permissions, it gets some errors, maybe it should check if it has access before start logging to a file.
   ![Screenshot 2019-11-11 at 08 40 20](https://user-images.githubusercontent.com/47524927/68570252-800e1700-0460-11ea-97b3-0b9d537f848a.png)

2. I got an error trying to share [FIXED]
   `2019-11-11 08:41:46.953 16372-16372/com.owncloud.android.debug E/(LogBaseFragment.kt:203) .sendLogContent(): java.lang.NoSuchFieldException: mail_logger `

3. I got also some
   `2019-11-11 08:41:31.522 16372-16372/com.owncloud.android.debug E/RecyclerView: No adapter attached; skipping layout `

Pixel 2 v10

  1. It's because you show File Activity with write-permission request there, and create on top the login screen. That's why, the user has no chance to accept permission and you start to log immediate. This is only in debug, on release, the pro-user ! has before to enable developer menu and it will not happen in release.
    I want to avoid a permission check on every log entry. This is not performant
  2. solved
  3. solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants