Windows Pageant: use an owner-drawn list box for the key list.

The main key list control in the Pageant window was previously an
ordinary LBS_HASSTRINGS list box, with tab characters aligning the
various parts of the key information into different columns. This was
fragile because any mistake in the font metrics could have overflowed
a tab stop and forced the text to move on to the next one.

Now I've switched the list box into LBS_OWNERDRAWFIXED mode, which
means that in place of a string for each list item I store a struct of
my choice, and I have to draw the list-box entries myself by
responding to WM_DRAWITEM. So now I'm drawing each component of the
key information as a separate call to ExtTextOut (plus one
TabbedTextOut to put the '(encrypted)' suffix on the end), which means
that the tab stops are now guaranteed to appear where I tell them to.

No functional change, for the moment: this is pure refactoring. As
closely as I can tell, the appearance of the list box is
pixel-for-pixel what it was before this commit. But it opens the door
for two further improvements (neither one done in this commit): I can
dynamically choose the tab stop locations based on querying the text
metrics of the strings that will actually need to fit in the columns,
and also, whatever reorganisation I need to do to make certificates
fit sensibly in this list box can now be done without worrying about
breaking anything terribly fragile.

windows/pageant.c

@@ -317,14 +317,24 @@ struct keylist_update_ctx {
     bool enable_reencrypt_controls;
 };
 
+struct keylist_display_data {
+    strbuf *alg, *bits, *hash, *comment, *info;
+};
+
 static void keylist_update_callback(
     void *vctx, char **fingerprints, const char *comment, uint32_t ext_flags,
     struct pageant_pubkey *key)
 {
     struct keylist_update_ctx *ctx = (struct keylist_update_ctx *)vctx;
     FingerprintType this_type = ssh2_pick_fingerprint(fingerprints, fptype);
-    const char *fingerprint = fingerprints[this_type];
+    ptrlen fingerprint = ptrlen_from_asciz(fingerprints[this_type]);
-    strbuf *listentry = strbuf_new();
+
+    struct keylist_display_data *disp = snew(struct keylist_display_data);
+    disp->alg = strbuf_new();
+    disp->bits = strbuf_new();
+    disp->hash = strbuf_new();
+    disp->comment = strbuf_new();
+    disp->info = strbuf_new();
 
     /* There is at least one key, so the controls for removing keys
      * should be enabled */
@@ -332,76 +342,55 @@ static void keylist_update_callback(
 
     switch (key->ssh_version) {
       case 1: {
-        put_fmt(listentry, "ssh1\t%s\t%s", fingerprint, comment);
-
         /*
-         * Replace the space in the fingerprint (between bit count and
+         * Expect the fingerprint to contain two words: bit count and
-         * hash) with a tab, for nice alignment in the box.
+         * hash.
          */
-        char *p = strchr(listentry->s, ' ');
+        put_dataz(disp->alg, "ssh1");
-        if (p)
+        put_datapl(disp->bits, ptrlen_get_word(&fingerprint, " "));
-            *p = '\t';
+        put_datapl(disp->hash, ptrlen_get_word(&fingerprint, " "));
         break;
       }
 
       case 2: {
         /*
-         * For nice alignment in the list box, we would ideally want
+         * Expect the fingerprint to contain three words: algorithm
-         * every entry to align to the tab stop settings, and have a
+         * name, bit count, hash.
-         * column for algorithm name, one for bit count, one for hex
+         */
-         * fingerprint, and one for key comment.
+        put_datapl(disp->alg, ptrlen_get_word(&fingerprint, " "));
-         *
+        put_datapl(disp->bits, ptrlen_get_word(&fingerprint, " "));
-         * Unfortunately, some of the algorithm names are so long that
+        put_datapl(disp->hash, ptrlen_get_word(&fingerprint, " "));
-         * they overflow into the bit-count field. Fortunately, at the
+
-         * moment, those are _precisely_ the algorithm names that
+        /*
-         * don't need a bit count displayed anyway (because for
+         * But we don't display the bit count if the algorithm isn't
-         * NIST-style ECDSA the bit count is mentioned in the
+         * one of the ones where it can vary. That way, those
-         * algorithm name, and for ssh-ed25519 there is only one
+         * algorithm names (which are generally longer) can safely
-         * possible value anyway). So we fudge this by simply omitting
+         * overlap into the bits column without colliding with
-         * the bit count field in that situation.
+         * pointless text.
-         *
-         * This is fragile not only in the face of further key types
-         * that don't follow this pattern, but also in the face of
-         * font metrics changes - the Windows semantics for list box
-         * tab stops is that \t aligns to the next one you haven't
-         * already exceeded, so I have to guess when the key type will
-         * overflow past the bit-count tab stop and leave out a tab
-         * character. Urgh.
          */
         const ssh_keyalg *alg = pubkey_blob_to_alg(
             ptrlen_from_strbuf(key->blob));
-
+        if (!(alg == &ssh_dsa || alg == &ssh_rsa))
-        bool include_bit_count = (alg == &ssh_dsa || alg == &ssh_rsa);
+            strbuf_clear(disp->bits);
-
-        int wordnumber = 0;
-        for (const char *p = fingerprint; *p; p++) {
-            char c = *p;
-            if (c == ' ') {
-                if (wordnumber < 2)
-                    c = '\t';
-                wordnumber++;
-            }
-            if (include_bit_count || wordnumber != 1)
-                put_byte(listentry, c);
-        }
-
-        put_fmt(listentry, "\t%s", comment);
-        break;
       }
     }
 
+    put_dataz(disp->comment, comment);
+
     if (ext_flags & LIST_EXTENDED_FLAG_HAS_NO_CLEARTEXT_KEY) {
-        put_fmt(listentry, "\t(encrypted)");
+        put_fmt(disp->info, "(encrypted)");
     } else if (ext_flags & LIST_EXTENDED_FLAG_HAS_ENCRYPTED_KEY_FILE) {
-        put_fmt(listentry, "\t(re-encryptable)");
+        put_fmt(disp->info, "(re-encryptable)");
 
         /* At least one key can be re-encrypted */
         ctx->enable_reencrypt_controls = true;
     }
 
+    /* This list box is owner-drawn but doesn't have LBS_HASSTRINGS,
+     * so we can use LB_ADDSTRING to hand the list box our display
+     * info pointer */
     SendDlgItemMessage(keylist, IDC_KEYLIST_LISTBOX,
-                       LB_ADDSTRING, 0, (LPARAM)listentry->s);
+                       LB_ADDSTRING, 0, (LPARAM)disp);
-    strbuf_free(listentry);
 }
 
 /*
@@ -410,6 +399,25 @@ static void keylist_update_callback(
 void keylist_update(void)
 {
     if (keylist) {
+        /*
+         * Clear the previous list box content and free their display
+         * structures.
+         */
+        {
+            int nitems = SendDlgItemMessage(keylist, IDC_KEYLIST_LISTBOX,
+                                            LB_GETCOUNT, 0, 0);
+            for (int i = 0; i < nitems; i++) {
+                struct keylist_display_data *disp =
+                    (struct keylist_display_data *)SendDlgItemMessage(
+                        keylist, IDC_KEYLIST_LISTBOX, LB_GETITEMDATA, i, 0);
+                strbuf_free(disp->alg);
+                strbuf_free(disp->bits);
+                strbuf_free(disp->hash);
+                strbuf_free(disp->comment);
+                strbuf_free(disp->info);
+                sfree(disp);
+            }
+        }
         SendDlgItemMessage(keylist, IDC_KEYLIST_LISTBOX,
                            LB_RESETCONTENT, 0, 0);
 
@@ -582,12 +590,6 @@ static INT_PTR CALLBACK KeyListProc(HWND hwnd, UINT msg,
         }
 
         keylist = hwnd;
-        {
-          static int tabs[] = { 35, 75, 300 };
-          SendDlgItemMessage(hwnd, IDC_KEYLIST_LISTBOX, LB_SETTABSTOPS,
-                             sizeof(tabs) / sizeof(*tabs),
-                             (LPARAM) tabs);
-        }
 
         int selection = 0;
         for (size_t i = 0; i < lenof(fptypes); i++) {
@@ -602,6 +604,105 @@ static INT_PTR CALLBACK KeyListProc(HWND hwnd, UINT msg,
         keylist_update();
         return 0;
       }
+      case WM_MEASUREITEM: {
+        assert(wParam == IDC_KEYLIST_LISTBOX);
+
+        MEASUREITEMSTRUCT *mi = (MEASUREITEMSTRUCT *)lParam;
+
+        /*
+         * Our list box is owner-drawn, but we put normal text in it.
+         * So the line height is the same as it would normally be,
+         * which is 8 dialog units.
+         */
+        RECT r;
+        r.left = r.right = r.top = 0;
+        r.bottom = 8;
+        MapDialogRect(hwnd, &r);
+        mi->itemHeight = r.bottom;
+
+        return 0;
+      }
+      case WM_DRAWITEM: {
+        assert(wParam == IDC_KEYLIST_LISTBOX);
+
+        DRAWITEMSTRUCT *di = (DRAWITEMSTRUCT *)lParam;
+
+        if (di->itemAction == ODA_FOCUS) {
+            /* Just toggle the focus rectangle either on or off. This
+             * is an XOR-type function, so it's the same call in
+             * either case. */
+            DrawFocusRect(di->hDC, &di->rcItem);
+        } else {
+            /* Draw the full text. */
+            bool selected = (di->itemState & ODS_SELECTED);
+            COLORREF newfg = GetSysColor(
+                selected ? COLOR_HIGHLIGHTTEXT : COLOR_WINDOWTEXT);
+            COLORREF newbg = GetSysColor(
+                selected ? COLOR_HIGHLIGHT : COLOR_WINDOW);
+            COLORREF oldfg = SetTextColor(di->hDC, newfg);
+            COLORREF oldbg = SetBkColor(di->hDC, newbg);
+
+            HFONT font = (HFONT)SendMessage(hwnd, WM_GETFONT, 0, 0);
+            HFONT oldfont = SelectObject(di->hDC, font);
+
+            /* ExtTextOut("") is an easy way to just draw the
+             * background rectangle */
+            ExtTextOut(di->hDC, di->rcItem.left, di->rcItem.top,
+                       ETO_OPAQUE | ETO_CLIPPED, &di->rcItem, "", 0, NULL);
+
+            struct keylist_display_data *disp =
+                (struct keylist_display_data *)di->itemData;
+
+            RECT r;
+
+            /* Apparently real list boxes start drawing at x=1, not x=0 */
+            r.left = r.top = r.bottom = 0;
+            r.right = 1;
+            MapDialogRect(hwnd, &r);
+            ExtTextOut(di->hDC, di->rcItem.left + r.right, di->rcItem.top,
+                       ETO_CLIPPED, &di->rcItem, disp->alg->s,
+                       disp->alg->len, NULL);
+
+            if (disp->bits->len) {
+                r.left = r.top = r.bottom = 0;
+                r.right = 35;
+                MapDialogRect(hwnd, &r);
+                ExtTextOut(di->hDC, di->rcItem.left + r.right, di->rcItem.top,
+                           ETO_CLIPPED, &di->rcItem, disp->bits->s,
+                           disp->bits->len, NULL);
+            }
+
+            r.left = r.top = r.bottom = 0;
+            r.right = 75;
+            MapDialogRect(hwnd, &r);
+            ExtTextOut(di->hDC, di->rcItem.left + r.right, di->rcItem.top,
+                       ETO_CLIPPED, &di->rcItem, disp->hash->s,
+                       disp->hash->len, NULL);
+
+            strbuf *sb = strbuf_new();
+            put_datapl(sb, ptrlen_from_strbuf(disp->comment));
+            if (disp->info->len) {
+                put_byte(sb, '\t');
+                put_datapl(sb, ptrlen_from_strbuf(disp->info));
+            }
+
+            r.left = r.top = r.bottom = 0;
+            r.right = 300;
+            MapDialogRect(hwnd, &r);
+            TabbedTextOut(di->hDC, di->rcItem.left + r.right, di->rcItem.top,
+                          sb->s, sb->len, 0, NULL, 0);
+
+            strbuf_free(sb);
+
+            SetTextColor(di->hDC, oldfg);
+            SetBkColor(di->hDC, oldbg);
+            SelectObject(di->hDC, oldfont);
+
+            if (di->itemState & ODS_FOCUS)
+                DrawFocusRect(di->hDC, &di->rcItem);
+        }
+        return 0;
+      }
       case WM_COMMAND:
         switch (LOWORD(wParam)) {
           case IDOK:

windows/pageant.rc

@@ -52,7 +52,7 @@ CAPTION "Pageant Key List"
 FONT 8, "MS Shell Dlg"
 BEGIN
     LISTBOX IDC_KEYLIST_LISTBOX, 10, 10, 420, 155,
-        LBS_EXTENDEDSEL | LBS_HASSTRINGS | LBS_USETABSTOPS | WS_VSCROLL | WS_TABSTOP
+        LBS_EXTENDEDSEL | LBS_OWNERDRAWFIXED | WS_VSCROLL | WS_TABSTOP
     PUSHBUTTON "&Add Key", IDC_KEYLIST_ADDKEY, 10, 187, 60, 14
     PUSHBUTTON "Add Key (&encrypted)", IDC_KEYLIST_ADDKEY_ENC, 75, 187, 80, 14
     PUSHBUTTON "Re-e&ncrypt", IDC_KEYLIST_REENCRYPT, 315, 187, 60, 14