From 76d350dd8cf7e26811e3cc131f41494006f9ad44 Mon Sep 17 00:00:00 2001 From: PatR Date: Tue, 26 Mar 2019 01:47:33 -0700 Subject: [PATCH 1/2] curses horizontal status The unresolved "first problem" mentioned earlier in commit 382286cb9930f95445d2b1b521dbcbc72a20f5c6 was caused by stale values in status fields which had become disabled. Polymorphing left an old BL_XP value and returning to original form left an old BL_HD one. They weren't displayed but the stale value was included in the line length calculation, resulting in 4 or 5 columns being set aside for a phantom value. That implicitly reduced the available length of the line and could result in extra spaces separating other fields being squeezed out while unused spaces remained at the end of the line. Experience points, time, and score didn't trigger this problem because they were being explicitly excluded if disabled. So stale values for them when they had been enabled then later disabled didn't matter. --- win/curses/cursstat.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/win/curses/cursstat.c b/win/curses/cursstat.c index 999b1ecda..ca88f6c5e 100644 --- a/win/curses/cursstat.c +++ b/win/curses/cursstat.c @@ -370,6 +370,11 @@ unsigned long *colormasks; w = xtra = 0; /* w: width so far; xtra: number of extra spaces */ prev_fld = BL_FLUSH; for (i = 0; (fld = (*fieldorder)[j][i]) != BL_FLUSH; ++i) { + /* when the core marks a field as disabled, it doesn't call + status_update() to tell us to throw away the old value, so + polymorph leaves stale XP and rehumanize leaves stale HD */ + if (!status_activefields[fld]) + *status_vals[fld] = '\0'; text = status_vals[fld]; if (i == 0 && *text == ' ') ++text; From 99ed00012e076be07846edceb85e3e5e53f2bb02 Mon Sep 17 00:00:00 2001 From: PatR Date: Tue, 26 Mar 2019 02:30:59 -0700 Subject: [PATCH 2/2] curses message history The curses interface maintains message history in a doubly linked list with a capacity limit. Once capacity is reached, the list head is advanced and the old head discarded, but it was leaving the new head's 'previous element' link pointing at that discarded element. tmp_mesg = first_mesg->next_mesg; (at this stage, tmp_mesg->prev_mesg points at first_mesg), free(first_mesg); first_mesg = tmp_mesg; (with necessary 'first_mesg->prev_msg = NULL' missing). The situation wasn't a significant problem because traversing the list was limited by a counter. Going from tail back to head exhausted the counter without ever accessing the stale pointer. Since it wasn't noticeable, I haven't added a fixes entry for this. I've also changed it to do fewer memory allocations and frees by reusing the old list head instead of always allocating a new element and freeing the one being replaced. --- win/curses/cursmesg.c | 59 +++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/win/curses/cursmesg.c b/win/curses/cursmesg.c index 96f91e60b..ccbc6f1e0 100644 --- a/win/curses/cursmesg.c +++ b/win/curses/cursmesg.c @@ -24,7 +24,7 @@ typedef struct nhpm { static void scroll_window(winid wid); static void unscroll_window(winid wid); static void directional_scroll(winid wid, int nlines); -static void mesg_add_line(char *mline); +static void mesg_add_line(const char *mline); static nhprev_mesg *get_msg_line(boolean reverse, int mindex); static int turn_lines = 1; @@ -95,7 +95,7 @@ curses_message_win_puts(const char *message, boolean recursed) if (!recursed) { strcpy(toplines, message); - mesg_add_line((char *) message); + mesg_add_line(message); } if (linespace < message_length) { @@ -398,7 +398,7 @@ curses_message_win_getline(const char *prompt, char *answer, int buffer) Strcat(tmpbuf, " "); nlines = curses_num_lines(tmpbuf,width); maxlines += nlines * 2; - linestarts = (char **) alloc((unsigned) (sizeof (char *) * maxlines)); + linestarts = (char **) alloc((unsigned) (maxlines * sizeof (char *))); p_answer = tmpbuf + strlen(tmpbuf); linestarts[0] = tmpbuf; @@ -515,7 +515,7 @@ curses_message_win_getline(const char *prompt, char *answer, int buffer) (void) strncpy(answer, p_answer, buffer); answer[buffer - 1] = '\0'; Strcpy(toplines, tmpbuf); - mesg_add_line((char *) tmpbuf); + mesg_add_line(tmpbuf); free(tmpbuf); curs_set(orig_cursor); curses_toggle_color_attr(win, NONE, A_BOLD, OFF); @@ -615,34 +615,55 @@ directional_scroll(winid wid, int nlines) /* Add given line to message history */ static void -mesg_add_line(char *mline) +mesg_add_line(const char *mline) { - nhprev_mesg *tmp_mesg = NULL; - nhprev_mesg *current_mesg = (nhprev_mesg *) alloc((unsigned) - (sizeof (nhprev_mesg))); + nhprev_mesg *current_mesg; - current_mesg->str = curses_copy_of(mline); + /* + * Messages are kept in a doubly linked list, with head 'first_mesg', + * tail 'last_mesg', and a maximum capacity of 'max_messages'. + */ + if (num_messages < max_messages) { + /* create a new list element */ + current_mesg = (nhprev_mesg *) alloc((unsigned) sizeof (nhprev_mesg)); + current_mesg->str = dupstr(mline); + } else { + /* instead of discarding list element being forced out, reuse it */ + current_mesg = first_mesg; + /* whenever new 'mline' is shorter, extra allocation size of the + original element will be frittered away, until eventually we'll + discard this 'str' and dupstr() a replacement; we could easily + track the allocation size but don't really need to do so */ + if (strlen(mline) <= strlen(current_mesg->str)) { + Strcpy(current_mesg->str, mline); + } else { + free((genericptr_t) current_mesg->str); + current_mesg->str = dupstr(mline); + } + } current_mesg->turn = moves; - current_mesg->next_mesg = NULL; if (num_messages == 0) { + /* very first message; set up head */ first_mesg = current_mesg; - } - - if (last_mesg != NULL) { + } else { + /* not first message; tail exists */ last_mesg->next_mesg = current_mesg; } current_mesg->prev_mesg = last_mesg; - last_mesg = current_mesg; + last_mesg = current_mesg; /* new tail */ if (num_messages < max_messages) { - num_messages++; + /* wasn't at capacity yet */ + ++num_messages; } else { - tmp_mesg = first_mesg->next_mesg; - free(first_mesg->str); - free(first_mesg); - first_mesg = tmp_mesg; + /* at capacity; old head is being removed */ + first_mesg = first_mesg->next_mesg; /* new head */ + first_mesg->prev_mesg = NULL; /* head has no prev_mesg */ } + /* since 'current_mesg' might be reusing 'first_mesg' and has now + become 'last_mesg', this update must be after head replacement */ + last_mesg->next_mesg = NULL; /* tail has no next_mesg */ }