From 625b016fd210d403a16cd5481d970980edd74311 Mon Sep 17 00:00:00 2001
From: Minggang Wang <minggangw@gmail.com>
Date: Mon, 8 Jan 2024 14:28:43 +0800
Subject: [PATCH] Leverage rcl functions to get topic of publisher/subscription
 (#951)

This patch implements:

1. Return the result of rcl_publisher_get_topic_name() directly, instead
of splitting it by "/" and returns the last part.

2. Use rcl_subscription_get_topic_name() to get the topic of a
subscription.

Meanwhile, this patch updates the following tests:

- test/test-node-oo.js
- test/test-node.js
- test/test-remapping.js

Fix: #949, #950
---
 lib/publisher.js       |  3 +--
 lib/subscription.js    |  2 +-
 src/rcl_bindings.cpp   | 15 +++++++++++++--
 test/test-node-oo.js   |  4 ++--
 test/test-node.js      |  4 ++--
 test/test-remapping.js |  4 ++--
 6 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/lib/publisher.js b/lib/publisher.js
index 7342c3c1..e442d022 100644
--- a/lib/publisher.js
+++ b/lib/publisher.js
@@ -32,8 +32,7 @@ class Publisher extends Entity {
    * @type {string}
    */
   get topic() {
-    const fullTopic = rclnodejs.getTopic(this._handle); // returns /my_node/mytopic
-    return fullTopic.split('/').pop();
+    return rclnodejs.getPublisherTopic(this._handle);
   }
 
   /**
diff --git a/lib/subscription.js b/lib/subscription.js
index 8f599e90..f7af18ba 100644
--- a/lib/subscription.js
+++ b/lib/subscription.js
@@ -70,7 +70,7 @@ class Subscription extends Entity {
    * @type {string}
    */
   get topic() {
-    return this._topic;
+    return rclnodejs.getSubscriptionTopic(this._handle);
   }
 
   /**
diff --git a/src/rcl_bindings.cpp b/src/rcl_bindings.cpp
index 71c42375..cee69318 100644
--- a/src/rcl_bindings.cpp
+++ b/src/rcl_bindings.cpp
@@ -942,7 +942,7 @@ NAN_METHOD(Publish) {
   info.GetReturnValue().Set(Nan::Undefined());
 }
 
-NAN_METHOD(GetTopic) {
+NAN_METHOD(GetPublisherTopic) {
   rcl_publisher_t* publisher = reinterpret_cast<rcl_publisher_t*>(
       RclHandle::Unwrap<RclHandle>(
           Nan::To<v8::Object>(info[0]).ToLocalChecked())
@@ -952,6 +952,16 @@ NAN_METHOD(GetTopic) {
   info.GetReturnValue().Set(Nan::New(topic).ToLocalChecked());
 }
 
+NAN_METHOD(GetSubscriptionTopic) {
+  rcl_subscription_t* subscription = reinterpret_cast<rcl_subscription_t*>(
+      RclHandle::Unwrap<RclHandle>(
+          Nan::To<v8::Object>(info[0]).ToLocalChecked())
+          ->ptr());
+
+  const char* topic = rcl_subscription_get_topic_name(subscription);
+  info.GetReturnValue().Set(Nan::New(topic).ToLocalChecked());
+}
+
 NAN_METHOD(CreateClient) {
   v8::Local<v8::Context> currentContent = Nan::GetCurrentContext();
   RclHandle* node_handle = RclHandle::Unwrap<RclHandle>(
@@ -2039,7 +2049,8 @@ std::vector<BindingMethod> binding_methods = {
     {"clearContentFilter", ClearContentFilter},
     {"createPublisher", CreatePublisher},
     {"publish", Publish},
-    {"getTopic", GetTopic},
+    {"getPublisherTopic", GetPublisherTopic},
+    {"getSubscriptionTopic", GetSubscriptionTopic},
     {"createClient", CreateClient},
     {"rclTakeResponse", RclTakeResponse},
     {"sendRequest", SendRequest},
diff --git a/test/test-node-oo.js b/test/test-node-oo.js
index 5bc2f65d..86e5a921 100644
--- a/test/test-node-oo.js
+++ b/test/test-node-oo.js
@@ -438,7 +438,7 @@ describe('topic & serviceName getter/setter', function () {
   it('publisher: topic property getter', function () {
     var node = new rclnodejs.Node('publisher', '/topic_getter');
     var publisher = node.createPublisher(RclString, 'chatter');
-    assert.deepStrictEqual(publisher.topic, 'chatter');
+    assert.deepStrictEqual(publisher.topic, '/topic_getter/chatter');
     node.destroy();
   });
 
@@ -449,7 +449,7 @@ describe('topic & serviceName getter/setter', function () {
       'chatter',
       (msg) => {}
     );
-    assert.deepStrictEqual(subscription.topic, 'chatter');
+    assert.deepStrictEqual(subscription.topic, '/topic_getter/chatter');
     node.destroy();
   });
 
diff --git a/test/test-node.js b/test/test-node.js
index 9f491af6..93d01af0 100644
--- a/test/test-node.js
+++ b/test/test-node.js
@@ -439,7 +439,7 @@ describe('topic & serviceName getter/setter', function () {
   it('publisher: topic property getter', function () {
     var node = rclnodejs.createNode('publisher', '/topic_getter');
     var publisher = node.createPublisher(RclString, 'chatter');
-    assert.deepStrictEqual(publisher.topic, 'chatter');
+    assert.deepStrictEqual(publisher.topic, '/topic_getter/chatter');
     node.destroy();
   });
 
@@ -450,7 +450,7 @@ describe('topic & serviceName getter/setter', function () {
       'chatter',
       (msg) => {}
     );
-    assert.deepStrictEqual(subscription.topic, 'chatter');
+    assert.deepStrictEqual(subscription.topic, '/topic_getter/chatter');
     node.destroy();
   });
 
diff --git a/test/test-remapping.js b/test/test-remapping.js
index 593f4c03..8a5247bb 100644
--- a/test/test-remapping.js
+++ b/test/test-remapping.js
@@ -52,7 +52,7 @@ describe('rcl node remapping', function () {
     node = rclnodejs.createNode('my_node');
     let publisher = node.createPublisher('std_msgs/msg/String', 'my_topic');
 
-    assert.equal(publisher.topic, 'foo_topic');
+    assert.equal(publisher.topic, '/foo_topic');
   });
 
   it('remap service name', async function () {
@@ -94,7 +94,7 @@ describe('rcl node remapping', function () {
 
     assert.equal(node.name(), 'foo_node');
     assert.equal(node.namespace(), '/foo_ns');
-    assert.equal(publisher.topic, 'foo_topic');
+    assert.equal(publisher.topic, '/foo_ns/foo_topic');
     assert.equal(service.serviceName, 'foo_service');
   });
 });