Restructure to make NotifyIcon internals more sensible.

Also fixes removing icons on exit.

index.d.ts

@@ -187,7 +187,7 @@ export namespace NotifyIcon {
          * A standard response is to invoke `Menu#showSync(event.mouseX, event.mouseY)`
          * for some menu.
          */
-        onSelect?: (this: void, event: SelectEvent) => void;
+        onSelect?: (this: NotifyIcon, event: SelectEvent) => void;
     }
 
     /**

src/data.cc

@@ -1,5 +1,5 @@
 #include "data.hh"
-#include "menu-object.hh"
+#include "notify-icon-object.hh"
 
 #include <map>
 #include <mutex>
@@ -27,14 +27,6 @@ EnvData* get_env_data(napi_env env) {
   return nullptr;
 }
 
-IconData* get_icon_data(napi_env env, int32_t icon_id) {
-  if (auto env_data = get_env_data(env); env_data) {
-    return env_data->get_icon_data(icon_id);
-  }
-
-  return nullptr;
-}
-
 static void message_pump_idle_cb(uv_idle_t* idle) {
   auto data = (EnvData*)idle->data;
 
@@ -54,40 +46,45 @@ static void message_pump_idle_cb(uv_idle_t* idle) {
   }
 }
 
-IconData* EnvData::get_icon_data(int32_t icon_id) {
-  if (auto it = icons.find(icon_id); it != icons.end()) {
-    return &it->second;
+napi_status EnvData::add_icon(int32_t id, napi_value value,
+                              NotifyIconObject* object) {
+  if (icons.empty()) {
+    uv_idle_start(&message_pump_idle, &message_pump_idle_cb);
   }
-
-  return nullptr;
+  napi_ref ref;
+  NAPI_RETURN_IF_NOT_OK(napi_create_reference(env, value, 1, &ref));
+  icons.insert({id, {ref, object}});
+  return napi_ok;
 }
 
-void EnvData::start_message_pump() {
-  uv_idle_start(&message_pump_idle, &message_pump_idle_cb);
+void EnvData::remove_icon(int32_t id) {
+  if (auto it = icons.find(id); it != icons.end()) {
+    napi_reference_unref(env, it->second.ref, nullptr);
+    icons.erase(id);
+    if (icons.empty()) {
+      uv_idle_stop(&message_pump_idle);
+    }
+  }
 }
 
-void EnvData::stop_message_pump() {
-  uv_idle_start(&message_pump_idle, &message_pump_idle_cb);
+EnvData::~EnvData() {
+  for (auto it = icons.begin(); it != icons.end(); it = icons.begin()) {
+    // remove will invalidate the iterator.
+    it->second.object->remove(env);
+  }
 }
 
-napi_status notify_icon_select(napi_env env, IconData* icon_data,
-                               bool right_button, int16_t mouse_x,
-                               int16_t mouse_y) {
-  napi_value icon_value;
-  NAPI_RETURN_IF_NOT_OK(
-      napi_get_reference_value(env, icon_data->icon_ref.ref, &icon_value));
-
-  napi_value event;
-  NAPI_RETURN_IF_NOT_OK(napi_create_object(env, &event,
-                                           {
-                                               {"icon", icon_value},
-                                               {"rightButton", right_button},
-                                               {"mouseX", mouse_x},
-                                               {"mouseY", mouse_y},
-                                           }));
-
-  NAPI_RETURN_IF_NOT_OK(icon_data->select_callback(nullptr, event));
-
+static napi_status notify_select(EnvData* env_data, int32_t icon_id,
+                                 bool right_button, int32_t mouse_x,
+                                 int32_t mouse_y) {
+  if (auto it = env_data->icons.find(icon_id); it != env_data->icons.end()) {
+    auto env = env_data->env;
+    napi_value value;
+    NAPI_RETURN_IF_NOT_OK(
+        napi_get_reference_value(env, it->second.ref, &value));
+    NAPI_RETURN_IF_NOT_OK(
+        it->second.object->select(env, value, right_button, mouse_x, mouse_y));
+  }
   return napi_ok;
 }
 
@@ -116,31 +113,30 @@ LRESULT messageWndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) {
           auto mouse_x = (int16_t)LOWORD(wParam);
           auto mouse_y = (int16_t)HIWORD(wParam);
 
-          if (auto icon_data = get_icon_data(env, icon_id); icon_data) {
+          if (auto env_data = get_env_data(env); env_data) {
             NapiHandleScope scope;
+            // if this fails, we can't even throw an error!
             NAPI_FATAL_IF_NOT_OK(scope.open(env));
-
-            if (notify_icon_select(env, icon_data, right_button, mouse_x,
-                                   mouse_y) != napi_ok) {
+            if (notify_select(env_data, icon_id, right_button, mouse_x,
+                              mouse_y) != napi_ok) {
               napi_throw_async_last_error(env);
             }
           }
-          break;
       }
       break;
     }
-      // case WM_MENUSELECT: {
-      //   auto menu = (HMENU)lParam;
-      //   auto flags = HIWORD(wParam);
-      //   if (flags & MF_POPUP) {
-      //     auto env = (napi_env)(void*)GetWindowLongPtrW(hwnd, GWLP_USERDATA);
-      //     int32_t item_id = LOWORD(wParam);
-      //     if (auto icon_data = get_icon_data_by_menu(env, menu, item_id);
-      //         icon_data && icon_data->select_callback) {
-      //     }
-      //   }
-      // }
   }
+  // case WM_MENUSELECT: {
+  //   auto menu = (HMENU)lParam;
+  //   auto flags = HIWORD(wParam);
+  //   if (flags & MF_POPUP) {
+  //     auto env = (napi_env)(void*)GetWindowLongPtrW(hwnd, GWLP_USERDATA);
+  //     int32_t item_id = LOWORD(wParam);
+  //     if (auto icon_data = get_icon_data_by_menu(env, menu, item_id);
+  //         icon_data && icon_data->select_callback) {
+  //     }
+  //   }
+  // }
 
   return DefWindowProc(hwnd, msg, wParam, lParam);
 }

src/data.hh

@@ -10,58 +10,20 @@
 #include <uv.h>
 
 #include "napi/napi.hh"
+#include "unique.hh"
 
 constexpr UINT WM_TRAYICON_CALLBACK = WM_USER + 123;
 
-// Like std::unique_ptr, but doesn't assume pointers and takes the deleter as a
-// value (auto-typed value template parameters is a c++17 feature, I believe).
-template <typename T, auto deleter, T default_value = nullptr>
-struct Unique {
-  T value = default_value;
-  Unique() = default;
-  Unique(T value) : value{value} {}
-  ~Unique() { deleter(value); }
-  Unique(const Unique&) = delete;
-  Unique& operator=(const Unique&) = delete;
-  Unique(Unique&& other) { assign(other.release()); }
-  Unique& operator=(Unique&& other) {
-    assign(other.release());
-    return *this;
-  }
-
-  operator T() const { return value; }
-  Unique& operator=(T new_value) {
-    assign(new_value);
-    return *this;
-  }
-
-  T release() { return std::exchange(value, default_value); }
-
-  void assign(T new_value) {
-    if (value != new_value) {
-      deleter(std::exchange(value, new_value));
-    }
-  }
-};
-
 using WndHandle = Unique<HWND, DestroyWindow>;
-using MenuHandle = Unique<HMENU, DestroyMenu>;
 
 struct NotifyIconObject;
-struct MenuObject;
-struct IconObject;
-
-struct IconData {
-  napi_env env;
-  uint16_t id;
-  GUID guid;
-  bool large_balloon_icon = false;
-  NapiUnwrappedRef<IconObject> icon_ref;
-  NapiUnwrappedRef<IconObject> notification_icon_ref;
-  NapiAsyncCallback select_callback;
-};
 
 struct EnvData {
+  struct IconData {
+    napi_ref ref;
+    NotifyIconObject* object;
+  };
+
   // Note that there's not much point trying to clean up these on
   // being destroyed, as that is when the environment is being destroyed.
   napi_env env = nullptr;
@@ -72,10 +34,10 @@ struct EnvData {
   std::unordered_map<int32_t, IconData> icons;
   uv_idle_t message_pump_idle = {this};
 
-  IconData* get_icon_data(int32_t icon_id);
+  napi_status add_icon(int32_t id, napi_value value, NotifyIconObject* object);
+  void remove_icon(int32_t id);
 
-  void start_message_pump();
-  void stop_message_pump();
+  ~EnvData();
 };
 
 EnvData* get_env_data(napi_env env);

src/menu-object.hh

@@ -2,6 +2,9 @@
 
 #include "data.hh"
 #include "napi/wrap.hh"
+#include "unique.hh"
+
+using MenuHandle = Unique<HMENU, DestroyMenu>;
 
 struct MenuObject : NapiWrapped<MenuObject> {
   MenuHandle menu;

src/napi/async.hh

@@ -103,42 +103,22 @@ struct NapiAsyncCallback : NapiRef {
     return napi_ok;
   }
 
-  template <typename... Args>
-  napi_status operator()(napi_value* result, Args&&... args) const {
-    NapiEscapableHandleScope handle_scope;
-    NAPI_RETURN_IF_NOT_OK(handle_scope.open(env));
-
-    // recv is checked for nullptr, but can be napi undefined.
-    // See https://github.com/nodejs/node/issues/26342
-    napi_value recv;
-    NAPI_RETURN_IF_NOT_OK(napi_get_undefined(env, &recv));
+  napi_value operator()(napi_value recv,
+                        std::initializer_list<napi_value> args) const {
+    napi_value result = nullptr;
 
     NapiCallbackScope callback_scope;
-    NAPI_RETURN_IF_NOT_OK(callback_scope.open(context));
+    NAPI_RETURN_NULL_IF_NOT_OK(callback_scope.open(context));
 
     napi_value func = nullptr;
-    NAPI_RETURN_IF_NOT_OK(NapiRef::get(&func));
-
-    napi_value arg_values[sizeof...(args)];
-    napi_value result_value = nullptr;
-    if (auto [status, status_arg_value] =
-            napi_create_many(env, arg_values, std::forward<Args>(args)...);
-        status != napi_ok) {
-      napi_throw_last_error(env);
-      return napi_pending_exception;
-    }
+    NAPI_RETURN_NULL_IF_NOT_OK(NapiRef::get(&func));
 
-    if (auto status = napi_call_function(env, recv, func, sizeof...(args),
-                                         arg_values, &result_value);
+    if (auto status = napi_call_function(env, recv, func, args.size(),
+                                         args.begin(), &result);
         status != napi_ok) {
       napi_throw_last_error(env);
-      return napi_pending_exception;
-    }
-
-    if (result) {
-      NAPI_RETURN_IF_NOT_OK(handle_scope.escape(result_value, result));
     }
 
-    return napi_ok;
+    return result;
   }
 };