Skip to content

Improve logging by placing logs in core functions #96

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.

Sign up for GitHub

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

Merged
tuancoltech merged 4 commits into from
Jul 30, 2025
Merged

Improve logging by placing logs in core functions #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ab66de
Improve logging by placing logs in core functions
tuancoltech Jul 16, 2025
b21dff6
Minor fix
shubertm Jul 29, 2025
1120802
Fix ktlint
shubertm Jul 29, 2025
2342409
Fix ktlint
shubertm Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/media/ArkAudioRecorderImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,22 @@ class ArkAudioRecorderImpl
}

override fun start() {
Log.d(tag, "start")
recorder?.start()
}

override fun pause() {
Log.d(tag, "pause")
recorder?.pause()
}

override fun resume() {
Log.d(tag, "resume")
recorder?.resume()
}

override fun reset() {
Log.d(tag, "reset")
recorder?.reset()
}

Expand All @@ -68,6 +72,7 @@ class ArkAudioRecorderImpl
return try {
recorder?.maxAmplitude ?: 0
} catch (e: Exception) {
Log.e(tag, "maxAmplitude exception: $e")
0
}
}
Expand All @@ -76,6 +81,7 @@ class ArkAudioRecorderImpl

override suspend fun deleteTempFile(): Boolean {
return try {
Log.d(tag, "deleteTempFile")
tempFile.delete()
} catch (e: Exception) {
Log.e(tag, "deleteTempFile exception: " + e.message)
Expand Down
11 changes: 10 additions & 1 deletion app/src/main/java/dev/arkbuilders/arkmemo/media/ArkMediaPlayerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ class ArkMediaPlayerImpl

private var maxAmplitude = 0

companion object {
private const val TAG = "ArkMediaPlayerImpl"
}

override fun init(
path: String,
onCompletion: () -> Unit,
onPrepared: () -> Unit,
) {
Log.d(TAG, "init")
if (player?.isPlaying == true) {
player?.stop()
onCompletionHandler()
Expand All @@ -43,16 +48,18 @@ class ArkMediaPlayerImpl
setDataSource(path)
prepare()
} catch (e: Exception) {
Log.e("ArkMediaPlayerImpl", "init exception: ${e.message}")
Log.e(TAG, "init exception: ${e.message}")
}
}
}

override fun play() {
Log.d(TAG, "play")
player?.start()
}

override fun stop() {
Log.d(TAG, "stop")
player?.let {
it.stop()
it.release()
Expand All @@ -61,10 +68,12 @@ class ArkMediaPlayerImpl
}

override fun pause() {
Log.d(TAG, "pause")
player?.pause()
}

override fun seekTo(position: Int) {
Log.d(TAG, "seekTo position: $position")
player?.seekTo(position)
}

Expand Down
14 changes: 11 additions & 3 deletions app/src/main/java/dev/arkbuilders/arkmemo/repo/NotesRepoHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ class NotesRepoHelper
) {
lateinit var root: Path

companion object {
private const val TAG = "NotesRepoHelper"
}

private lateinit var propertiesStorage: PropertiesStorage
private val lazyPropertiesStorage by lazy {
CoroutineScope(iODispatcher).async {
Expand All @@ -43,6 +47,7 @@ class NotesRepoHelper
}

suspend fun init(root: String) {
Log.d(TAG, "init")
this.root = Path(root)
propertiesStorage = lazyPropertiesStorage.await()
}
Expand All @@ -53,6 +58,7 @@ class NotesRepoHelper
description: String? = null,
): Boolean {
with(propertiesStorage) {
Log.d(TAG, "persistNoteProperties for resource: $resourceId title: $noteTitle")
val properties =
Properties(
setOf(noteTitle),
Expand Down Expand Up @@ -85,13 +91,14 @@ class NotesRepoHelper
extension = resourcePath.extension,
modified = resourcePath.getLastModifiedTime(),
)
Log.d("notes-repo", "resource renamed to ${resourcePath.name} successfully")
Log.d(TAG, "resource renamed to ${resourcePath.name} successfully")
}

fun readProperties(
id: ResourceId,
defaultTitle: String,
): UserNoteProperties {
Log.d(TAG, "readProperties for resource id: $id")
val title =
propertiesStorage.getProperties(id).titles.let {
if (it.isNotEmpty()) it.elementAt(0) else defaultTitle
Expand All @@ -112,6 +119,7 @@ class NotesRepoHelper

suspend fun deleteNote(note: Note): Unit =
withContext(Dispatchers.IO) {
Log.d(TAG, "deleteNote: ${note.title}")
val id = note.resource?.id

val path = root.resolve("${note.resource?.name}")
Expand All @@ -120,13 +128,13 @@ class NotesRepoHelper
try {
propertiesStorage.remove(resourceId)
} catch (ex: NullPointerException) {
Log.e("NotesRepoHelper", "deleteNote exception: " + ex.message)
Log.e(TAG, "deleteNote exception: " + ex.message)
}
}

propertiesStorage.persist()
note.resource?.name?.let { name ->
Log.d("NotesRepoHelper", "$name has been deleted. id: " + id)
Log.d(TAG, "$name has been deleted. id: " + id)
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/repo/graphics/GraphicNotesRepo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class GraphicNotesRepo
note: GraphicNote,
callback: (SaveNoteResult) -> Unit,
) = withContext(iODispatcher) {
Log.d(GRAPHICS_REPO, "write")
val tempPath = createTempFile()
note.svg?.generate(tempPath)
val size = tempPath.fileSize()
Expand Down Expand Up @@ -115,6 +116,7 @@ class GraphicNotesRepo

private suspend fun readStorage() =
withContext(iODispatcher) {
Log.d(GRAPHICS_REPO, "readStorage")
root.listFiles(SVG_EXT) { path ->
val svg = SVG.parse(path)
if (svg == null) {
Expand Down Expand Up @@ -147,6 +149,7 @@ class GraphicNotesRepo
fileName: String,
svg: SVG?,
): Bitmap? {
Log.d(GRAPHICS_REPO, "exportBitmapFromSvg")
// Check if thumb bitmap already exists
val file = File(thumbDirectory, "$fileName.png")
try {
Expand Down Expand Up @@ -189,6 +192,7 @@ class GraphicNotesRepo
canvas.drawPath(path.path, path.paint)
canvas.restore()
} ?: let {
Log.w(GRAPHICS_REPO, "exportBitmapFromSvg either SVG or its paths are null!")
return null
}

Expand Down
3 changes: 3 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/repo/text/TextNotesRepo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class TextNotesRepo
note: TextNote,
callback: (SaveNoteResult) -> Unit,
) = withContext(iODispatcher) {
Log.d(TEXT_REPO, "write note: $note")
val tempPath = createTempFile()
val lines = note.text.split('\n')
tempPath.writeLines(lines)
Expand Down Expand Up @@ -96,6 +97,7 @@ class TextNotesRepo

private suspend fun readStorage(): List<TextNote> =
withContext(iODispatcher) {
Log.d(TEXT_REPO, "readStorage")
root.listFiles(NOTE_EXT) { path ->
val size = path.fileSize()
val id = computeId(size, path)
Expand Down Expand Up @@ -123,6 +125,7 @@ class TextNotesRepo
)
}
} catch (e: Exception) {
Log.e(TEXT_REPO, "readStorage exception: $e")
e.printStackTrace()
TextNote(
text = "",
Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/repo/voices/VoiceNotesRepo.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class VoiceNotesRepo
note: VoiceNote,
callback: (SaveNoteResult) -> Unit,
) = withContext(iODispatcher) {
Log.d(VOICES_REPO, "write")
val tempPath = note.path
val size = tempPath.fileSize()
val id = computeId(size, tempPath)
Expand Down Expand Up @@ -100,6 +101,7 @@ class VoiceNotesRepo

private suspend fun readStorage(): List<VoiceNote> =
withContext(iODispatcher) {
Log.d(VOICES_REPO, "readStorage")
root.listFiles(VOICE_EXT) { path ->
val id = computeId(path.fileSize(), path)
val resource =
Expand Down
7 changes: 6 additions & 1 deletion app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package dev.arkbuilders.arkmemo.ui.activities

import android.content.Intent
import android.os.Bundle
import android.util.Log
import android.view.WindowManager
import androidx.activity.result.contract.ActivityResultContracts
import androidx.annotation.IdRes
Expand Down Expand Up @@ -41,6 +42,7 @@ class MainActivity : AppCompatActivity(R.layout.activity_main) {
init {
FilePickerDialog.readPermLauncher =
registerForActivityResult(ActivityResultContracts.RequestPermission()) { isGranted ->
Log.d(ACTIVITY_TAG, "readPermLauncher isGranted: $isGranted")
if (isGranted) {
FilePickerDialog.show()
} else {
Expand All @@ -50,6 +52,7 @@ class MainActivity : AppCompatActivity(R.layout.activity_main) {

FilePickerDialog.readPermLauncherSdkR =
registerForActivityResult(PermissionContract()) { isGranted ->
Log.d(ACTIVITY_TAG, "readPermLauncherSdkR isGranted: $isGranted")
if (isGranted) {
FilePickerDialog.show()
} else {
Expand All @@ -68,6 +71,7 @@ class MainActivity : AppCompatActivity(R.layout.activity_main) {
}

supportFragmentManager.onArkPathPicked(this) {
Log.d(ACTIVITY_TAG, "onArkPathPicked path: $it")
memoPreferences.storePath(it.toString())
showFragment(savedInstanceState)
}
Comment on lines 73 to 77

@coderabbitai coderabbitai Bot Jul 22, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Full storage path logged – possible privacy issue
onArkPathPicked path: $it prints the absolute directory chosen by the user. On shared devices this leaks personal folder structure. Limit to last path segment or guard with BuildConfig.DEBUG.

🤖 Prompt for AI Agents 
In app/src/main/java/dev/arkbuilders/arkmemo/ui/activities/MainActivity.kt
around lines 73 to 77, the log statement prints the full absolute path chosen by
the user, which can expose sensitive directory information. Modify the log to
either only print the last segment of the path or wrap the logging statement
inside a check for BuildConfig.DEBUG to ensure it only logs in debug builds,
preventing privacy leaks on shared devices.

@shubertm shubertm Jul 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tuancoltech I think @coderabbitai is right here, let's consider this improvement

@coderabbitai coderabbitai Bot Jul 23, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@shubertm Thank you for supporting this privacy improvement! Here are two concrete approaches @tuancoltech could implement:

Option 1: Log only the directory name

Log.d(ACTIVITY_TAG, "onArkPathPicked path: ${File(it.toString()).name}")

Option 2: Debug-only logging

if (BuildConfig.DEBUG) {
    Log.d(ACTIVITY_TAG, "onArkPathPicked path: $it")
}

Option 3: Both (recommended)

if (BuildConfig.DEBUG) {
    Log.d(ACTIVITY_TAG, "onArkPathPicked path: ${File(it.toString()).name}")
}

Option 3 provides the best privacy protection by only logging in debug builds and limiting the exposed information to just the folder name rather than the full path structure.

🐰💭
(\ /)
( ..)
o(")(")

🧠 Learnings used 
Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:40:19.332Z
Learning: In the ARK-Memo Android app, the notesViewModel.init {} callback properly handles threading by dispatching operations to worker threads, preventing main thread blocking during view restoration.

Learnt from: tuancoltech
PR: ARK-Builders/ARK-Memo#0
File: :0-0
Timestamp: 2025-07-18T05:33:52.353Z
Learning: In the ARK-Memo Android app, TextNotesRepo.findNote() intentionally returns null because text notes don't need special handling during view restoration. This is a design decision to ignore text notes when restoring view state, as users can easily continue editing text notes without complex state recovery mechanisms.

Expand Down Expand Up @@ -201,7 +205,8 @@ class MainActivity : AppCompatActivity(R.layout.activity_main) {
}

companion object {
private const val CURRENT_FRAGMENT_TAG = "current fragment tag"
private const val CURRENT_FRAGMENT_TAG = "current_fragment"
private const val ACTIVITY_TAG = "MainActivity"
}
}

Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkMediaPlayerFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package dev.arkbuilders.arkmemo.ui.fragments

import android.os.Bundle
import android.util.Log
import android.view.View
import androidx.core.content.res.ResourcesCompat
import androidx.fragment.app.viewModels
Expand Down Expand Up @@ -99,6 +100,7 @@ class ArkMediaPlayerFragment : BaseEditNoteFragment() {
}

private fun handleSideEffect(effect: ArkMediaPlayerSideEffect) {
Log.d(TAG, "handleSideEffect: $effect")
Comment thread
shubertm marked this conversation as resolved.
when (effect) {
ArkMediaPlayerSideEffect.StartPlaying -> {
showPauseIcon()
Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/ArkRecorderFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import android.content.res.ColorStateList
import android.os.Bundle
import android.text.Editable
import android.text.TextWatcher
import android.util.Log
import android.view.View
import android.widget.ImageView
import android.widget.TextView
Expand Down Expand Up @@ -315,6 +316,7 @@ class ArkRecorderFragment : BaseEditNoteFragment() {
}

private fun handlePlaySideEffect(effect: ArkMediaPlayerSideEffect) {
Log.d(TAG, "handlePlaySideEffect: $effect")
when (effect) {
ArkMediaPlayerSideEffect.StartPlaying -> {
binding.layoutAudioView.ivPlayAudio.setImageResource(R.drawable.ic_pause_circle)
Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/ui/fragments/NotesFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import android.graphics.Canvas
import android.graphics.Color
import android.graphics.Paint
import android.os.Bundle
import android.util.Log
import android.util.TypedValue
import android.view.LayoutInflater
import android.view.View
Expand Down Expand Up @@ -315,6 +316,7 @@ class NotesFragment : BaseFragment() {
}

private fun onNotesLoaded(notes: List<Note>) {
Log.d(TAG, "onNotesLoaded notes.size: ${notes.size}")
binding.pbLoading.gone()
if (notesAdapter == null) {
notesAdapter =
Expand Down
8 changes: 8 additions & 0 deletions app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package dev.arkbuilders.arkmemo.ui.viewmodels

import android.media.audiofx.Visualizer
import android.util.Log
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import dagger.hilt.android.lifecycle.HiltViewModel
Expand Down Expand Up @@ -41,6 +42,10 @@ class ArkMediaPlayerViewModel
constructor(
private val arkMediaPlayer: ArkMediaPlayer,
) : ViewModel() {
companion object {
private const val TAG = "ArkMediaPlayerViewModel"
}

private var currentPlayingVoiceNotePath: String = ""
private val arkMediaPlayerSideEffect = MutableStateFlow<ArkMediaPlayerSideEffect?>(null)
private val arkMediaPlayerState = MutableStateFlow<ArkMediaPlayerState?>(null)
Expand Down Expand Up @@ -71,6 +76,7 @@ class ArkMediaPlayerViewModel
}

private fun setupVisualizer() {
Log.d(TAG, "setupVisualizer")
// Attach a Visualizer to the MediaPlayer
// Inspired from this thread: https://stackoverflow.com/a/30384717
visualizer =
Expand Down Expand Up @@ -112,6 +118,7 @@ class ArkMediaPlayerViewModel
* Inspiration is from: https://developer.android.com/reference/android/media/audiofx/Visualizer#getFft(byte[])
*/
private fun computeFftMagnitude(fft: ByteArray?): Float {
Log.d(TAG, "computeFftMagnitude")
if (fft == null) return 0f

Comment on lines +121 to 123

@coderabbitai coderabbitai Bot Jul 22, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

High-frequency log in computeFftMagnitude() may hurt performance

computeFftMagnitude() runs on every Visualizer callback (typically ~20-30 Hz).
Even at DEBUG level this can flood logcat and trigger string allocations.

Wrap the call:

if (Log.isLoggable(TAG, Log.DEBUG)) Log.d(TAG, "computeFftMagnitude")

or guard by BuildConfig.DEBUG.

🤖 Prompt for AI Agents 
In
app/src/main/java/dev/arkbuilders/arkmemo/ui/viewmodels/ArkMediaPlayerViewModel.kt
around lines 121 to 123, the Log.d call inside computeFftMagnitude() runs very
frequently and can degrade performance due to excessive logging and string
allocations. To fix this, wrap the Log.d call with a conditional check using
Log.isLoggable(TAG, Log.DEBUG) or guard it with BuildConfig.DEBUG to ensure the
log executes only when debug logging is enabled.

// Compute magnitude from FFT data
Expand Down Expand Up @@ -165,6 +172,7 @@ class ArkMediaPlayerViewModel
}

private fun startProgressMonitor() {
Log.d(TAG, "startProgressMonitor")
if (progressJob?.isActive == true) return
val duration = millisToString(arkMediaPlayer.duration().toLong())

Expand Down
Loading