fix: refuse Shift alone with editing keys, and keep a saved Taildrop file when its delete fails

docs/index.html

@@ -1181,11 +1181,15 @@ <h2 id="keyboard-title">Keyboard</h2>
                     <p>
                         A bare key is not accepted: it would be taken from every
                         application. <kbd class="kbd">Shift</kbd> alone is accepted only
-                        with a key that types nothing, such as
-                        <kbd class="kbd">Shift</kbd>+<kbd class="kbd">F5</kbd>;
-                        <kbd class="kbd">Shift</kbd>+<kbd class="kbd">A</kbd> is how a
-                        capital A is typed. The shortcut does nothing on the lock screen
-                        or the login screen.
+                        with a key that types nothing and that editing text does not
+                        need, such as <kbd class="kbd">Shift</kbd>+<kbd class="kbd"
+                            >F5</kbd
+                        >. <kbd class="kbd">Shift</kbd>+<kbd class="kbd">A</kbd> is how
+                        a capital A is typed, and <kbd class="kbd">Shift</kbd> with an
+                        arrow key, Home, End, Page Up, Page Down, Tab, Enter or a dead
+                        key selects text, moves focus, ends a line or types an accented
+                        letter: the keys GNOME Settings refuses, plus dead keys. The
+                        shortcut does nothing on the lock screen or the login screen.
                     </p>
                     <p>
                         It is stored under <code>quickts-open-menu</code>. The prefix
@@ -1789,7 +1793,12 @@ <h2 id="localapi-title">The LocalAPI</h2>
                                 </td>
                                 <td data-label="Used for" class="muted">
                                     Receiving, streamed to disk. The file is written
-                                    before the daemon is told to forget it.
+                                    before the daemon is told to forget it. If the
+                                    daemon will not, the file still counts as saved and
+                                    is not offered again while QuickTS runs. It stays in
+                                    Tailscale’s inbox, where
+                                    <code>tailscale file get</code> can clear it, and
+                                    after a reload it can be offered again.
                                 </td>
                             </tr>
                             <tr>

modules/model.js

@@ -95,6 +95,15 @@ export class TailscaleModel {
     #peersReadAt = 0;
     #menuOpen = false;
 
+    /**
+     * Files saved here that tailscaled then would not delete, by name, with
+     * the size they were listed at. waitingFiles hides them, so a saved file
+     * is not offered, and saved as a duplicate, a second time. Nothing here
+     * deletes them: a name and a size are not proof that the file listed
+     * later is the one that was saved.
+     */
+    #savedNotDeleted = new Map();
+
     /**
      * @param {object} options Options.
      * @param {object} options.client Transport, from modules/io.js.
@@ -306,43 +315,78 @@ export class TailscaleModel {
     async waitingFiles() {
         if (this.#disposed || !isUp(this.#state)) return [];
 
+        let files;
         try {
-            return waitingFiles(await this.#request(waitingFilesRequest()));
+            files = waitingFiles(await this.#request(waitingFilesRequest()));
         } catch (error) {
             if (!isCanceled(error))
                 console.debug(`[quickts] could not list waiting files: ${error}`);
             return [];
         }
+
+        // Forget a saved file no longer listed at the size it was saved at: it
+        // has left the inbox, and a file listed under that name now is a
+        // different one, not yet saved.
+        const listed = new Map(files.map(({ name, size }) => [name, size]));
+        for (const [name, size] of this.#savedNotDeleted)
+            if (!listed.has(name) || listed.get(name) !== size)
+                this.#savedNotDeleted.delete(name);
+
+        return files.filter(
+            ({ name, size }) =>
+                !(
+                    this.#savedNotDeleted.has(name) &&
+                    this.#savedNotDeleted.get(name) === size
+                ),
+        );
     }
 
     /**
      * Save one waiting file, then let the daemon forget it.
      *
      * In that order. Deleting first loses the file if the write fails.
      *
+     * Once the file is written, a delete that fails does not undo that: the
+     * path is still returned, with no error, because the file is saved. The
+     * file stays in tailscaled's inbox, where `tailscale file get` can clear
+     * it; while this model lives, waitingFiles hides it, so it is not offered,
+     * and saved as a duplicate, a second time. Only a numeric size is
+     * recorded: without one there is nothing but the name to match.
+     *
      * @param {string} name The name as the daemon lists it.
+     * @param {number} size Its size as the daemon lists it.
      * @returns {Promise<{path: string, error: string}>} Where it went, or a
      *   REASON from modules/errors.js if it did not — untranslated, so
      *   modules/taildrop-section.js can turn it into a literal `_()` call
      *   rather than being handed English composed at run time.
      */
-    async saveFile(name) {
+    async saveFile(name, size) {
         if (this.#disposed) return { path: '', error: '' };
 
         // The daemon listed a name that cannot be a plain file here. It is
         // left on the daemon, where `tailscale file get` can still reach it.
         if (!isSafeFileName(name)) return { path: '', error: REASON.PROTOCOL };
 
+        let path;
         try {
-            const path = await this.#client.saveFile(getFileRequest(name), name);
-            await this.#request(deleteFileRequest(name));
-
-            return { path, error: '' };
+            path = await this.#client.saveFile(getFileRequest(name), name);
         } catch (error) {
             if (isCanceled(error)) return { path: '', error: '' };
 
             return { path: '', error: reasonOf(error) };
         }
+
+        try {
+            await this.#request(deleteFileRequest(name));
+        } catch (error) {
+            if (Number.isFinite(size)) this.#savedNotDeleted.set(name, size);
+            if (!isCanceled(error))
+                console.debug(
+                    `[quickts] saved a file but could not remove it from Taildrop: ${reasonOf(error)}`,
+                );
+        }
+
+        return { path, error: '' };
     }
 
     /**

modules/shortcuts.js

@@ -23,6 +23,59 @@ export const CAPTURE_IGNORE = 'ignore';
 /** Bind the combination and close. */
 export const CAPTURE_ASSIGN = 'assign';
 
+/**
+ * Keys that Shift alone may not be bound to, although none of them types a
+ * visible character: Shift with one of them selects text, moves focus or
+ * ends a line in every application.
+ *
+ * GNOME Settings' own list, forbidden_keyvals in is_valid_binding()
+ * (gnome-control-center, panels/keyboard/keyboard-shortcuts.c), plus
+ * ISO_Left_Tab, which is what GTK reports for Shift+Tab. Values are
+ * Gdk.KEY_* under gjs with Gdk 4.
+ */
+const SHIFT_FORBIDDEN_KEYVALS = new Set([
+    0xff50, // Home
+    0xff51, // Left
+    0xff52, // Up
+    0xff53, // Right
+    0xff54, // Down
+    0xff55, // Page_Up
+    0xff56, // Page_Down
+    0xff57, // End
+    0xff09, // Tab
+    0xfe20, // ISO_Left_Tab
+    0xff8d, // KP_Enter
+    0xff0d, // Return
+    0xff7e, // Mode_switch
+]);
+
+/**
+ * The dead keys, as inclusive keyval ranges.
+ *
+ * Derived under gjs with Gdk 4.22: Gdk.keyval_name(k) for every k in
+ * 0xfe50..0xfeff names dead_grave..dead_currency at 0xfe50-0xfe6f and
+ * dead_a..dead_hamza at 0xfe80-0xfe8d, and nothing else starting with dead_.
+ * The Gdk.KEY_dead_* constants add dead_lowline..dead_longsolidusoverlay at
+ * 0xfe90-0xfe93, which keyval_name cannot name (it returns "0xfe90"), so they
+ * are listed too.
+ */
+const DEAD_KEY_RANGES = [
+    [0xfe50, 0xfe6f],
+    [0xfe80, 0xfe8d],
+    [0xfe90, 0xfe93],
+];
+
+/**
+ * Whether a key is a dead key, which types nothing itself but puts an accent
+ * on the next letter typed.
+ *
+ * @param {number} keyval Key value.
+ * @returns {boolean} True for a dead key.
+ */
+function isDeadKey(keyval) {
+    return DEAD_KEY_RANGES.some(([first, last]) => keyval >= first && keyval <= last);
+}
+
 /**
  * Whether a key's own code point types something visible.
  *
@@ -39,10 +92,20 @@ function typesVisibly(codePoint) {
 }
 
 /**
- * Whether a captured combination may be bound as a global shortcut.
+ * Whether a captured key combination may be bound as a global shortcut.
+ *
+ * A bare key would steal it from every application, so it never may. Shift
+ * alone may only with a key that types no visible character and is not one
+ * that editing text needs (SHIFT_FORBIDDEN_KEYVALS) or a dead key: Shift+F5
+ * may, Shift+A, Shift+Left and Shift+dead_acute may not. Any other modifier
+ * makes a combination bindable, subject to Gtk's own accelerator check.
  *
- * A bare key would steal it from every application. Shift alone is bindable
- * only when the key types nothing on its own, close to GNOME Settings' rule.
+ * Built on GNOME Settings' is_valid_binding() (gnome-control-center,
+ * panels/keyboard/keyboard-shortcuts.c), and differs in three ways: this
+ * refuses every bare key, where GNOME allows one such as F5; it refuses
+ * Shift with a dead key, which GNOME's list leaves out; and it judges what
+ * Shift alone types by whether the key's code point is a visible character,
+ * where GNOME checks per-script keyval ranges.
  *
  * @param {number} mask Modifier mask, already reduced to the default mod mask.
  * @param {number} keyval Key value.
@@ -57,7 +120,13 @@ export function isValidBinding(
     { shiftMask, acceleratorValid, codePoint },
 ) {
     if (mask === 0) return false;
-    if (mask === shiftMask && typesVisibly(codePoint)) return false;
+    if (
+        mask === shiftMask &&
+        (typesVisibly(codePoint) ||
+            SHIFT_FORBIDDEN_KEYVALS.has(keyval) ||
+            isDeadKey(keyval))
+    )
+        return false;
 
     return acceleratorValid(keyval, mask);
 }

modules/taildrop-section.js

@@ -286,7 +286,7 @@ export class InboxSection {
         row.label.text = fill(_('Saving %s…'), file.name);
         row.setSensitive(false);
 
-        const { path, error } = await this._model.saveFile(file.name);
+        const { path, error } = await this._model.saveFile(file.name, file.size);
         if (generation !== this._generation) return;
 
         if (error) {

template.sha256

@@ -16,7 +16,7 @@ cbde6c3009a0b09910a20dc4bcd17bdf19c9174eee6d74c67c825efbfd9c17ca  mise.toml
 508b2aabe2c485cc9cdaf51ca7dd1a70e53c70c65c7dd0149dc1458a4f78a679  scripts/template-check.sh
 f0a3265f966c1017a80bcba09bff0c465ccec94b7ad9dd6456742f94905b3ae2  template.list
 f13c2fca51ef7a2db1755805d5f24a4a55c97605d40610168f1d39d12ad408af  tests/docs.spec.js
-450e8d7c616dfaeb3ad697cd6d457444aa9cd300fb0261da763a7f5d9499bd0f  tests/stubs/gi-glib.js
+8b3467177303c86b68bea7532c3bcec8585f5d66d0b7f1d604d22ef5790bea15  tests/stubs/gi-glib.js
 3969a5d8b38c607f1f70be1e6e88ce3ff75749710e1591437b511a5fff300df1  tests/stubs/gi-gobject.js
 dbebfe1620af3038a86d9454c57778ee4a5885f07748e6acb3f83aa11353dc86  tests/stubs/gi-meta.js
 757435b7d2469254f94210744c243fd0dc49983800f2f227fd90c7eb413e385a  tests/stubs/gi-pango.js

tests/model.test.js

@@ -1082,6 +1082,103 @@ describe('waiting files', () => {
         expect(daemon.deleted.at(-1)).toContain('a.txt');
     });
 
+    // The file is already written by then. Reporting the failure as an error
+    // threw that path away, and saving again made a duplicate "a (1).txt".
+    it('keeps the saved path when the daemon will not forget the file', async () => {
+        const { model, daemon } = setup();
+        daemon.responses.files = [{ Name: 'a.txt', Size: 4 }];
+        await model.start();
+        daemon.failures.set(
+            'DELETE /localapi/v0/files/a.txt',
+            new TransportError(REASON.HTTP, '500'),
+        );
+
+        const result = await model.saveFile('a.txt', 4);
+
+        expect(result).toEqual({ path: '/home/someone/Downloads/a.txt', error: '' });
+        expect(daemon.saved).toHaveLength(1);
+    });
+
+    // Nothing here ever deletes a file on its own: a name and a size are not
+    // proof that the file listed now is the one that was saved.
+    describe('a file saved that the daemon would not forget', () => {
+        const savedButKept = async size => {
+            const { model, daemon } = setup();
+            daemon.responses.files = [{ Name: 'a.txt', Size: 4 }];
+            await model.start();
+            await model.waitingFiles();
+            daemon.failures.set(
+                'DELETE /localapi/v0/files/a.txt',
+                new TransportError(REASON.HTTP, '500'),
+            );
+            await model.saveFile('a.txt', size);
+            // Were a DELETE sent now, it would succeed.
+            daemon.failures.clear();
+            daemon.reset();
+
+            return { model, daemon };
+        };
+
+        // Only GET /files/ may be asked for: the file itself never again.
+        const deleteRequests = daemon => daemon.pathsMatching('/files/a.txt');
+
+        // Listed, it would be offered to save again, and a second save is a
+        // duplicate.
+        it('is not listed again, and no later listing deletes it', async () => {
+            const { model, daemon } = await savedButKept(4);
+
+            expect(await model.waitingFiles()).toEqual([]);
+            expect(await model.waitingFiles()).toEqual([]);
+            expect(deleteRequests(daemon)).toEqual([]);
+            expect(daemon.deleted).toEqual([]);
+        });
+
+        // The original left the inbox some other way (`tailscale file get`, or
+        // a reply lost in a restart), and a new file of the same name and size
+        // arrived before the next listing. It cannot be told apart, so it is
+        // hidden — but it stays in Tailscale's inbox, never deleted unsaved.
+        it('hides, but never deletes, a same-name same-size file that replaced it', async () => {
+            const { model, daemon } = await savedButKept(4);
+            daemon.responses.files = [{ Name: 'a.txt', Size: 4 }];
+
+            expect(await model.waitingFiles()).toEqual([]);
+            expect(deleteRequests(daemon)).toEqual([]);
+            expect(daemon.deleted).toEqual([]);
+        });
+
+        it('is forgotten once no longer listed, so a later file of that name is shown', async () => {
+            const { model, daemon } = await savedButKept(4);
+
+            daemon.responses.files = [];
+            expect(await model.waitingFiles()).toEqual([]);
+
+            daemon.responses.files = [{ Name: 'a.txt', Size: 4 }];
+            expect(await model.waitingFiles()).toEqual([{ name: 'a.txt', size: 4 }]);
+            expect(deleteRequests(daemon)).toEqual([]);
+        });
+
+        it('does not hide a file of that name listed at another size', async () => {
+            const { model, daemon } = await savedButKept(4);
+            daemon.responses.files = [{ Name: 'a.txt', Size: 99 }];
+
+            expect(await model.waitingFiles()).toEqual([{ name: 'a.txt', size: 99 }]);
+            expect(deleteRequests(daemon)).toEqual([]);
+        });
+
+        // With no size to match, a later listing could only be matched on the
+        // name, which is not enough to hide anything by.
+        it.each([
+            ['no size', undefined],
+            ['a string', '4'],
+            ['NaN', Number.NaN],
+        ])('records nothing when saved with %s', async (_reason, size) => {
+            const { model, daemon } = await savedButKept(size);
+
+            expect(await model.waitingFiles()).toEqual([{ name: 'a.txt', size: 4 }]);
+            expect(deleteRequests(daemon)).toEqual([]);
+        });
+    });
+
     // tailscaled validates names on the way in; this is where one becomes a
     // path here, so it is checked again rather than trusted.
     it.each(['../escape.txt', '.bashrc', 'sub/dir.txt'])(