-
Notifications
You must be signed in to change notification settings - Fork 731
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
Feature/fga/log tags voip #3723
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few remark/question
object VOIP : LoggerTag("VOIP", null) | ||
|
||
val value: String | ||
get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Affect instead of get()
to avoid computation each time it's called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, you're right
@@ -180,12 +183,18 @@ internal class CallSignalingHandler @Inject constructor(private val activeCallHa | |||
if (event.roomId == null || event.senderId == null) { | |||
return | |||
} | |||
val now = System.currentTimeMillis() | |||
val age = now - (event.ageLocalTs ?: now) | |||
if (age > 40_000 && event.getClearType() == EventType.CALL_INVITE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
40_000
: define a const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
import timber.log.Timber | ||
import java.util.HashSet | ||
|
||
private val loggerTag = LoggerTag("API21AudioDeviceDetector", LoggerTag.VOIP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using API21AudioDeviceDetector::class.simpleName
instead of hard coded string? For proguard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
@@ -22,16 +22,13 @@ package org.matrix.android.sdk.api.logger | |||
* val loggerTag = LoggerTag("MyTag", LoggerTag.VOIP) | |||
* Timber.tag(loggerTag.value).v("My log message") | |||
*/ | |||
open class LoggerTag(private val _value: String, private val parentTag: LoggerTag? = null) { | |||
open class LoggerTag(_value: String, parentTag: LoggerTag? = null) { | |||
|
|||
object VOIP : LoggerTag("VOIP", null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, null
could be ommited but I can live with that :)
@@ -185,7 +186,7 @@ internal class CallSignalingHandler @Inject constructor(private val activeCallHa | |||
} | |||
val now = System.currentTimeMillis() | |||
val age = now - (event.ageLocalTs ?: now) | |||
if (age > 40_000 && event.getClearType() == EventType.CALL_INVITE) { | |||
if (age > MAX_AGE_TO_RING) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not checking the type anymore? EDIT: I guess it's doen before, else we should not be in this fun, sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, next time does not forget the file for the changelog. I will ad it on develop
Add some Parent class to add handler logger tags more properly.
Start branching that for Voip logs/
Also make end call notification disappear quickly if possible.