Fix and cleanup class name / path

[CastagnaIT]

An issue has been raised when libandroidjni was used as a dependency of InputStream Adaptive
xbmc/inputstream.adaptive#1997

where the CJNIMediaDrmOnEventListener has the class name hardcoded with the "base" class name
or in other hand the kodi package name

libandroidjni/src/MediaDrmOnEventListener.cpp

Line 28 in 59b6850

static std::string s_className = "org/xbmc/kodi/interfaces/XBMCMediaDrmOnEventListener";

while investigating for a fix i noticed also some confusion between class names / paths uses i mean "the/class/name" vs "the.class.name", where in some cases both are mixed

"the/class/name" --> can be defined as "class name"
"the.class.name" --> can be defined as "fully qualified class name" aka path

Based on these definitions i cleaned up the code by adding more clear methods descriptions to avoid future mixed usages

the CJNIBase has the new SetBaseClassName that can be used by add-ons to set the "base" class name provided by InputStream android interface (example on xbmc/inputstream.adaptive#2007)

[kodiai]

📝 Kodiai Draft Review Summary

Draft — This PR is still in draft. Feedback is exploratory; findings use softer language.

What Changed

This PR refactors the libandroidjni codebase to clarify the distinction between "class names" (the/class/name) and "fully qualified class name paths" (the.class.name), and introduces a configurable base class name mechanism to fix issues when the library is used as a dependency. Reviewed: core logic

Strengths

• ✅ Adds comprehensive documentation to clarify the distinction between class names and class paths, which helps prevent future confusion
• ✅ Introduces the base class name mechanism that allows add-ons to configure their package prefix, directly addressing the reported InputStream Adaptive issue

Observations

Impact

[CRITICAL] src/JNIBase.cpp (27-29): Missing static member initialization for m_baseClassName
The static member m_baseClassName is declared in the header but never initialized in the .cpp file. When GetBaseClassName() is called before SetBaseClassName(), it will return an uninitialized std::string, which causes undefined behavior. Consider that classes using the "/XBMCInputDeviceListener" pattern in their constructors will call GetBaseClassName() during static initialization, potentially before PopulateStaticFields() runs.

[MAJOR] src/MediaDrmOnEventListener.cpp (33): Type mismatch - passing class name to loadClass() instead of class path
The loadClass() method expects a fully qualified class name path (dot-separated like "this.is.an.example"), but s_className is now a class name with slash separators. After the constructor processes it, GetClassName() returns "org/xbmc/kodi/interfaces/XBMCMediaDrmOnEventListener", but loadClass() needs "org.xbmc.kodi.interfaces.XBMCMediaDrmOnEventListener". You might want to use ClassNameToPath(GetClassName()) here to convert it properly.

[MAJOR] src/InputManager.cpp (39-40): Manual conversion duplicates ClassNameToPath functionality
This code manually converts slashes to dots, but the ClassNameToPath() helper method now provides this exact functionality. Consider replacing lines 38-41 with m_object = new_object(CJNIContext::getClassLoader().loadClass(ClassNameToPath(GetClassName()))); for consistency with the refactored API.

Preference

[MINOR] src/ClassLoader.h (33): Documentation inconsistency - parameter description doesn't match signature
Optional: The documentation states the parameter format is "this.is.an.example" (dot-separated), which is correct for a class path. However, the term "class path" can be ambiguous in Java contexts where it also refers to the search path for class files. You might want to consider using "fully qualified class name" in the description to be more precise, matching standard Java terminology.

Suggestions

• Optional: Consider adding a unit test or assertion to verify that SetBaseClassName() is called before any class using the "/" prefix pattern is instantiated. This would catch initialization order issues early.
• Future consideration: The initialization of m_baseClassName depends on PopulateStaticFields() being called at the right time. You might want to document this initialization requirement in the header file or add a runtime check.

Verdict

🔴 Address before merging -- (3) blocking issue(s) found (CRITICAL/MAJOR)

Review Details

• Files reviewed: 11
• Lines changed: +77 -22
• Profile: strict (auto, lines changed: 99)
• Author: first-time (adapted tone)
• Findings: 1 critical, 2 major, 0 medium, 1 minor (includes 4 from summary observations)
• Review completed: 2026-03-03T10:43:03.179Z
• Keyword parsing: No keywords detected

[CastagnaIT]

@kodiai propose a diff patch fix for your suggestion about to add assertion to verify that SetBaseClassName() is called before any class using the "/" prefix pattern is instantiated. This would catch initialization order issues early.

[kodiai]

kodiai response

Here's a diff patch that adds an assertion to verify SetBaseClassName() is called before any class using the "/" prefix pattern is instantiated:

diff --git a/src/JNIBase.h b/src/JNIBase.h
index 1234567..abcdefg 100644
--- a/src/JNIBase.h
+++ b/src/JNIBase.h
@@ -21,6 +21,7 @@
 
 #include "jutils.hpp"
 
+#include <cassert>
 #include <list>
 
 class CJNIBase
diff --git a/src/JNIBase.cpp b/src/JNIBase.cpp
index 1234567..abcdefg 100644
--- a/src/JNIBase.cpp
+++ b/src/JNIBase.cpp
@@ -30,8 +30,18 @@ int CJNIBase::RESULT_CANCELED = 0;
 
 CJNIBase::CJNIBase(std::string className)
 {
-  // If starts with / add the base class name prefix (e.g. org/xbmc/kodi)
+  // If starts with "/" add the base class name prefix (e.g. org/xbmc/kodi)
   if (!className.empty() && className.front() == '/')
+  {
+    // Verify that SetBaseClassName() has been called before instantiating
+    // any class using the "/" prefix pattern to avoid initialization order issues
+    assert(!m_baseClassName.empty() && 
+           "SetBaseClassName() must be called before instantiating classes with '/' prefix. "
+           "Add-ons using libandroidjni as a dependency should call CJNIBase::SetBaseClassName() "
+           "with their package name (e.g. 'org/xbmc/kodi') during initialization.");
+    
     className = GetBaseClassName() + className;
+  }
 
   m_className = className;
 }

How it works:

(1) The assertion is placed in the CJNIBase(std::string className) constructor at src/JNIBase.cpp:31, right where the "/" prefix is detected.

(2) It verifies that m_baseClassName is not empty before allowing the "/" prefix substitution to proceed.

(3) The assertion message clearly explains:

• What went wrong (SetBaseClassName wasn't called)
• When it should be called (before instantiating classes with "/" prefix)
• How to fix it (call CJNIBase::SetBaseClassName() during initialization)

(4) This catches the initialization order issue early at development/debug time rather than causing runtime failures later when class loading fails.

Example failure scenario: If an add-on creates a CJNIMediaDrmOnEventListener (which uses "/interfaces/XBMCMediaDrmOnEventListener" at src/MediaDrmOnEventListener.cpp:28) without first calling SetBaseClassName(), the assertion will trigger with a clear error message pointing to the root cause.

[CastagnaIT]

@kodiai i updated PR, please give a draft review

[kodiai]

kodiai response

Decision: APPROVE
Issues: none

[joseluismarti]

The CJNIBase class constructor supported both dot and slash formats as parameters, normalizing everything to '/'.
I believe this change no longer necessary.

[CastagnaIT]

yes internally CJNIBase was doing the thing to "convert" the class name passed, but this was not clear and make only messy code
better to make clear these things, standardize formats to avoid these mixtures

for example CJNIFileProvider was passing to class loader a wrong format that i fixed here

[joseluismarti]

I find it more flexible and robust to allow initialization using both ways of representing the class.

Agreed, GetDotClassName(m_classname) should be added to CJNIFileProvider.

One more thing: why aren't you using CJNIContext::getPackageName() to append the package to the class name?

[CastagnaIT]

I find it more flexible and robust to allow initialization using both ways of representing the class.

I don't fully agree, yes its more flexible, but prone to making easy mistakes, so better the explicit way

One more thing: why aren't you using CJNIContext::getPackageName() to append the package to the class name?

where you mean? about this on CJNIBase?
className = GetBaseClassName() + className;
addons currently cant make work CJNIContext because cant access to native activity class, at least i couldn't find any way, so cant call getPackageName

from addon side you can get it by calling interface
CInterfaceAndroidSystem::GetClassName

[SemperJ]

Doesn't this also need to be adjusted?
#define LOG_TAG "org.xbmc.kodi [libandroidjni]"

[CastagnaIT]

Doesn't this also need to be adjusted? #define LOG_TAG "org.xbmc.kodi [libandroidjni]"

thanks for point it out, for consistency i will change it to "Kodi" as already used in xbmc
https://github.com/xbmc/xbmc/blob/a25493de532d19a144acb358ac513766fed7a91a/xbmc/platform/android/activity/android_main.cpp#L75

[CastagnaIT]

@kodiai updated PR, please do last review

[kodiai]

kodiai response

Decision: NOT APPROVED

Issues:

• (1) [critical] src/JNIBase.cpp (84): Potential undefined behavior when baseClassName is empty - calling back() and pop_back() on an empty string causes undefined behavior. Add check: if (!m_baseClassName.empty() && m_baseClassName.back() == '/').

[CastagnaIT]

@kodiai fixed your critical issue, please finish review

[kodiai]

kodiai response

Decision: APPROVE

Issues: none