Better logging gui #2725

hannesa2 · 2019-11-15T20:09:52Z

It's the #2694 but on ownCloud repo, instead of my fork

Implements #2730
Needs owncloud/android-library#283

QA

Test plan: https://github.com/owncloud/QA/blob/master/Mobile/Android/Release_2.14.0/2725%20-%20Timber.md

(1) Searching is case sensitive Better logging gui #2725 (comment) [NOT REPRODUCIBLE]
(2) Sharing logs with ownCloud Better logging gui #2725 (comment) [FIXED]
(3) name of the log file should be branded [FIXED]
(4) Log rotation improvements Better logging gui #2725 (comment) [FIXED]
(5) Share icon is used as clear log option when the clear log option has enough room in top bar. Better logging gui #2725 (comment) [FIXED]
(6) [improvement] Add timestamp to log files name Better logging gui #2725 (comment) [WONT DO HERE]

abelgardep · 2019-11-18T11:32:54Z

I have found some problems @hannesa2 .

1- I got an infinity loop clearing logfile

2- stopLogging is not used anywhere

3- Some of these problems are still reproducible (It would be interesting to solve them)
#2694 (comment)

hannesa2 · 2019-11-20T06:55:38Z

I got an infinity loop clearing logfile -> fixed
stopLogging is not used anywhere -> I kept it, because when someone has the idea to re-intoduce stopLogging. If I immediate delete it, I expect the call for it
I write there

abelgardep

Ok, thanks @hannesa2 . Moving this forward

abelgardep · 2019-11-20T07:12:23Z

Ready to QA @jesmrec

jesmrec · 2019-11-20T11:53:53Z

@hannesa2 it is very appreciated. Also, it'd be appreciated if the library PR: owncloud/android-library#275 is moved to the oC repo as well.

jesmrec · 2019-11-20T12:09:46Z

(1) [NR]

The log searching is case-sensitive. This is a bit tricky, since the main target is getting matches, regardless if capitals are there or not. Is it feasible to set the searching as non case sensitive or is it a Timber feature?

jesmrec · 2019-11-20T12:25:12Z

(2) [FIXED]

Open the Logs view
Share the logs with ownCloud

Current: nothing happens
Expected: folder picker?

This took my attention, since other apps in the same device displays the folder picker to share with ownCloud, but the Log app. But, not sure it this is a matter of the library itself. Sharing with other available providers works properly.

jesmrec · 2019-11-20T13:10:06Z

owncloudApp/src/main/java/com/owncloud/android/ui/activity/LogHistoryActivity.kt

+    companion object {
+        private const val logCatTargetFileName = "logfile.log"
+        private const val logCatSearchHint = "search logcat"
+        private const val bothLogsTargetFileName = "Owncloud.log"


Please, don't use Owncloud as hardcoded string inside the code, due to branding reasons. You can use the app_name or other parameter.

ok, let's call it log.log (or something other less-meaningful) before it was

even current and older hasn't that much meaningful name

app_name needs context , so companion object is not the best place anymore then

But the branch is in your repo, you can do what you want

jesmrec · 2019-11-20T13:15:52Z

(4) [FIXED]

About log rotation, every day is created a new file. Long term will flood the app folder with logs. Any way/chance to clean logs (automatically or manually)?

Logs are stored in the root folder of the app in the internal storage. Shouldn't be stored in a specific folder called Logs? (feasible?)

additional ideas welcome

hannesa2 · 2019-11-20T13:39:16Z

(2)

Open the Logs view

Share the logs with ownCloud

Current: nothing happens
Expected: folder picker?

This took my attention, since other apps in the same device displays the folder picker to share with ownCloud, but the Log app. But, not sure it this is a matter of the library itself. Sharing with other available providers works properly.

No need for a file picker anymore. It has just one recent file

hannesa2 · 2019-11-20T13:47:48Z

(4)

About log rotation, every day is created a new file. Long term will flood the app folder with logs. Any way/chance to clean logs (automatically or manually)?

Logs are stored in the root folder of the app in the internal storage. Shouldn't be stored in a specific folder called Logs? (feasible?)

additional ideas welcome

This file logging is pointless since you see in release log. And additional you can share logcat with others.
Why is a file needed ?

But about flooting: Maybe we should delete all *.log files in that directory during clear log ?

hannesa2 · 2019-11-20T13:49:26Z

(1)

The log searching is case-sensitive. This is a bit tricky, since the main target is getting matches, regardless if capitals are there or not. Is it feasible to set the searching as non case sensitive or is it a Timber feature?

It's the behaviour like before. Make an issue and I apply it later.
Please don't expand requirements, otherwise it will be an endless story

jesmrec · 2019-11-20T14:51:35Z

(1) -> in the previous log implementation, searching was not case sensitive. For example:

(2)

No need for a file picker anymore. It has just one recent file

if you open the Log view and you want to sync it in your oC server by sharing, you can't (in the former implementation you could). The option is there and does nothing. IMHO is confusing if you select ownCloud in the picker and nothing happens. Every action should have a reaction. It is not a matter of watching the file, but storing the file.

(3) -> will check the new names. pending.

(4)

Maybe we should delete all *.log files in that directory during clear log ?

that works for me. What do you think @abelgardep @davigonz

jesmrec · 2019-11-20T15:10:35Z

@hannesa2 in the report (1) you are right, ignore my answer, since the word chunk appears twice in the line, one in uppercase and other in lowercase. former filter worked in case sensitive as well.

So, the behaviour is the same and we are not losing functionality. We can go on with it.

hannesa2 · 2019-11-21T21:01:52Z

(2)

A: during investigation, I solved a lot of lint issues, but my experience with #2680 told me not to push this, because it causes too much afford. You prefer to go with a bad current state, instead of an improvement which not fit 100% concerning automatic code format

B: file chooser
By the way this use case ! Ok, it should work.
I guess @davigonz knows, what the sense of this prevention is. (It blocks share from oC to oC)
I removed it, and it works. But please change this by your own. I see no sense in it

hannesa2 · 2019-11-22T06:41:26Z

(4)

Maybe we should delete all *.log files in that directory during clear log ?

that works for me. What do you think @abelgardep @davigonz

When you want to go this way, I'm ready prepared

abelgardep · 2019-11-22T08:45:32Z

(4)

Maybe we should delete all *.log files in that directory during clear log ?

that works for me. What do you think @abelgardep @davigonz

When you want to go this way, I'm ready prepared

Go ahead 👍

davigonz · 2019-11-22T13:14:25Z

A: during investigation, I solved a lot of lint issues, but my experience with #2680 told me not to push this, because it causes too much afford. You prefer to go with a bad current state, instead of an improvement which not fit 100% concerning automatic code format

About #2680 , as @abelgardep pointed in #2680 (comment) , there's some automatic transformations from Java to Kotlin that are a bit dangerous, mostly related to nullability.

causes too much afford

Do you mean effort? Code reviews cause effort, it's something usual in a development process, you might probably know that. We do not merge code as it is, if some changes are needed, they should be applied. Otherwise, we will pile up not 100% proper code and it will end up in something not that good.

I guess @davigonz knows, what the sense of this prevention is. (It blocks share from oC to oC)
I removed it, and it works. But please change this by your own. I see no sense in it

Yes, I can not give the details about that change but it was needed. I will have a look at it anyway.

By the way, I've noticed that there's a duplicated share icon when I install it in a Nexus 6P, any idea?

hannesa2 · 2019-11-22T22:41:06Z

(4)

Go ahead 👍

@abelgardep done

hannesa2 · 2019-11-22T22:45:23Z

By the way, I've noticed that there's a duplicated share icon when I install it in a Nexus 6P, any idea?

Sorry, I've no such device.

I add this share for each fragment separately with

override fun onCreateOptionsMenu(menu: Menu?, inflater: MenuInflater?) {

    inflater?.inflate(R.menu.menu_log, menu)

My expectation is that it's only added for recent fragment

hannesa2 · 2019-11-26T05:48:55Z

Before:

git push upstream

Now:

This has nothing to do with upper/lowercase branch names. You can call it, what you want, simply sha1 rules

davigonz · 2019-11-26T07:53:03Z

@hannesa2

Yesterday I performed the next steps in your library repo:

git checkout BetterLogging
git fetch
git rebase

And the result was what you see in the image below

The remote tracking reference is origin/BetterLogging and it was containing the commits of your remote betterLogging branch (increase cache size, split log into debug and release) instead of the commits of your remote BetterLogging branch. No idea about what was going on under the hood, but is the first time I see something like this. Maybe I'm missing something.

davigonz · 2019-11-26T09:30:25Z

(5) [FIXED]

Share icon appears as the clear log option when the clear log option has enough room to appear in the top bar.

The fix is included via AppDevNext/Logcat#29 , could you check it @hannesa2 ? Thanks

davigonz · 2019-11-26T15:56:51Z

@jesmrec all the bugs should already be fixed, could you check them?

jesmrec · 2019-11-27T08:45:42Z

(6) (improvement)

After fixing (2) and (3), the name of the logs file to share is always

<app name>.log

would it be posible to add the timestamp (same as the log stored in device). So, if some files are stored, can be distinguised. Something like

_20191127_101145.log

what do you think?

hannesa2 · 2019-11-27T12:19:19Z

owncloudApp/src/main/java/com/owncloud/android/ui/activity/LogHistoryActivity.kt

@@ -51,7 +50,7 @@ class LogHistoryActivity : ToolbarActivity() {

            fileLoggingTree()?.let {
                logFragment = BothLogsFragment.newInstance(
-                    bothLogsTargetFileName,
+                    "${getString(R.string.app_name)}.log",


I didn't debugged it, but is it that smart to use R.string.app_name ? What's about when the app has a name like "my Cloud" with space ?
Leave it like it was and all is fine

I tried it and no problem at all with spaces in the log to share

hannesa2 · 2019-11-27T12:23:01Z

(6) (improvement)

would it be posible to add the timestamp (same as the log stored in device). So, if some files are stored, can be distinguised. Something like

_20191127_101145.log

what do you think?

https://github.com/hannesa2/Logcat/blob/5cba849239a0d71ef21f69d91c986c875c16b580/LogcatLib/src/main/java/info/hannes/timber/FileLoggingTree.kt#L23-L28
I don't recommend it, then the name will be _20191127_101145.2019-11-27.log ...please not

Maybe this on https://github.com/owncloud/android-library/pull/283/files#r351259590 ? But it's up to you. For me it was fine, everything now makes it not better

jesmrec · 2019-11-27T13:03:05Z

sorry, GH text processor played me a trick.

What i meant, is using the name of the app as name of the file (what i called there as "<nameApp>.log" and GH ellipsized):

ownCloud.log
MyApp.log
MyCompany.log

depending on the parameter in setup.xml for the appName.

so, the improvement will change those names for

ownCloud_20191127_101145.log
MyApp_20191127_101145.log
MyCompany_20191127_101145.log

sorry for not reviewing the published comment

davigonz · 2019-11-27T14:55:56Z

ownCloud_20191127_101145.log
MyApp_20191127_101145.log
MyCompany_20191127_101145.log

@jesmrec This is not that trivial so I would keep it as it is, just appname.log . The dates and time appear inside the log files.

jesmrec · 2019-11-27T15:05:34Z

Ok, let's keep it ftm... awaiting for more feedback.

From my side this is approved.

Thanks for the contribution @hannesa2!

…. log files

hannesa2 mentioned this pull request Nov 15, 2019

Better logging GUI #2694

Closed

jesmrec added Estimation - 3 (M) Sprint labels Nov 18, 2019

abelgardep mentioned this pull request Nov 18, 2019

[FEATURE REQUEST] Logging with Timber #2730

Closed

abelgardep removed Estimation - 3 (M) Sprint labels Nov 18, 2019

abelgardep approved these changes Nov 20, 2019

View reviewed changes

abelgardep added the code approved label Nov 20, 2019

abelgardep assigned hannesa2 Nov 20, 2019

jesmrec self-requested a review November 20, 2019 11:51

jesmrec requested changes Nov 20, 2019

View reviewed changes

hannesa2 mentioned this pull request Nov 21, 2019

Better logging (on upstream) owncloud/android-library#283

Merged

davigonz force-pushed the BetterLoggingGUI branch from 8138a20 to dfe1ae8 Compare November 26, 2019 15:49

hannesa2 commented Nov 27, 2019

View reviewed changes

jesmrec added the Approved by qa label Nov 27, 2019

jesmrec self-requested a review November 27, 2019 15:06

jesmrec approved these changes Nov 27, 2019

View reviewed changes

hannesa2 and others added 10 commits November 27, 2019 16:10

multidex

e085d62

better logging

0b75538

remove some pointless warnings

5601b69

apply review

553075a

Move hardcoded strings to const val and revert some changes

224db33

fix progress dialog and 'No adapter attached'

421ed09

delete all *.log in directory

a5e649f

Allow sharing external storage files included in ownCloud folder, e.g…

073c8e8

…. log files

Make the logfile name used when sharing brandable

f285ca9

Use new version of the library that fixes clear log icon

040fec0

davigonz force-pushed the BetterLoggingGUI branch from dfe1ae8 to 040fec0 Compare November 27, 2019 15:11

davigonz merged commit fe4c3c0 into master Nov 27, 2019

delete-merged-branch bot deleted the BetterLoggingGUI branch November 27, 2019 17:54

jesmrec mentioned this pull request Nov 28, 2019

Adjustments in Settings UI tests to avoid flakyness #2737

Merged

hannesa2 mentioned this pull request Dec 1, 2019

Reduce logging in ChunkFromFileRequestBody #2743

Closed

9 tasks

davigonz mentioned this pull request Dec 2, 2019

lint check #2736

Closed

Better logging gui #2725

Better logging gui #2725

Conversation

hannesa2 commented Nov 15, 2019 • edited by jesmrec Loading

abelgardep commented Nov 18, 2019 • edited Loading

hannesa2 commented Nov 20, 2019

abelgardep left a comment

Choose a reason for hiding this comment

abelgardep commented Nov 20, 2019

jesmrec commented Nov 20, 2019

jesmrec commented Nov 20, 2019 • edited Loading

(1) [NR]

jesmrec commented Nov 20, 2019 • edited Loading

(2) [FIXED]

jesmrec Nov 20, 2019

Choose a reason for hiding this comment

hannesa2 Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

jesmrec commented Nov 20, 2019 • edited Loading

(4) [FIXED]

hannesa2 commented Nov 20, 2019

(2)

hannesa2 commented Nov 20, 2019

(4)

hannesa2 commented Nov 20, 2019

(1)

jesmrec commented Nov 20, 2019

jesmrec commented Nov 20, 2019

hannesa2 commented Nov 21, 2019 • edited Loading

hannesa2 commented Nov 22, 2019

abelgardep commented Nov 22, 2019

davigonz commented Nov 22, 2019

hannesa2 commented Nov 22, 2019

hannesa2 commented Nov 22, 2019

hannesa2 commented Nov 26, 2019

davigonz commented Nov 26, 2019 • edited Loading

davigonz commented Nov 26, 2019 • edited by jesmrec Loading

(5) [FIXED]

davigonz commented Nov 26, 2019

jesmrec commented Nov 27, 2019 • edited by davigonz Loading

(6) (improvement)

hannesa2 Nov 27, 2019

Choose a reason for hiding this comment

davigonz Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

hannesa2 commented Nov 27, 2019 • edited Loading

(6) (improvement)

jesmrec commented Nov 27, 2019

davigonz commented Nov 27, 2019 • edited Loading

jesmrec commented Nov 27, 2019

hannesa2 commented Nov 15, 2019 •

edited by jesmrec

Loading

abelgardep commented Nov 18, 2019 •

edited

Loading

jesmrec commented Nov 20, 2019 •

edited

Loading

jesmrec commented Nov 20, 2019 •

edited

Loading

hannesa2 Nov 20, 2019 •

edited

Loading

jesmrec commented Nov 20, 2019 •

edited

Loading

hannesa2 commented Nov 21, 2019 •

edited

Loading

davigonz commented Nov 26, 2019 •

edited

Loading

davigonz commented Nov 26, 2019 •

edited by jesmrec

Loading

jesmrec commented Nov 27, 2019 •

edited by davigonz

Loading

davigonz Nov 27, 2019 •

edited

Loading

hannesa2 commented Nov 27, 2019 •

edited

Loading

davigonz commented Nov 27, 2019 •

edited

Loading