rovo89 / XposedBridge

The Java part of the Xposed framework.
3.91k stars 1.1k forks source link

Limit log size #17

Closed rovo89 closed 10 years ago

rovo89 commented 10 years ago

As requested by @neniuideo in https://github.com/rovo89/XposedInstaller/issues/186 and often on XDA, it would be good to limit the log size in order to reduce the side-effects of incompatible modules.

My answer to that: http://forum.xda-developers.com/xposed/framework-xposed-rom-modding-modifying-t1574401/post52824136

pylerSM commented 10 years ago

Many people complains about huge log file (especially now, when some modules are not fully compatible with 2.6). I think size limit is good thing, and maybe Xposed should show warning about "too big log file" and provide option to clear it from dialog.

rovo89 commented 10 years ago

Yes, it's on the roadmap for some future version, and it would only make sense if the user is warned about it:

I will consider a cut-off at a certain size, like 5 MB. That's already very much, it should usually be about 1-2 kB only. Of course, the user would have to be warned about this, e.g. by a red text in the welcome screen when more than 3 MB have been exceeded. But definitely "no" to making the log switchable or even off by default.

Simply clearing the log isn't a solution though, the issue with the module(s) must be fixed.

pylerSM commented 10 years ago

Best solution would be find broken module, deactivate it & ask user to contact dev.

rovo89 commented 10 years ago

I wouldn't go as far as defining a module as "broken" or even deactivate it. Especially because such a detection is hardly reliable when only a stack trace is available.

However, I thought about detecting the package names of installed modules in the log file and highlight them. Maybe a click on them could then display a toast with the title of the module. That leaves the interpretation of the situation to the user (which I think is good), but provides a little help to unexperienced users who might think that the framework is broken.

pylerSM commented 10 years ago

Maybe put highlighted package name (or uid) before anything logged using XposedBridge.log() ? lemme show how I imagine that..

Xposed log with package uids: com.aaa.example: isXZSystemFeatureEnabled=false com.bbb.example: isActivated=true

Xposed log with package names: CoolestModule: isXZSystemFeatureEnabled=false AwesomeModule: isActivated=true

I think second option is better, users can see immediatelly which module has problems.

Hm, if this is enabled, we can show per module logs using filtering (click on module, show module logs?) with save option (if we have metadata for email, we can ask user to send log to module developer (provide API for module developer will be fantastic... simply awesome but needs too much work)

But... i think you disagree with this :D ...

rovo89 commented 10 years ago

Well, first of all, the log is meant only for errors or unexpected situations. Otherwise, it will end up like logcat, which is flooded with information.

Nevertheless, it would be great to see which module caused a certain error. Unfortunately, I think the framework could (at most) guess that. Or do you have any idea how this could be determined?

pylerSM commented 10 years ago

Let's look on this from other side...

What about compability tags? When uploading module, xposed module dev will select Xposed versions, which are compatible with his module. This can fix many problems. I think before releasing new Xposed version, all xposed devs should get private beta version of Xposed to test their modules. If they tested their modules and everything is OK, they can add Xposed vX.X between compatible Xposed releases. If you are trying download module and module is incompatible with your Xposed version, download will simply fail and it will show dialog about incompatibility.

rovo89 commented 10 years ago

Developers already need to specify the minimum required Xposed version in their module. Specifying a maximum version would be counter-productive because you would have to adjust it everytime a new Xposed version is published. Xposed's API is very stable, so this would be much effort with almost zero gain. Even the "illegal access" exceptions happened only because some modules relied on a side-effect and didn't use the helpers provided in the API. This was noticable because some widely used modules were affected, but many others didn't need to be updated.

Anyway, please keep it to the topic. This issue is only about limiting the log size. Further suggestions are welcome, but only as a separate issue.

theknut commented 10 years ago

This sounds rather hacky but you could use something like !startsWith("at android.") && !contains("de.robv.android.xposed.") && contains(".java:")?! at de.robv.android.xposed.XposedHelpers.findClass(XposedHelpers.java:52) at de.depressiverobot.android.xposed.hideappsxposed.HideAppsXposed.handleLoadPackage(HideAppsXposed.java:70) at de.robv.android.xposed.IXposedHookLoadPackage$Wrapper.handleLoadPackage(IXposedHookLoadPackage.java:20) So in this case the condition would be true for at de.depressiverobot.android.xposed.hideappsxposed.HideAppsXposed.handleLoadPackage(HideAppsXposed.java:70) You could then remove the "at " and read behind the first word starting with capital letters until the next dot. Oh my... explaining it already shows how stupid this idea is...

I actually wouldn't even do anything like this since some modules are not well coded and for example hook classes which are in the support library which is included in every app using it. So every app would be hooked but only one app is actually to be modified. These mods are the reason for such logfiles I guess.

If you go through the entire error log only to determine the packagename sounds like a very heavy operation. Most users don't even look the log anyways. The one who look at it already know how to read it properly. So I think you are wasting your time highlighting anything in there. Of course it would make it easier for noobs but I think it's not worth the effort.

Just my two cents...

On Thu, Jun 5, 2014 at 7:30 PM, rovo89 notifications@github.com wrote:

Developers already need to specify the minimum required Xposed version in their module. Specifying a maximum version would be counter-productive because you would have to adjust it everytime a new Xposed version is published. Xposed's API is very stable, so this would be much effort with almost zero gain. Even the "illegal access" exceptions happened only because some modules relied on a side-effect and didn't use the helpers provided in the API. This was noticable because some widely used modules were affected, but many others didn't need to be updated.

Anyway, please keep it to the topic. This issue is only about limiting the log size. Further suggestions are welcome, but only as a separate issue.

— Reply to this email directly or view it on GitHub https://github.com/rovo89/XposedBridge/issues/17#issuecomment-45249317.

pylerSM commented 10 years ago

rovo89, simply deprecate XposedBridge.log(text) and create XposedBridge.logError(text) so it will be more understandable for devs (I suppose) that it isn't logcat like API.