From 149ef9d1531226ba19199b91c883c0415b699d1f Mon Sep 17 00:00:00 2001
From: jfreegman <jfreegman@gmail.com>
Date: Sat, 24 Feb 2024 00:13:34 -0500
Subject: [PATCH] refactor: Window ID's and window creation error handling

We now use 16-bit ID's for windows and re-use previously discarded
ID's instead of using an incrementer. We also now do proper
error handling on window creation.
---
 src/audio_call.c      |  9 +++++-
 src/conference.c      | 29 +++++++++++++------
 src/conference.h      |  4 +--
 src/friendlist.c      | 65 +++++++++++++++++++++++++++++++++++++------
 src/friendlist.h      |  2 +-
 src/game_base.c       |  2 +-
 src/game_base.h       |  2 +-
 src/global_commands.c |  2 +-
 src/groupchats.c      | 18 +++++++++---
 src/groupchats.h      |  2 +-
 src/log.c             |  2 +-
 src/log.h             |  2 +-
 src/main.c            |  4 +--
 src/toxic.h           |  1 -
 src/windows.c         | 21 +++++++++-----
 src/windows.h         | 15 +++++++---
 16 files changed, 136 insertions(+), 44 deletions(-)

diff --git a/src/audio_call.c b/src/audio_call.c
index 221a1615a..96a827d9e 100644
--- a/src/audio_call.c
+++ b/src/audio_call.c
@@ -424,7 +424,14 @@ void callback_recv_invite(Toxic *toxic, uint32_t friend_number)
     }
 
     if (Friends.list[friend_number].window_id == -1) {
-        Friends.list[friend_number].window_id = add_window(toxic, new_chat(toxic->tox, Friends.list[friend_number].num));
+        const int window_id = add_window(toxic, new_chat(toxic->tox, Friends.list[friend_number].num));
+
+        if (window_id < 0) {
+            fprintf(stderr, "Failed to create new chat window in callback_recv_invite()\n");
+            return;
+        }
+
+        Friends.list[friend_number].window_id = window_id;
     }
 
     const Call *call = &CallControl.calls[friend_number];
diff --git a/src/conference.c b/src/conference.c
index 42af456b4..9c8f0f09d 100644
--- a/src/conference.c
+++ b/src/conference.c
@@ -204,7 +204,9 @@ static void init_conference_logging(ToxWindow *self, Toxic *toxic, uint32_t conf
     }
 }
 
-int64_t init_conference_win(Toxic *toxic, uint32_t conferencenum, uint8_t type, const char *title, size_t length)
+static void delete_conference(ToxWindow *self, Toxic *toxic, uint32_t conferencenum);
+
+int init_conference_win(Toxic *toxic, uint32_t conferencenum, uint8_t type, const char *title, size_t length)
 {
     if (toxic == NULL) {
         return -1;
@@ -223,17 +225,32 @@ int64_t init_conference_win(Toxic *toxic, uint32_t conferencenum, uint8_t type,
             // probably it so happens that this will (at least typically) be
             // the case, because toxic and tox maintain the indices in
             // parallel ways. But it isn't guaranteed by the API.
-            conferences[i].conferencenum = conferencenum;
-            conferences[i].window_id = add_window(toxic, self);
+            if (i == max_conference_index) {
+                ++max_conference_index;
+            }
+
             conferences[i].active = true;
+            conferences[i].conferencenum = conferencenum;
             conferences[i].num_peers = 0;
             conferences[i].type = type;
             conferences[i].start_time = get_unix_time();
             conferences[i].audio_enabled = false;
             conferences[i].last_sent_audio = 0;
 
+            const int window_id = add_window(toxic, self);
+
+            if (window_id < 0) {
+                fprintf(stderr, "Failed to create new conference window\n");
+                delete_conference(self, toxic, conferencenum);
+                return -1;
+            }
+
+            conferences[i].window_id = window_id;
+
             if (!tox_conference_get_id(toxic->tox, conferencenum, (uint8_t *) conferences[i].id)) {
                 fprintf(stderr, "Failed to fetch conference ID for conferencenum: %u\n", conferencenum);
+                delete_conference(self, toxic, conferencenum);
+                return -1;
             }
 
 #ifdef AUDIO
@@ -246,10 +263,6 @@ int64_t init_conference_win(Toxic *toxic, uint32_t conferencenum, uint8_t type,
 
             init_conference_logging(self, toxic, conferencenum);
 
-            if (i == max_conference_index) {
-                ++max_conference_index;
-            }
-
             return conferences[i].window_id;
         }
     }
@@ -330,7 +343,7 @@ void conference_rename_log_path(Toxic *toxic, uint32_t conferencenum, const char
 
     if (rename_logfile(toxic->windows, toxic->c_config, chat->title, new_title, myid, conference_id,
                        chat->window_id) != 0) {
-        fprintf(stderr, "Failed to rename conference log to `%s`\n", new_title);
+        fprintf(stderr, "Failed to rename conference log to '%s'\n", new_title);
     }
 }
 
diff --git a/src/conference.h b/src/conference.h
index 78ba63192..0ee494857 100644
--- a/src/conference.h
+++ b/src/conference.h
@@ -61,7 +61,7 @@ typedef struct NameListEntry {
 
 typedef struct {
     uint32_t conferencenum;
-    int64_t window_id;
+    uint16_t window_id;
     bool active;
     uint8_t type;
     int side_pos;    /* current position of the sidebar - used for scrolling up and down */
@@ -90,7 +90,7 @@ typedef struct {
 /* Frees all Toxic associated data structures for a conference (does not call tox_conference_delete() ) */
 void free_conference(ToxWindow *self, Windows *windows, const Client_Config *c_config, uint32_t conferencenum);
 
-int64_t init_conference_win(Toxic *toxic, uint32_t conferencenum, uint8_t type, const char *title, size_t length);
+int init_conference_win(Toxic *toxic, uint32_t conferencenum, uint8_t type, const char *title, size_t length);
 
 /* destroys and re-creates conference window with or without the peerlist */
 void redraw_conference_win(ToxWindow *self);
diff --git a/src/friendlist.c b/src/friendlist.c
index 6df776839..885eea160 100644
--- a/src/friendlist.c
+++ b/src/friendlist.c
@@ -433,7 +433,14 @@ static void friendlist_onMessage(ToxWindow *self, Toxic *toxic, uint32_t num, To
         return;
     }
 
-    Friends.list[num].window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+    const int window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+
+    if (window_id < 0) {
+        fprintf(stderr, "Failed to create new chat window in friendlist_onMessage\n");
+        return;
+    }
+
+    Friends.list[num].window_id = window_id;
 }
 
 static void friendlist_onConnectionChange(ToxWindow *self, Toxic *toxic, uint32_t num, Tox_Connection connection_status)
@@ -657,7 +664,14 @@ static void friendlist_onGameInvite(ToxWindow *self, Toxic *toxic, uint32_t frie
         return;
     }
 
-    Friends.list[friend_number].window_id = add_window(toxic, new_chat(tox, Friends.list[friend_number].num));
+    const int window_id = add_window(toxic, new_chat(tox, Friends.list[friend_number].num));
+
+    if (window_id < 0) {
+        fprintf(stderr, "Failed to create new chat window in friendlist_onGameInvite\n");
+        return;
+    }
+
+    Friends.list[friend_number].window_id = window_id;
 }
 
 #endif // GAMES
@@ -680,7 +694,14 @@ static void friendlist_onFileRecv(ToxWindow *self, Toxic *toxic, uint32_t num, u
         return;
     }
 
-    Friends.list[num].window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+    const int window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+
+    if (window_id < 0) {
+        fprintf(stderr, "Failed to create new chat window in friendlist_onFileRecv\n");
+        return;
+    }
+
+    Friends.list[num].window_id = window_id;
 }
 
 static void friendlist_onConferenceInvite(ToxWindow *self, Toxic *toxic, int32_t num, uint8_t type,
@@ -706,7 +727,14 @@ static void friendlist_onConferenceInvite(ToxWindow *self, Toxic *toxic, int32_t
         return;
     }
 
-    Friends.list[num].window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+    const int window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+
+    if (window_id < 0) {
+        fprintf(stderr, "Failed to create new chat window in friendlist_onConferenceInvite\n");
+        return;
+    }
+
+    Friends.list[num].window_id = window_id;
 }
 
 static void friendlist_onGroupInvite(ToxWindow *self, Toxic *toxic, uint32_t num, const char *data, size_t length,
@@ -730,7 +758,14 @@ static void friendlist_onGroupInvite(ToxWindow *self, Toxic *toxic, uint32_t num
         return;
     }
 
-    Friends.list[num].window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+    const int window_id = add_window(toxic, new_chat(tox, Friends.list[num].num));
+
+    if (window_id < 0) {
+        fprintf(stderr, "Failed to create new chat window in friendlist_onGroupInvite\n");
+        return;
+    }
+
+    Friends.list[num].window_id = window_id;
 }
 
 /* move friendlist/blocklist cursor up and down */
@@ -1015,7 +1050,14 @@ static bool friendlist_onKey(ToxWindow *self, Toxic *toxic, wint_t key, bool ltr
 
             /* Jump to chat window if already open */
             if (Friends.list[f].window_id < 0) {
-                Friends.list[f].window_id = add_window(toxic, new_chat(tox, Friends.list[f].num));
+                const int window_id = add_window(toxic, new_chat(tox, Friends.list[f].num));
+
+                if (window_id < 0) {
+                    fprintf(stderr, "Failed to create new chat window in friendlist_onKey\n");
+                    return true;
+                }
+
+                Friends.list[f].window_id = window_id;
             }
 
             set_active_window_by_id(toxic->windows, Friends.list[f].window_id);
@@ -1408,8 +1450,15 @@ static void friendlist_onAV(ToxWindow *self, Toxic *toxic, uint32_t friend_numbe
     }
 
     if (state != TOXAV_FRIEND_CALL_STATE_FINISHED) {
-        Friends.list[friend_number].window_id = add_window(toxic, new_chat(tox, Friends.list[friend_number].num));
-        set_active_window_by_id(toxic->windows, Friends.list[friend_number].window_id);
+        const int window_id = add_window(toxic, new_chat(tox, Friends.list[friend_number].num));
+
+        if (window_id < 0) {
+            fprintf(stderr, "Failed to create new chat window in friendlist_onAV");
+            return;
+        }
+
+        Friends.list[friend_number].window_id = window_id;
+        set_active_window_by_id(toxic->windows, window_id);
     }
 }
 #endif /* AUDIO */
diff --git a/src/friendlist.h b/src/friendlist.h
index 7d80e64b7..81262c9ea 100644
--- a/src/friendlist.h
+++ b/src/friendlist.h
@@ -85,7 +85,7 @@ typedef struct {
     size_t statusmsg_len;
     char pub_key[TOX_PUBLIC_KEY_SIZE];
     uint32_t num;
-    int64_t window_id;
+    int window_id;
     bool active;
     Tox_Connection connection_status;
     bool is_typing;
diff --git a/src/game_base.c b/src/game_base.c
index 57e9ccc5a..ecc85e284 100644
--- a/src/game_base.c
+++ b/src/game_base.c
@@ -274,7 +274,7 @@ int game_initialize(const ToxWindow *parent, Toxic *toxic, GameType type, uint32
 
     GameData *game = self->game;
 
-    const int64_t window_id = add_window(toxic, self);
+    const int window_id = add_window(toxic, self);
 
     if (window_id < 0) {
         free(game);
diff --git a/src/game_base.h b/src/game_base.h
index dc823a5cf..d236cfb3f 100644
--- a/src/game_base.h
+++ b/src/game_base.h
@@ -151,7 +151,7 @@ struct GameData {
     int        parent_max_x; // max dimensions of parent window
     int        parent_max_y;
 
-    int64_t    window_id;
+    uint16_t   window_id;
     WINDOW     *window;
 
     Toxic      *toxic;  // must be locked with Winthread mutex
diff --git a/src/global_commands.c b/src/global_commands.c
index 19ca11b7b..c9eb8f3c4 100644
--- a/src/global_commands.c
+++ b/src/global_commands.c
@@ -523,7 +523,7 @@ void cmd_conference(WINDOW *window, ToxWindow *self, Toxic *toxic, int argc, cha
 #endif
     }
 
-    if (init_conference_win(toxic, conferencenum, type, NULL, 0) == -1) {
+    if (init_conference_win(toxic, conferencenum, type, NULL, 0) < 0) {
         line_info_add(self, c_config, false, NULL, NULL, SYS_MSG, 0, 0, "Conference window failed to initialize.");
         tox_conference_delete(tox, conferencenum, NULL);
         return;
diff --git a/src/groupchats.c b/src/groupchats.c
index 7393ab169..2fc6c665e 100644
--- a/src/groupchats.c
+++ b/src/groupchats.c
@@ -387,19 +387,29 @@ int init_groupchat_win(Toxic *toxic, uint32_t groupnumber, const char *groupname
 
     for (int i = 0; i <= max_groupchat_index; ++i) {
         if (!groupchats[i].active) {
-            groupchats[i].window_id = add_window(toxic, self);
+            if (i == max_groupchat_index) {
+                ++max_groupchat_index;
+            }
+
             groupchats[i].active = true;
             groupchats[i].groupnumber = groupnumber;
             groupchats[i].num_peers = 0;
             groupchats[i].time_connected = get_unix_time();
 
-            if (!tox_group_get_chat_id(tox, groupnumber, (uint8_t *) groupchats[i].chat_id, NULL)) {
+            const int window_id = add_window(toxic, self);
+
+            if (window_id < 0) {
+                fprintf(stderr, "Failed to create new groupchat window\n");
                 close_groupchat(self, toxic, groupnumber);
                 return -1;
             }
 
-            if (i == max_groupchat_index) {
-                ++max_groupchat_index;
+            groupchats[i].window_id = window_id;
+
+            if (!tox_group_get_chat_id(tox, groupnumber, (uint8_t *) groupchats[i].chat_id, NULL)) {
+                fprintf(stderr, "Failed to fetch new groupchat ID\n");
+                close_groupchat(self, toxic, groupnumber);
+                return -1;
             }
 
             set_active_window_by_id(toxic->windows, groupchats[i].window_id);
diff --git a/src/groupchats.h b/src/groupchats.h
index a95b20d61..2c0763740 100644
--- a/src/groupchats.h
+++ b/src/groupchats.h
@@ -67,7 +67,7 @@ typedef struct {
     bool       active;
     uint64_t   time_connected;    /* The time we successfully connected to the group */
 
-    int64_t    window_id;
+    uint16_t   window_id;
     int        side_pos;     /* current position of the sidebar - used for scrolling up and down */
 } GroupChat;
 
diff --git a/src/log.c b/src/log.c
index e20e07a15..2d4e1a3d8 100644
--- a/src/log.c
+++ b/src/log.c
@@ -377,7 +377,7 @@ int load_chat_history(struct chatlog *log, ToxWindow *self, const Client_Config
  * Return -1 on failure.
  */
 int rename_logfile(Windows *windows, const Client_Config *c_config, const char *src, const char *dest,
-                   const char *selfkey, const char *otherkey, uint32_t window_id)
+                   const char *selfkey, const char *otherkey, uint16_t window_id)
 {
     ToxWindow *toxwin = get_window_pointer_by_id(windows, window_id);
     struct chatlog *log = NULL;
diff --git a/src/log.h b/src/log.h
index 696ab93e3..ec1557d93 100644
--- a/src/log.h
+++ b/src/log.h
@@ -98,6 +98,6 @@ int load_chat_history(struct chatlog *log, ToxWindow *self, const Client_Config
  * Return -1 on failure.
  */
 int rename_logfile(Windows *windows, const Client_Config *c_config, const char *src, const char *dest,
-                   const char *selfkey, const char *otherkey, uint32_t window_id);
+                   const char *selfkey, const char *otherkey, uint16_t window_id);
 
 #endif /* LOG_H */
diff --git a/src/main.c b/src/main.c
index 57caa0567..bf8c298fd 100644
--- a/src/main.c
+++ b/src/main.c
@@ -307,9 +307,9 @@ static void load_conferences(Toxic *toxic)
 
         title[length] = 0;
 
-        const int64_t win_id = init_conference_win(toxic, conferencenum, type, (const char *) title, length);
+        const int win_id = init_conference_win(toxic, conferencenum, type, (const char *) title, length);
 
-        if (win_id == -1) {
+        if (win_id < 0) {
             tox_conference_delete(tox, conferencenum, NULL);
             continue;
         }
diff --git a/src/toxic.h b/src/toxic.h
index e40f8954a..6b5472d5b 100644
--- a/src/toxic.h
+++ b/src/toxic.h
@@ -71,7 +71,6 @@ typedef struct Windows {
     ToxWindow  **list;
     uint16_t   count;
     uint16_t   active_index;
-    uint32_t   next_id;
 } Windows;
 
 typedef struct Toxic {
diff --git a/src/windows.c b/src/windows.c
index 2be29a743..709e4797e 100644
--- a/src/windows.c
+++ b/src/windows.c
@@ -761,7 +761,7 @@ void on_group_voice_state(Tox *tox, uint32_t groupnumber, Tox_Group_Voice_State
  * Returns the windows list index of the window associated with `id`.
  * Returns -1 if `id` is not found.
  */
-static int get_window_index(Windows *windows, uint32_t id)
+static int get_window_index(Windows *windows, uint16_t id)
 {
     for (uint16_t i = 0; i < windows->count; ++i) {
         if (windows->list[i]->id == id) {
@@ -772,16 +772,23 @@ static int get_window_index(Windows *windows, uint32_t id)
     return -1;
 }
 
-static uint32_t get_new_window_id(Windows *windows)
+static uint16_t get_new_window_id(Windows *windows)
 {
-    const uint32_t new_id = windows->next_id;
-    ++windows->next_id;
+    uint16_t new_id = 0;
+
+    for (uint16_t i = 0; i < UINT16_MAX; ++i) {
+        if (get_window_pointer_by_id(windows, i) == NULL) {
+            new_id = i;
+            break;
+        }
+    }
+
     return new_id;
 }
 
 /* CALLBACKS END */
 
-int64_t add_window(Toxic *toxic, ToxWindow *w)
+int add_window(Toxic *toxic, ToxWindow *w)
 {
     if (w == NULL || LINES < 2) {
         fprintf(stderr, "Failed to add window.\n");
@@ -835,7 +842,7 @@ void set_active_window_by_type(Windows *windows, Window_Type type)
     fprintf(stderr, "Warning: attemping to set active window with no active type: %d\n", type);
 }
 
-void set_active_window_by_id(Windows *windows, uint32_t id)
+void set_active_window_by_id(Windows *windows, uint16_t id)
 {
     const int idx = get_window_index(windows, id);
 
@@ -1357,7 +1364,7 @@ void refresh_inactive_windows(Windows *windows, const Client_Config *c_config)
 /* Returns a pointer to the ToxWindow associated with `id`.
  * Returns NULL if no ToxWindow exists.
  */
-ToxWindow *get_window_pointer_by_id(Windows *windows, uint32_t id)
+ToxWindow *get_window_pointer_by_id(Windows *windows, uint16_t id)
 {
     for (uint16_t i = 0; i < windows->count; ++i) {
         ToxWindow *w = windows->list[i];
diff --git a/src/windows.h b/src/windows.h
index 596e932e1..ca6d5d460 100644
--- a/src/windows.h
+++ b/src/windows.h
@@ -226,7 +226,7 @@ struct ToxWindow {
     char name[TOXIC_MAX_NAME_LENGTH + 1];
     int colour;  /* The ncurses colour pair of the window name */
     uint32_t num;    /* corresponds to friendnumber in chat windows */
-    uint32_t id; /* a unique and permanent identifier for this window */
+    uint16_t id; /* a unique and permanent identifier for this window */
     bool scroll_pause; /* true if this window is not scrolled to the bottom */
     unsigned int pending_messages;  /* # of new messages in this window since the last time it was focused */
     int x;
@@ -323,19 +323,26 @@ struct Help {
 
 void init_windows(Toxic *toxic);
 void draw_active_window(Toxic *toxic);
-int64_t add_window(Toxic *toxic, ToxWindow *w);
 void del_window(ToxWindow *w, Windows *windows, const Client_Config *c_config);
 void kill_all_windows(Toxic *toxic);    /* should only be called on shutdown */
 void on_window_resize(Windows *windows);
 void force_refresh(WINDOW *w);
-ToxWindow *get_window_pointer_by_id(Windows *windows, uint32_t id);
+ToxWindow *get_window_pointer_by_id(Windows *windows, uint16_t id);
 ToxWindow *get_active_window(const Windows *windows);
 void draw_window_bar(ToxWindow *self, Windows *windows);
 
+/*
+ * Initializes window `w` and adds it to the windows list.
+ *
+ * Returns the window's unique ID on success.
+ * Returns -1 on failure.
+ */
+int add_window(Toxic *toxic, ToxWindow *w);
+
 /*
  * Sets the active window to the window associated with `id`.
  */
-void set_active_window_by_id(Windows *windows, uint32_t id);
+void set_active_window_by_id(Windows *windows, uint16_t id);
 
 /*
  * Sets the active window to the first found window of window type `type`.