diff --git a/claude.md b/claude.md index 63c361e8c..5b08da7f1 100644 --- a/claude.md +++ b/claude.md @@ -129,7 +129,14 @@ whenever something precedes the call on its line (`do!`, `let! x =`): the offsid appended `Snapshot` call goes in front of `ToTask`, `ConfigureAwait` or `GetAwaiter` in either language, none of which returns something to call it on. And a `Remove` of `settings.Snapshot("old");` takes the statement, since `settings;` is none, while one whose value -is awaited, assigned, returned or passed takes only the call. +is awaited, assigned, returned or passed takes only the call. A Remove of a call with a line to +itself keeps that line, empty, when the line under it holds a Snapshot call: the same Remove is +applied once per framework, and a sibling with the same literal would otherwise come up onto the +recorded line and lose its own. F# lexes inside a block comment, so the scanner steps over +strings, char literals and `(*)` there, and over a double backticked name in code, all held to +`dotnet fsi`. An `InlinePatchFile` says an original value is present and empty on a line of its +own, since an older reader skips a line it does not know and rejects a known field that is not +base64. A snapshot's key is built once an entry. An accept reads, lexes and rewrites the whole source file, and the rewrite is what costs: a file written a moment ago is scanned by whatever watches the drive before the next thing can open it, @@ -196,7 +203,11 @@ the comment there about not caching "nothing staged" asks for. half its own size or less is copied out of one scaled copy, made on the pool and kept in `ImageCache`'s composite slot with pan out of the key, where it was scaled from full resolution on every paint, 50 ms for a 4000 by 3000 pair. Between half and full size nothing is kept, since - the copy would cost up to the decoded picture again. + the copy would cost up to the decoded picture again. Frames are handed over from inside user32's + own loop for every part of the scroll bar, not the thumb alone, and `Present` stops that timer. + An Alt chord is no command. A held key gives one accept or discard (the repeat flag, read in + `ProcessCmdKey`, for what `ViewerSession.ChangesQueue` names), while navigation keeps its + repeats. A minimised window waits between frames as a hidden one does. - macOS renders with **AppKit and Core Text** (`native/swift/`), Linux with **raylib and Dear ImGui** (`native/`). Both implement the same C ABI, so the managed interop layer is identical. - macOS took the same treatment as Windows: a real menu bar, an `NSMenu` context menu, `NSView` @@ -220,7 +231,14 @@ the comment there about not caching "nothing staged" asks for. copy's slot, except in a capture, past its own size, or when both panes name one picture. None of this head can be compiled or run from Windows: CI's `macos-14` job is the first build, and its OSX baselines come from that job's `received-*` artifacts. That job only captures, so it - exercises none of the clip test and none of `reduced`. + exercises none of the clip test and none of `reduced`. A picture or a scaled copy that lands + during a draw clipped to less than the window is owed a whole redraw, which `takeFinished` + reports at the next present. The pump returns as soon as an event leaves input, and waits a + tenth of a second for a window that is ordered out, miniaturised or covered, which the managed + side is not told. A wheel is told from a trackpad by `hasPreciseScrollingDeltas`, and a + control-click is taken in `mouseDown`, since the view has no `NSMenu` for AppKit to ask for. + A drag reports the frame's own centre on an axis its picture cannot move on, as the Linux head + does: only when the space is what cut the picture short can it move. - Linux draws its own menu, so it keeps that baseline. Its tooltip and pane scrollbar are ImGui's, the scrollbar being `ScrollbarEx` driven in rows rather than pixels so its travel is exactly `ViewerSession`'s clamp. Its footer wraps as the macOS one does (`LayOutFooter`), with two @@ -243,7 +261,19 @@ the comment there about not caching "nothing staged" asks for. about in `deview_present` before a window is left alone, or the window keeps the frame before. The checkerboard is one quad of a two by two texture set to repeat, behind a picture that has a pixel to see through, which the decoder looks for as it decodes: it was a quad a dark square, - 113,000 triangles a frame at 4K, behind opaque pictures too. + 113,000 triangles a frame at 4K, behind opaque pictures too. Input comes from GLFW's mouse + button, scroll, key, character and cursor-enter callbacks, set over raylib's and still calling + them, not from raylib's state read once a frame: a press and release that arrive together were + never seen that way. Presses go to ImGui in order and keys to the managed side one a poll, and + those queues are among what `deview_present` asks before leaving a window alone. A control chord + is read by what its key types, a layout with no Latin letters falls back to position, and arrows + and paging repeat. A pointer that has left the window is nowhere to ImGui unless a button is + held. A label with `##` in it is drawn by the shim over an item given no text, since ImGui hides + what follows the mark. Pictures get mipmaps on the decoder thread and are sampled from them + under half size, and one past `GL_MAX_TEXTURE_SIZE` is not drawn. The window is scaled by GLFW's + content scale (`Xft.dpi` over 96), read once and without `FLAG_WINDOW_HIGHDPI`; a capture never + is, and `MeasureGrid` reports the cells of the last window frame. An Xvfb used to check a layout + or a scale needs `-noreset`, or the setting is gone before the viewer connects. - Group headers fold. `SessionState.Collapsed` holds `QueueItem.GroupKey`s and `QueueProjection` skips their members, so the marker rides in the label and no head or ABI field knows about it. Whether an entry is hidden is always read back out of `VisibleEntries`, never recomputed — the @@ -472,6 +502,20 @@ the comment there about not caching "nothing staged" asks for. implementation removes the failure mode instead of detecting it. - Plain text, every value base64, for the same reason `InlinePatchFile` is: snapshot text contains quotes, braces and newlines, and the `inline` body carries an `InlinePatchFile` payload verbatim. +- A connection is one exchange, except for the library's telling sends to an owner that says it + keeps one. An owner ends an ordinary reply with `keeps: 1`; a client that sees it opens one + more connection with `keep: 1` and sends its settles, retires, moves and deletes down that, + each ended by an empty line and answered the same way (`ViewerClient.TrySend(ViewerMessage)`). + The client closes first, so a connection per settle left a port in TIME_WAIT for two minutes + per passing inline verification: 2,576 settles left 2,576, and now leave one. Skipping settles + instead was not safe, since one skipped for a snapshot another process queued leaves it on + offer. An older owner never says the line and gets a connection each, an older client skips + it, and a send that finds its connection gone falls back to the ordinary exchange. So a field + can only ever be added as a new line. `ViewerServer.Stop` closes kept connections. +- `ViewerServer.Serve` is the accept loop with its accept handed in. A failed accept is retried + at once, and from the second in a row after 100 ms: with no descriptors left a failure persists, + and the loop took a core. A send whose caller cancelled throws `OperationCanceledException` and + records nothing about the port. - Compiles for every DiffEngine target, so the socket calls carry `#if` branches for the frameworks with no cancellation overloads. `ViewerProtocolTests` runs on all of them. - `ViewerServer`'s accept loop awaits with `ConfigureAwait(false)`, one of two places that matter @@ -590,6 +634,17 @@ the comment there about not caching "nothing staged" asks for. back through `Application.Run()`. - A move that arrives for a tracked pair with no tool keeps the tool it was tracked with (`Tracker.Retarget`): one over the viewer port carries two paths and nothing else. +- A move's process id is believed only when the image it names has the file name of the + payload's `Exe` (`ProcessEx.TryGetTool`): a library from before `ProcessCleanup.StillRunning` + can send an id that has since been reused, and the tray would kill whatever holds it. A tool + started through a `.cmd`, or one whose image cannot be read, is tracked with no process. A + move lets go of its process wherever it leaves for good (`Tracker.Release`). +- `ITrackedFiles.Version` is what `ListingTag` asks about the tracked files: the identity of the + tracked objects, so `TrackedMove` and `TrackedDelete` must stay immutable in everything a + listing carries. The scan removes a move by key and value, so one staged again since it looked + is left. `Tracker.Clear` hands the queue's discard to a worker, as a single discard does. A hot + key's action is caught in `KeyRegister`, because a throw from a message filter comes out of + `Application.Run()`. - Allows accepting/discarding diffs from system tray **Packaging.Tests (`src/Packaging.Tests/`):** @@ -622,4 +677,7 @@ the comment there about not caching "nothing staged" asks for. - `ViewerClient` remembers a port found unowned for ten minutes (`RecheckUnownedAfter`), and the library's telling sends - settle, retire, move, delete, the first inline or diff send - skip the connect while that stands. A refused loopback connection costs two seconds on Windows (firewall stealth mode drops the reset), and a green run settles once per inline verification, which was six minutes for a class of 188 inline tests. Probes (`IsOwned`), the hosts and `InlineQueueClient` always ask and correct the memory; so does `SettleAppliedInline`, being one send per accept. Asking, on Windows, is the operating system's listener table first (`ListenerTable`, shared with `PiperClient`): no listener on the port means nobody to connect to, said without the two seconds, and a listener or a table that cannot be read leaves the connect to answer. So the first telling send of a test process, the launch gate's probe and each of its polls no longer wait to be refused. By port alone, whichever address, since the table is only believed when it says nobody is there. Not for a port that accepted a connection in the last second (`TrustOwnerFor`), because reading the table is reading every connection the machine has, and a run of settles to a live owner would pay more for each than the connect costs - `TrayDisabledChecker` respects `DiffEngine_TrayDisabled` env var, behind `DiffRunner.TrayDisabled`. Separate from `Disabled` because tracking a pending move is separate from launching a tool: every exit of `InnerLaunch`, `Disabled` included, still calls `AddMove`. `PendingFiles.TrayAvailable` is the single gate - Tests use TUnit and Verify for snapshot testing +- The three WinForms test and benchmark hosts set `UnhandledExceptionMode.ThrowException` from a module initializer with `threadScope: false`. The one argument overload covers only the thread that calls it, which leaves every test thread showing WinForms' dialog on the desktop of whoever is running the tests, and waiting for a click. Never run a test that throws inside a window message in a build without it. `DiffEngineTray.Tests` also declines the tray's "open an issue" box (`IssueLauncher.Declined`) and records what was asked in `ModuleInitializer.IssuesAsked` +- `MachineSettings.Ignore` clears `DiffEngine_Disabled`, so a snapshot test that is expected to fail is run with a build server variable such as `TEAMCITY_VERSION` set, or Verify tells the real tray about it +- `ps` is asked with `-ww`: procps lets an exported `COLUMNS` cut every command line `ProcessCleanup` reads - The native pixel snapshots (`PixelTests`) are opt in through `DIFFENGINE_VIEWER_PIXEL_TESTS`, which `MachineSettings.Ignore` has to leave alone: it clears every `DiffEngine_*` variable without regard to case, and clearing that one skipped them on the CI job that sets it, silently, for as long as nobody looked. Every call into the shim goes through one thread (`OnShimThread`), because on Linux the window's GL context belongs to the thread that made it and each test starts on whichever pool thread picks it up. The Linux baselines reproduce in an `ubuntu:24.04` container set up as the `unix` job in `build.yml` is - the shim built from source, Xvfb, llvmpipe - which is also the only way to run the C++ at all from Windows diff --git a/native/include/deview.h b/native/include/deview.h index fd4e67976..7cd46d944 100644 --- a/native/include/deview.h +++ b/native/include/deview.h @@ -329,6 +329,8 @@ typedef struct DeviewInput { /* * Where a drag has left an enlarged picture: the point now at the middle of what shows, as * fractions of the picture's width and height, already kept inside what the space can show. + * On an axis the dragged picture cannot move on, because all of it shows, it is the centre + * the frame was drawn with, unchanged: the other pane's picture may be able to move there. * panX is -1 on the frames with no such drag, which is almost all of them. * * A position rather than a distance, measured from where the button went down, so the picture diff --git a/native/src/deview.cpp b/native/src/deview.cpp index 85f7d0289..0d6835215 100644 --- a/native/src/deview.cpp +++ b/native/src/deview.cpp @@ -65,6 +65,59 @@ typedef void (*DeviewRefresh)(void* window); DeviewRefresh glfwSetWindowRefreshCallback(void* window, DeviewRefresh callback); } +/* + * And its callbacks for a mouse button, the wheel, a key and a character. raylib sets all four, and + * keeps from them what was so when it last read the window system's events: whether a button is + * down, the last wheel message, which keys are down. That is a state, read once a frame, and a + * press and a release that arrive between two reads leave it as it was. So these are set over + * raylib's, which are kept and still called, and what they are told is kept as what happened: see + * State::presses. + * + * And the one for the pointer crossing the window's edge, which raylib also keeps and nothing + * else here could be told by: where the pointer is stays where it last was in the window, for as + * long as it is anywhere else. See State::pointerInside. + */ +extern "C" +{ +typedef void (*DeviewButtonEvent)(void* window, int button, int action, int mods); +typedef void (*DeviewScrollEvent)(void* window, double across, double down); +typedef void (*DeviewKeyEvent)(void* window, int key, int scancode, int action, int mods); +typedef void (*DeviewCharacterEvent)(void* window, unsigned int codepoint); +typedef void (*DeviewCrossingEvent)(void* window, int entered); +DeviewCrossingEvent glfwSetCursorEnterCallback(void* window, DeviewCrossingEvent callback); +DeviewButtonEvent glfwSetMouseButtonCallback(void* window, DeviewButtonEvent callback); +DeviewScrollEvent glfwSetScrollCallback(void* window, DeviewScrollEvent callback); +DeviewKeyEvent glfwSetKeyCallback(void* window, DeviewKeyEvent callback); +DeviewCharacterEvent glfwSetCharCallback(void* window, DeviewCharacterEvent callback); + +/* What a key types on the layout in use, unshifted, or null for a key that types nothing. The + * key is one of GLFW's numbers, or -1 for the key with that scancode. */ +const char* glfwGetKeyName(int key, int scancode); +} + +/* + * And GLFW's way to an entry point of the GL the window was made with, for the one thing asked of + * GL that rlgl has no call for: how large a texture it will take. See TextureLimit. + */ +extern "C" +{ +typedef void (*DeviewGlEntry)(void); +DeviewGlEntry glfwGetProcAddress(const char* name); +} + +/* + * And how much larger than a pixel the desktop wants everything drawn, which under X11 is the + * Xft.dpi resource over 96. raylib asks GLFW this only for a window it was told to scale itself, + * which this one is not: see State::scale. + */ +extern "C" void glfwGetWindowContentScale(void* window, float* across, float* down); + +/* GLFW's own numbers, which its headers would have named. */ +constexpr int glfwRelease = 0; +constexpr int glfwShift = 0x0001; +constexpr int glfwControl = 0x0002; +constexpr int glfwSuper = 0x0008; + namespace { void ClearCloseFlag() @@ -159,8 +212,8 @@ struct CachedTexture /* Whether the frame being built asked for this picture. What ForgetUnusedPictures keeps. */ bool used = false; - /* Sampled as its own pixels rather than smoothed: see SampleAsPixels. */ - bool point = false; + /* How it is sampled now, which is by how it was last drawn: see Sample. */ + int sampling = 0; /* Some of it can be seen through, so it is drawn over a checkerboard: see SeeThrough. */ bool translucent = false; @@ -197,6 +250,10 @@ struct Decoder std::deque requests; std::vector done; bool stopping = false; + + /* The longest side of a texture the window's GL takes, read on its thread before this one + * was started: see TextureLimit. */ + int textureLimit = 0; }; /* @@ -260,6 +317,31 @@ struct State * first drawn, so they are kept for as long as the atlas is. */ std::deque> fontData; + /* + * How much larger than a pixel the desktop wants things drawn: 1 on an ordinary display, 2 on + * one whose desktop is set to twice the size, and anything between. + * + * Nothing here asked. Under X11 a pixel is a pixel whatever the display, so the window was + * 1100 by 700 of them and its text 15 to the em on a display with twice as many to the inch, + * where everything else on the desktop is twice that: the viewer at half size. + * + * The window is drawn larger rather than handed to raylib to scale, which it would do by + * drawing the same frame through a transform: the text here is rasterised at the size it is + * shown, and everything stays in the pixels the pointer is reported in, so nothing that is + * hit tested has two sets of coordinates to keep apart. What is scaled is the font, ImGui's + * paddings and spacings, the few lengths this file gives in pixels, and the size a window + * opens at when there is none remembered. A remembered one is in pixels already. + * + * Read once, as the window is made: GLFW works it out as it starts and keeps it. Never applied + * to a capture, which draws at the size it is told at a scale of 1, on any display. + */ + float scale = 1.0f; + + /* A character cell's width and a row's height in the last frame built for the window, which + * is what the grid reported to the managed side is counted in: see MeasureGrid. */ + float cellWidth = 0.0f; + float lineHeight = 0.0f; + /* Whether the last screen carried a context menu, which is what makes Escape and a click * outside it a dismissal rather than what they would otherwise mean. */ bool menuOpen = false; @@ -269,6 +351,87 @@ struct State * nothing at all. */ float scrollRemainder = 0.0f; + /* + * What the pointer's buttons, the wheel and the keys have done since each was last taken, as + * GLFW reported it: every press and release in the order they came, every wheel message added + * up, every key pressed with the character it typed. + * + * Read as a state once a frame, which is how raylib offers them, a press and a release that + * arrived together had never happened, and of several wheel messages only the last had. A tap + * on a touchpad is such a press, and so is every click xdotool sends: under Xvfb none of ten + * clicks was seen, and three of ten presses of Page Down. + * + * The presses are ImGui's, handed over at the top of the next frame built. It takes a press and + * a release handed over together a frame apart, so what is drawn is clicked as it would be by a + * button that was held. Nothing else here asks raylib about a button, so there is no second + * account of one to disagree with ImGui's. + */ + struct Press + { + int button; + bool down; + }; + + std::vector presses; + bool held[3] = {}; + + /* The wheel twice over, because it is taken twice: by ImGui with the presses, and by + * deview_poll_input for the managed side, which is not at the same moment. */ + float wheelAcross = 0.0f; + float wheelDown = 0.0f; + float wheelNotches = 0.0f; + + /* + * A key pressed, or repeating while it is held, with the character it typed if it typed one. + * Handed to the managed side one a poll, as the other two heads hand theirs, so two keys + * between two polls are both acted on and in their order. + */ + struct KeyPress + { + /* GLFW's number for the key, which is where it is on a US keyboard, or -1 for a key it + * has no number for, and the window system's own number for it. */ + int key; + int scancode; + int mods; + bool repeated; + unsigned int character; + }; + + std::deque keys; + + /* Whether the next character GLFW reports was typed by the key press at the back of the queue, + * which is so only straight after that press: GLFW reports the two together. */ + bool characterFollows = false; + + /* A key has been pressed since Arrived last asked. */ + bool keyed = false; + + /* + * Whether the pointer is over the window, by the last crossing GLFW reported, and whether the + * last frame was built with it gone. + * + * ImGui was told where the pointer is on every frame, and raylib goes on answering with the + * last place it was in the window. So a pointer that left over a queue row was still on that + * row: it stayed lit, and its tooltip came up with the pointer on another window. A pointer + * that has left is now reported to ImGui as nowhere, which is what it has for that. + * + * Not while a button is held. The window system goes on reporting a pointer that was pressed + * in the window wherever it is taken, and a selection dragged past the window's edge has to + * go on being one. + * + * Taken to be inside until a crossing says otherwise, so a window that opens under the pointer + * and is told of no crossing is no worse off than it was. + */ + bool pointerInside = true; + bool pointerGone = false; + + /* raylib's callbacks, which go on being called. */ + DeviewCrossingEvent raylibCrossing = nullptr; + DeviewButtonEvent raylibButton = nullptr; + DeviewScrollEvent raylibScroll = nullptr; + DeviewKeyEvent raylibKey = nullptr; + DeviewCharacterEvent raylibCharacter = nullptr; + /* * The queue column, owned here rather than by the table. * @@ -340,6 +503,11 @@ struct State float centreY = 0.5f; float across = 1.0f; float down = 1.0f; + + /* Whether there is more of it across, and down, than the space shows: whether the space + * cut it short that way, which is the only way it can be moved. */ + bool movesAcross = false; + bool movesDown = false; }; PictureSpace pictureSpaces[2]; @@ -350,6 +518,7 @@ struct State * would drift by whatever each frame's clamp took off it. */ bool panning = false; + int32_t panSide = 0; ImVec2 panStart{}; PictureSpace panFrom{}; @@ -429,6 +598,13 @@ struct State State state; +/* The scale the frame being built is drawn at: the display's for the window's, and 1 for a + * capture's. See State::scale. */ +float Scale() +{ + return state.capturing ? 1.0f : state.scale; +} + /* GLFW's refresh callback, called from inside PollInputEvents: the window system has uncovered * some of the window, or shown it, and what was there is gone. */ extern "C" void WindowRefreshed(void* window) @@ -436,6 +612,100 @@ extern "C" void WindowRefreshed(void* window) state.stale = true; } +/* The five below are called from inside PollInputEvents too, each after raylib's own. */ +extern "C" void PointerCrossed(void* window, int entered) +{ + if (state.raylibCrossing != nullptr) + { + state.raylibCrossing(window, entered); + } + + state.pointerInside = entered != 0; +} + +extern "C" void ButtonChanged(void* window, int button, int action, int mods) +{ + if (state.raylibButton != nullptr) + { + state.raylibButton(window, button, action, mods); + } + + /* Left, right and middle, which GLFW and ImGui number alike. */ + if (button >= 0 && + button < 3) + { + state.presses.push_back({button, action != glfwRelease}); + state.held[button] = action != glfwRelease; + } +} + +extern "C" void WheelTurned(void* window, double across, double down) +{ + if (state.raylibScroll != nullptr) + { + state.raylibScroll(window, across, down); + } + + state.wheelAcross += static_cast(across); + state.wheelDown += static_cast(down); + state.wheelNotches += static_cast(down); +} + +extern "C" void KeyChanged(void* window, int key, int scancode, int action, int mods) +{ + if (state.raylibKey != nullptr) + { + state.raylibKey(window, key, scancode, action, mods); + } + + state.characterFollows = false; + if (action == glfwRelease) + { + return; + } + + state.keyed = true; + + /* One repeat of a key waiting at a time. A loop that was held up for seconds is handed every + * repeat the window system kept for it at once, and would go on scrolling for as long again + * after the key was let go. */ + const bool repeated = action != 1; + if (repeated) + { + for (const State::KeyPress& waiting : state.keys) + { + if (waiting.repeated && + waiting.key == key && + waiting.scancode == scancode) + { + return; + } + } + } + + state.keys.push_back({key, scancode, mods, repeated, 0}); + state.characterFollows = true; +} + +extern "C" void CharacterTyped(void* window, unsigned int codepoint) +{ + if (state.raylibCharacter != nullptr) + { + state.raylibCharacter(window, codepoint); + } + + /* Typed by the press just queued: GLFW reports a key and then its character. One that follows + * a repeat that was not queued goes with it, and one that comes by itself is what an input + * method composed, which is none of the keys read here. */ + if (state.characterFollows && + !state.keys.empty()) + { + state.keys.back().character = codepoint; + } + + state.characterFollows = false; +} + /* * Whether a remembered window would open somewhere it can be reached: its top edge on a monitor, * with enough of its width there to take hold of. Monitors come and go between runs, and a window @@ -623,38 +893,99 @@ char RowMarker(int kind) /* ---- pictures ---- */ /* - * How a picture's texture is sampled, set once as it is made. + * The three ways a picture's texture is sampled, by the size it is drawn at. * - * Bilinear, which is the whole of what a fitted picture needs: it is only ever drawn at its own - * size or smaller. Clamped at its edges rather than repeating, which is raylib's default: sampled - * at its last column, a repeating texture takes in its first, and a picture that is opaque on the - * left and clear on the right grew a line of its left edge down its right. + * Smoothed between its own pixels, which is all a picture drawn at its own size or a little under + * needs. As the pixels it has, once it is enlarged past its own size, which is what zooming that + * far in is for: smoothed, a one pixel difference between the two sides is a blur on both. And + * from its reduced copies, once it is drawn at under half its size. Smoothing looks at the four + * pixels nearest each point it samples and at none of the ones between two such points, so a + * screenshot fitted at a third of its size lost whichever of its one pixel lines fell between + * them, and its small text came apart. The reduced copies are each half the size of the one + * before, every pixel of them an average of the ones it stands for, so a thin line is fainter + * there and not gone. + */ +constexpr int sampleSmoothed = 0; +constexpr int sampleAsPixels = 1; +constexpr int sampleReduced = 2; + +/* + * How a picture's texture is sampled as it is made: smoothed. Clamped at its edges rather than + * repeating, which is raylib's default: sampled at its last column, a repeating texture takes in + * its first, and a picture that is opaque on the left and clear on the right grew a line of its + * left edge down its right. */ void PrepareTexture(CachedTexture& entry) { SetTextureFilter(entry.texture, TEXTURE_FILTER_BILINEAR); SetTextureWrap(entry.texture, TEXTURE_WRAP_CLAMP); - entry.point = false; + entry.sampling = sampleSmoothed; } -/* - * A picture enlarged past its own size is drawn as the pixels it has, which is what zooming that - * far in is for: smoothed, a one pixel difference between the two sides is a blur on both. Every - * other picture is smoothed. Changed only when it has to be, since it is a texture parameter and - * this is asked every frame. - */ -void SampleAsPixels(const std::string& path, bool point) +/* Changed only when it has to be, since it is a texture parameter and this is asked every + * frame. A picture whose reduced copies could not be made is smoothed instead. */ +void Sample(const std::string& path, int sampling) { const auto found = state.pictures.find(path); if (found == state.pictures.end() || - !found->second.loaded || - found->second.point == point) + !found->second.loaded) + { + return; + } + + CachedTexture& entry = found->second; + if (sampling == sampleReduced && + entry.texture.mipmaps <= 1) + { + sampling = sampleSmoothed; + } + + if (entry.sampling == sampling) { return; } - SetTextureFilter(found->second.texture, point ? TEXTURE_FILTER_POINT : TEXTURE_FILTER_BILINEAR); - found->second.point = point; + SetTextureFilter( + entry.texture, + sampling == sampleAsPixels ? TEXTURE_FILTER_POINT : + sampling == sampleReduced ? TEXTURE_FILTER_TRILINEAR : + TEXTURE_FILTER_BILINEAR); + entry.sampling = sampling; +} + +/* + * The longest side of a texture the window's GL will take, or zero where it would not say. + * + * A picture past it cannot be drawn. Handed to GL all the same, it came back as a texture with a + * name and no pixels, which draws as black: a box of it where the picture should be, in place of + * the nothing a picture this head cannot show is drawn as. It is 16384 under Mesa's software + * rasteriser, and a screenshot of the whole of a long page is past that. + * + * Asked of GL by name, through GLFW, since rlgl reads this number only to log it. On the thread + * that owns the context, once. + */ +int TextureLimit() +{ + static int limit = -1; + if (limit < 0) + { + limit = 0; + typedef void (*GetIntegers)(unsigned int name, int* values); + const GetIntegers getIntegers = reinterpret_cast(glfwGetProcAddress("glGetIntegerv")); + if (getIntegers != nullptr) + { + constexpr unsigned int maxTextureSize = 0x0D33; + getIntegers(maxTextureSize, &limit); + } + } + + return limit; +} + +bool FitsATexture(const Image& image, int limit) +{ + return limit <= 0 || + (image.width <= limit && image.height <= limit); } /* @@ -709,6 +1040,52 @@ bool SeeThrough(const Image& image) return false; } +/* + * A picture read off the disk and made ready to be a texture: whether any of it can be seen + * through, and its reduced copies, which are made here because here is off the window's thread + * for every picture but a capture's. The file, stb_image and raylib's resampling, and nothing + * that touches GL. A picture no texture can hold is left as it was read, since nothing will be + * made of it. + */ +Image ReadPicture(const std::string& path, int textureLimit, bool& translucent) +{ + Image image = LoadImage(path.c_str()); + translucent = SeeThrough(image); + if (image.data != nullptr && + FitsATexture(image, textureLimit)) + { + ImageMipmaps(&image); + } + + return image; +} + +/* + * The texture for a picture that has been read, or false for one there is none for: a file that + * could not be read, a picture longer on a side than a texture can be, or a context that would + * not make one. On the thread that owns the context. + */ +bool MakeTexture(const Image& image, bool translucent, CachedTexture& entry) +{ + if (image.data == nullptr || + !FitsATexture(image, TextureLimit())) + { + return false; + } + + const Texture2D texture = LoadTextureFromImage(image); + if (!IsTextureValid(texture)) + { + return false; + } + + entry.texture = texture; + entry.loaded = true; + entry.translucent = translucent; + PrepareTexture(entry); + return true; +} + void DecodeLoop(std::shared_ptr decoder) { std::unique_lock lock(decoder->mutex); @@ -724,9 +1101,7 @@ void DecodeLoop(std::shared_ptr decoder) decoder->requests.pop_front(); lock.unlock(); - /* The file and stb_image under it, and nothing that touches GL. */ - decode.image = LoadImage(decode.path.c_str()); - decode.translucent = SeeThrough(decode.image); + decode.image = ReadPicture(decode.path, decoder->textureLimit, decode.translucent); lock.lock(); if (decoder->stopping) @@ -744,6 +1119,7 @@ void RequestDecode(const std::string& path, std::uintmax_t length, std::filesyst if (!state.decoder) { state.decoder = std::make_shared(); + state.decoder->textureLimit = TextureLimit(); std::thread(DecodeLoop, state.decoder).detach(); } @@ -837,21 +1213,12 @@ bool TakeDecoded() CachedTexture& entry = found->second; entry.decoding = false; landed = true; - if (decode.image.data != nullptr) + if (MakeTexture(decode.image, decode.translucent, entry)) { - const Texture2D texture = LoadTextureFromImage(decode.image); - if (IsTextureValid(texture)) - { - entry.texture = texture; - entry.loaded = true; - entry.translucent = decode.translucent; - PrepareTexture(entry); - - /* GL hands out the name of a texture that has been unloaded again, so a frame - * drawn with this one can be, number for number, a frame drawn with the one - * that had the name before it. */ - state.stale = true; - } + /* GL hands out the name of a texture that has been unloaded again, so a frame + * drawn with this one can be, number for number, a frame drawn with the one + * that had the name before it. */ + state.stale = true; } } @@ -977,21 +1344,11 @@ const CachedTexture* Picture(const std::string& path, bool& loading) entry.used = true; if (state.capturing) { - /* What LoadTexture does, taken apart so the pixels can be looked at on the way through. */ - const Image image = LoadImage(path.c_str()); - if (image.data != nullptr) - { - const Texture2D texture = LoadTextureFromImage(image); - if (IsTextureValid(texture)) - { - entry.texture = texture; - entry.loaded = true; - entry.translucent = SeeThrough(image); - PrepareTexture(entry); - } - - UnloadImage(image); - } + /* What the decoder's thread and then TakeDecoded do, here and now. */ + bool translucent = false; + const Image image = ReadPicture(path, TextureLimit(), translucent); + MakeTexture(image, translucent, entry); + UnloadImage(image); } else { @@ -1779,56 +2136,166 @@ void PumpInput(float elapsed) io.DisplaySize = ImVec2(static_cast(GetScreenWidth()), static_cast(GetScreenHeight())); io.DeltaTime = elapsed; + /* Nowhere, for a pointer that has left the window: see State::pointerInside. */ const Vector2 mouse = GetMousePosition(); - io.AddMousePosEvent(mouse.x, mouse.y); - io.AddMouseButtonEvent(ImGuiMouseButton_Left, IsMouseButtonDown(MOUSE_BUTTON_LEFT)); - io.AddMouseButtonEvent(ImGuiMouseButton_Right, IsMouseButtonDown(MOUSE_BUTTON_RIGHT)); - io.AddMouseButtonEvent(ImGuiMouseButton_Middle, IsMouseButtonDown(MOUSE_BUTTON_MIDDLE)); + if (state.pointerGone) + { + io.AddMousePosEvent(-FLT_MAX, -FLT_MAX); + } + else + { + io.AddMousePosEvent(mouse.x, mouse.y); + } + + /* Every press and release since the last frame built, in the order they came. */ + for (const State::Press& press : state.presses) + { + io.AddMouseButtonEvent(press.button, press.down); + } + + state.presses.clear(); - const Vector2 wheel = GetMouseWheelMoveV(); - io.AddMouseWheelEvent(wheel.x, wheel.y); + if (state.wheelAcross != 0.0f || + state.wheelDown != 0.0f) + { + io.AddMouseWheelEvent(state.wheelAcross, state.wheelDown); + state.wheelAcross = 0.0f; + state.wheelDown = 0.0f; + } } -int ReadKey() +/* The one character a string is, or zero for a string that is none or several. */ +unsigned int OnlyCharacter(const char* text) { - /* Super as well as control, so a macOS keyboard driving the Linux build through a remote - * session still copies with the chord its user has in their fingers. */ - const bool control = - IsKeyDown(KEY_LEFT_CONTROL) || IsKeyDown(KEY_RIGHT_CONTROL) || - IsKeyDown(KEY_LEFT_SUPER) || IsKeyDown(KEY_RIGHT_SUPER); - if (control) + if (text == nullptr || + *text == '\0') { - /* Answered before the unmodified keys below, and returning none for anything else: without - * this ctrl+a fell through to plain A, which accepts. */ - if (IsKeyPressed(KEY_C)) return DEVIEW_KEY_COPY; - if (IsKeyPressed(KEY_A)) return DEVIEW_KEY_SELECT_ALL; - /* With control as well as without, since that is the chord everything else that zooms - * taught. By position here: a character is not reported while control is held. */ - if (IsKeyPressed(KEY_EQUAL) || IsKeyPressed(KEY_KP_ADD)) return DEVIEW_KEY_ZOOM_IN; - if (IsKeyPressed(KEY_MINUS) || IsKeyPressed(KEY_KP_SUBTRACT)) return DEVIEW_KEY_ZOOM_OUT; - if (IsKeyPressed(KEY_ZERO) || IsKeyPressed(KEY_KP_0)) return DEVIEW_KEY_ZOOM_RESET; - return DEVIEW_KEY_NONE; + return 0; } - /* The key itself held down, rather than read off the case of what was typed: see below. */ - const bool shift = IsKeyDown(KEY_LEFT_SHIFT) || IsKeyDown(KEY_RIGHT_SHIFT); + unsigned int codepoint = 0; + const int length = ImTextCharFromUtf8(&codepoint, text, nullptr); + return length > 0 && text[length] == '\0' ? codepoint : 0; +} - /* Letters by the character typed rather than by key position. raylib's key codes are - * positions on a US layout, so on AZERTY the key labelled Q reported KEY_A and accepted - a - * snapshot written into source by a key meant to quit - while the one labelled A quit. - * Characters follow the layout, the way the macOS and Windows heads already do. */ - for (int character = GetCharPressed(); character != 0; character = GetCharPressed()) +/* Whether the layout in use has a key that types a letter, unshifted. Asked of every key GLFW + * names, which are the ones that type something, by their own numbers. */ +bool LayoutTypes(unsigned int letter) +{ + for (int key = KEY_APOSTROPHE; key <= 162; key++) + { + if (OnlyCharacter(glfwGetKeyName(key, 0)) == letter) + { + return true; + } + } + + return false; +} + +/* + * Which character a key press is to be read as, in lower case, or zero for none. + * + * What it typed, where it typed something. While control is held nothing is typed, and it is what + * the key types unshifted on the layout in use, which GLFW will say: read by position, as a chord + * was, Ctrl+A on AZERTY was the key labelled Q, and Ctrl+C on Dvorak the one labelled J. + * + * A layout with no Latin letters has neither. Russian, Greek, Hebrew and Arabic type their own + * letters from the keys a US keyboard has A to Z on, so not one of this head's letters could be + * typed there, and its user had the footer's buttons and nothing else. Then the key is read as + * the letter a US keyboard has in its place, which is what such a keyboard has printed on it + * beside its own. Only where no key of the layout + * types that letter: a layout that has it somewhere else keeps it there and nowhere else, or a + * Turkish keyboard's dotless i, which sits where a US keyboard has R, would toggle the drawing. + */ +unsigned int LetterOf(const State::KeyPress& press) +{ + const unsigned int typed = press.character != 0 + ? press.character + : OnlyCharacter(glfwGetKeyName(press.key, press.scancode)); + if (typed == 0) + { + return 0; + } + + if (typed < 0x80) { /* Which letter, and nothing of its case. A capital says that Shift or Caps Lock was on and * not which of them, so read as typed Caps Lock turned a plain A into accept all - every * pending snapshot written into source, with nothing asked first, by the key that accepts * one - and left D, V, Q, N, P, M, R and J doing nothing. */ - if (character >= 'A' && character <= 'Z') + return typed >= 'A' && typed <= 'Z' ? typed + ('a' - 'A') : typed; + } + + if (press.key >= KEY_A && + press.key <= KEY_Z) + { + const unsigned int letter = static_cast(press.key - KEY_A) + 'a'; + if (!LayoutTypes(letter)) { - character += 'a' - 'A'; + return letter; } + } + + return 0; +} + +/* What one key press asks for, or none: a key this head has no use for, or a repeat of one that + * acts once however long it is held. */ +int KeyOf(const State::KeyPress& press) +{ + const unsigned int letter = LetterOf(press); - switch (character) + /* Super as well as control, so a macOS keyboard driving the Linux build through a remote + * session still copies with the chord its user has in their fingers. */ + if ((press.mods & (glfwControl | glfwSuper)) != 0) + { + if (press.repeated) + { + return DEVIEW_KEY_NONE; + } + + /* Answered before the unmodified keys below, and returning none for anything else: without + * this ctrl+a fell through to plain A, which accepts. */ + switch (letter) + { + case 'c': return DEVIEW_KEY_COPY; + case 'a': return DEVIEW_KEY_SELECT_ALL; + /* With control as well as without, since that is the chord everything else that zooms + * taught. */ + case '+': + case '=': return DEVIEW_KEY_ZOOM_IN; + case '-': return DEVIEW_KEY_ZOOM_OUT; + case '0': return DEVIEW_KEY_ZOOM_RESET; + default: break; + } + + /* And by position, as these three always were, for a layout whose key there types + * something else unshifted: AZERTY has its digits on Shift. The keypad's are the same + * keys on every layout. */ + switch (press.key) + { + case KEY_EQUAL: + case KEY_KP_ADD: return DEVIEW_KEY_ZOOM_IN; + case KEY_MINUS: + case KEY_KP_SUBTRACT: return DEVIEW_KEY_ZOOM_OUT; + case KEY_ZERO: + case KEY_KP_0: return DEVIEW_KEY_ZOOM_RESET; + default: return DEVIEW_KEY_NONE; + } + } + + /* The key itself held down, rather than read off the case of what was typed: see LetterOf. */ + const bool shift = (press.mods & glfwShift) != 0; + + /* Letters by the character typed rather than by key position. raylib's key codes are + * positions on a US layout, so on AZERTY the key labelled Q reported KEY_A and accepted - a + * snapshot written into source by a key meant to quit - while the one labelled A quit. + * Characters follow the layout, the way the macOS and Windows heads already do. Only a key + * that typed something: with Alt held none does, and Alt+A is not this head's to act on. */ + if (press.character != 0) + { + switch (letter) { /* Accept all is A with Shift held, which is what the other two heads go by. */ case 'a': return shift ? DEVIEW_KEY_ACCEPT_ALL : DEVIEW_KEY_ACCEPT; @@ -1852,14 +2319,52 @@ int ReadKey() } } - if (IsKeyPressed(KEY_UP)) return DEVIEW_KEY_SCROLL_UP; - if (IsKeyPressed(KEY_DOWN)) return DEVIEW_KEY_SCROLL_DOWN; - if (IsKeyPressed(KEY_PAGE_UP)) return DEVIEW_KEY_PAGE_UP; - if (IsKeyPressed(KEY_PAGE_DOWN)) return DEVIEW_KEY_PAGE_DOWN; - if (IsKeyPressed(KEY_HOME)) return DEVIEW_KEY_HOME; - if (IsKeyPressed(KEY_END)) return DEVIEW_KEY_END; - if (IsKeyPressed(KEY_TAB)) return shift ? DEVIEW_KEY_PREVIOUS_ITEM : DEVIEW_KEY_NEXT_ITEM; - if (IsKeyPressed(KEY_ESCAPE)) return DEVIEW_KEY_QUIT; + /* The arrows and paging go on for as long as they are held, at the rate the window system + * repeats a key: one row a press was all a held arrow scrolled. */ + switch (press.key) + { + case KEY_UP: return DEVIEW_KEY_SCROLL_UP; + case KEY_DOWN: return DEVIEW_KEY_SCROLL_DOWN; + case KEY_PAGE_UP: return DEVIEW_KEY_PAGE_UP; + case KEY_PAGE_DOWN: return DEVIEW_KEY_PAGE_DOWN; + default: break; + } + + if (press.repeated) + { + return DEVIEW_KEY_NONE; + } + + switch (press.key) + { + case KEY_HOME: return DEVIEW_KEY_HOME; + case KEY_END: return DEVIEW_KEY_END; + case KEY_TAB: return shift ? DEVIEW_KEY_PREVIOUS_ITEM : DEVIEW_KEY_NEXT_ITEM; + case KEY_ESCAPE: return DEVIEW_KEY_QUIT; + default: return DEVIEW_KEY_NONE; + } +} + +/* + * The next key waiting that asks for anything, and whether it was Escape. One a poll: the rest + * wait for the polls after it, which is what keeps two keys pressed between two polls both acted + * on, and a key pressed and let go between two of raylib's readings acted on at all. + */ +int ReadKey(bool& escape) +{ + escape = false; + while (!state.keys.empty()) + { + const State::KeyPress press = state.keys.front(); + state.keys.pop_front(); + const int key = KeyOf(press); + if (key != DEVIEW_KEY_NONE) + { + escape = press.key == KEY_ESCAPE; + return key; + } + } + return DEVIEW_KEY_NONE; } @@ -1873,9 +2378,13 @@ int ReadKey() */ void MeasureGrid() { + /* As the last frame built for the window found them, once there has been one. Between frames + * ImGui answers with the font at the size it was added at, which on a scaled display is not + * the size a frame draws it at: asked here, a window at twice the scale was told it had + * twice the rows it has. */ ImGui::SetCurrentContext(state.context); - const float width = ImGui::CalcTextSize("M").x; - const float height = ImGui::GetTextLineHeightWithSpacing(); + const float width = state.cellWidth > 0.0f ? state.cellWidth : ImGui::CalcTextSize("M").x; + const float height = state.lineHeight > 0.0f ? state.lineHeight : ImGui::GetTextLineHeightWithSpacing(); state.input.columns = width > 0.0f ? static_cast(static_cast(GetScreenWidth()) / width) : 0; @@ -2160,7 +2669,7 @@ void UpdateSelection( mouse.x < leftHit.cellLeft || /* The splitter's grab zone overlaps the left pane's edge, and a drag that started * there would otherwise also select whatever it began over. */ - (dividerX >= 0.0f && mouse.x <= dividerX + grabWidth)) + (dividerX >= 0.0f && mouse.x <= dividerX + grabWidth * Scale())) { return; } @@ -2261,7 +2770,7 @@ void DrawChecker(ImDrawList* list, const ImVec2& min, const ImVec2& max) return; } - const float repeat = checkerSize * 2.0f; + const float repeat = checkerSize * Scale() * 2.0f; list->AddImage( static_cast(checker->id), min, @@ -2388,10 +2897,13 @@ void DrawPaneImage(const DeviewScreen* screen, const DeviewPane& pane, const Pan ImVec2 size = fitted; ImVec2 uvMin(0.0f, 0.0f); ImVec2 uvMax(1.0f, 1.0f); - SampleAsPixels( + const float drawnWidth = fitted.x * (pane.imageZoom > 1.0f ? pane.imageZoom : 1.0f); + const float ownWidth = static_cast(decoded->texture.width); + Sample( path, - pane.imageZoom > 1.0f && - fitted.x * pane.imageZoom >= static_cast(decoded->texture.width)); + pane.imageZoom > 1.0f && drawnWidth >= ownWidth ? sampleAsPixels : + drawnWidth * 2.0f < ownWidth ? sampleReduced : + sampleSmoothed); if (pane.imageZoom > 1.0f) { const ImVec2 whole(fitted.x * pane.imageZoom, fitted.y * pane.imageZoom); @@ -2412,6 +2924,8 @@ void DrawPaneImage(const DeviewScreen* screen, const DeviewPane& pane, const Pan space.centreY = centreY; space.across = across; space.down = down; + space.movesAcross = std::floor(whole.x) > size.x; + space.movesDown = std::floor(whole.y) > size.y; } /* @@ -2483,8 +2997,9 @@ bool UpdatePan(const DeviewScreen* screen) return false; } - for (const State::PictureSpace& space : state.pictureSpaces) + for (int32_t side = 0; side < 2; side++) { + const State::PictureSpace& space = state.pictureSpaces[side]; if (space.present && space.enlarged && mouse.x >= space.left && @@ -2493,6 +3008,7 @@ bool UpdatePan(const DeviewScreen* screen) mouse.y < space.top + space.height) { state.panning = true; + state.panSide = side; state.panStart = mouse; state.panFrom = space; break; @@ -2505,12 +3021,33 @@ bool UpdatePan(const DeviewScreen* screen) } } - /* The picture follows the pointer, so the point at the middle moves the other way. */ + if (state.panSide >= screen->paneCount) + { + state.panning = false; + return false; + } + + /* + * The picture follows the pointer, so the point at the middle moves the other way, as far as + * this pane's picture can go. + * + * Only the way it can go at all. The centre is one point for both panes, and the two pictures + * need not be the same shape: one that is all in view from top to bottom has nowhere to go + * that way, and clamped like the other axis its report was the middle, every frame of the + * drag. So dragging it sideways took the other pane's picture back to its middle row, from + * wherever it had been dragged to. An axis this pane's picture cannot move on is reported as + * the frame's own centre, the one the managed side handed over, which leaves it where it is. + */ const State::PictureSpace& from = state.panFrom; + const DeviewPane& pane = screen->panes[state.panSide]; const float x = from.centreX - (mouse.x - state.panStart.x) / from.wholeWidth; const float y = from.centreY - (mouse.y - state.panStart.y) / from.wholeHeight; - state.input.panX = std::min(std::max(x, from.across * 0.5f), 1.0f - from.across * 0.5f); - state.input.panY = std::min(std::max(y, from.down * 0.5f), 1.0f - from.down * 0.5f); + state.input.panX = from.movesAcross + ? std::min(std::max(x, from.across * 0.5f), 1.0f - from.across * 0.5f) + : pane.imageCenterX; + state.input.panY = from.movesDown + ? std::min(std::max(y, from.down * 0.5f), 1.0f - from.down * 0.5f) + : pane.imageCenterY; if (!ImGui::IsMouseDown(ImGuiMouseButton_Left)) { @@ -2534,6 +3071,114 @@ void TextWithin(const char* begin, const char* end, float width) ImGui::RenderTextEllipsis(ImGui::GetWindowDrawList(), position, limit, limit.x, begin, end, &size); } +/* + * ImGui reads a label for more than its text. From "##" on it is the item's identity and is not + * drawn, which is how two buttons that say the same thing are told apart, and it is so for every + * label an item takes: a queue row, a menu item, a button, a pane's header. Those are a file's + * name, a test's, a solution's, and a name with "##" in it was cut short there: "Notes##2.txt" in + * the queue was "Notes". + * + * A label is never an identity here, since every item is given one by its index. So one with the + * mark in it is drawn by this side, where the item would have drawn it, over an item that is given + * no text at all. One without it is left to the item, which is every label there has been. + */ +bool Marked(const std::string& label) +{ + return label.find("##") != std::string::npos; +} + +void DrawLabel(const ImVec2& position, const std::string& label) +{ + ImGui::GetWindowDrawList()->AddText( + position, + ImGui::GetColorU32(ImGuiCol_Text), + label.data(), + label.data() + label.size()); +} + +bool SelectableLabel(const std::string& label, bool selected = false) +{ + if (!Marked(label)) + { + return ImGui::Selectable(label.c_str(), selected); + } + + /* Where Selectable puts its text: at the cursor, on the line's baseline. */ + const ImGuiWindow* window = ImGui::GetCurrentWindow(); + const ImVec2 position(window->DC.CursorPos.x, window->DC.CursorPos.y + window->DC.CurrLineTextBaseOffset); + const bool pressed = ImGui::Selectable("##label", selected); + DrawLabel(position, label); + return pressed; +} + +/* The width of a label's text, all of it. */ +float LabelWidth(const std::string& label) +{ + return ImGui::CalcTextSize(label.data(), label.data() + label.size()).x; +} + +bool ButtonLabel(const std::string& label) +{ + if (!Marked(label)) + { + return ImGui::Button(label.c_str()); + } + + /* The size Button gives itself from its text, and the text where it puts it: inside the + * frame's padding. */ + const ImGuiStyle& style = ImGui::GetStyle(); + const bool pressed = ImGui::Button( + "##label", + ImVec2( + LabelWidth(label) + style.FramePadding.x * 2.0f, + ImGui::GetTextLineHeight() + style.FramePadding.y * 2.0f)); + const ImVec2 corner = ImGui::GetItemRectMin(); + DrawLabel(ImVec2(corner.x + style.FramePadding.x, corner.y + style.FramePadding.y), label); + return pressed; +} + +/* + * The header row of the panes' table, as TableHeadersRow lays it out, for a table with a marked + * header: that one is given no text and has its own drawn over it, cut short with an ellipsis at + * the column's edge as the table would have cut it. + */ +void HeadersRow(const std::string* headers, int first, int columns) +{ + ImGui::TableNextRow(ImGuiTableRowFlags_Headers, ImGui::TableGetHeaderRowHeight()); + for (int column = 0; column < columns; column++) + { + if (!ImGui::TableSetColumnIndex(column)) + { + continue; + } + + ImGui::PushID(column); + if (column >= first && + Marked(headers[column - first])) + { + const std::string& header = headers[column - first]; + const ImVec2 position = ImGui::GetCursorScreenPos(); + ImGui::TableHeader(""); + const float edge = ImGui::TableGetCellBgRect(ImGui::GetCurrentTable(), column).Max.x; + const ImVec2 size = ImGui::CalcTextSize(header.data(), header.data() + header.size()); + ImGui::RenderTextEllipsis( + ImGui::GetWindowDrawList(), + position, + ImVec2(edge, position.y + size.y), + edge, + header.data(), + header.data() + header.size(), + &size); + } + else + { + ImGui::TableHeader(ImGui::TableGetColumnName(column)); + } + + ImGui::PopID(); + } +} + /* * How the footer is laid out: its buttons, on as many rows as the window's width makes of them, * and the status line, right aligned beside the last of those rows or on a line of its own. @@ -2577,10 +3222,8 @@ Footer LayOutFooter(const DeviewScreen* screen, float width) const DeviewButton& button = screen->buttons[index]; footer.labels.push_back(Copy(screen, button.labelOffset, button.labelLength)); - /* What ImGui::Button makes of a label: its text, less whatever follows a ##, inside the - * frame's padding. */ - const float size = - ImGui::CalcTextSize(footer.labels.back().c_str(), nullptr, true).x + style.FramePadding.x * 2.0f; + /* What a button makes of a label: its text inside the frame's padding. */ + const float size = LabelWidth(footer.labels.back()) + style.FramePadding.x * 2.0f; const bool wraps = index > 0 && reach + style.ItemSpacing.x + size > width; footer.wraps.push_back(wraps); if (wraps) @@ -2684,9 +3327,17 @@ void BuildFrame(const DeviewScreen* screen) const bool hasQueue = screen->queueCount > 0; const int columns = hasQueue ? 3 : 2; const float cell = ImGui::CalcTextSize("M").x; + if (!state.capturing) + { + state.cellWidth = cell; + state.lineHeight = ImGui::GetTextLineHeightWithSpacing(); + } + ImVec2 menuAnchor; + float menuRowTop = 0.0f; bool menuAnchored = false; - if (state.queueWidth <= 0.0f) + if (!state.capturing && + state.queueWidth <= 0.0f) { state.queueWidth = cell * queueCells; } @@ -2695,7 +3346,13 @@ void BuildFrame(const DeviewScreen* screen) * rows the table happens to have. */ const ImVec2 bodyMin = ImGui::GetCursorScreenPos(); const ImVec2 bodyAvail = ImGui::GetContentRegionAvail(); - const float queueWidth = ClampQueueWidth(state.queueWidth, bodyAvail.x, cell); + + /* A capture's queue column is the width it starts at, in its own cells. The window's is in + * the window's, which are larger on a scaled display, and may have been dragged. */ + const float queueWidth = ClampQueueWidth( + state.capturing ? cell * queueCells : state.queueWidth, + bodyAvail.x, + cell); /* Where the border between the queue and the panes ended up, read back from the table rather * than recomputed, and -1 until a row has been laid out. */ @@ -2725,9 +3382,20 @@ void BuildFrame(const DeviewScreen* screen) ImGui::TableSetupColumn(pending, ImGuiTableColumnFlags_WidthFixed, queueWidth); } - ImGui::TableSetupColumn(Copy(screen, left.headerOffset, left.headerLength).c_str()); - ImGui::TableSetupColumn(Copy(screen, right.headerOffset, right.headerLength).c_str()); - ImGui::TableHeadersRow(); + const std::string headers[] = { + Copy(screen, left.headerOffset, left.headerLength), + Copy(screen, right.headerOffset, right.headerLength)}; + ImGui::TableSetupColumn(headers[0].c_str()); + ImGui::TableSetupColumn(headers[1].c_str()); + if (Marked(headers[0]) || + Marked(headers[1])) + { + HeadersRow(headers, hasQueue ? 1 : 0, columns); + } + else + { + ImGui::TableHeadersRow(); + } int bodyRows = left.rowCount > right.rowCount ? left.rowCount : right.rowCount; if (screen->queueCount > bodyRows) @@ -2754,7 +3422,7 @@ void BuildFrame(const DeviewScreen* screen) * saying the marker can be clicked. */ ImGui::PushStyleColor(ImGuiCol_Text, ImGui::GetStyleColorVec4(ImGuiCol_TextDisabled)); ImGui::PushID(index); - if (ImGui::Selectable(label.c_str(), false)) + if (SelectableLabel(label)) { state.input.clickedQueueItem = index; } @@ -2776,7 +3444,7 @@ void BuildFrame(const DeviewScreen* screen) } ImGui::PushID(index); - if (ImGui::Selectable(label.c_str(), selected)) + if (SelectableLabel(label, selected)) { state.input.clickedQueueItem = index; } @@ -2819,6 +3487,7 @@ void BuildFrame(const DeviewScreen* screen) screen->menuCount > 0) { menuAnchor = ImVec2(ImGui::GetItemRectMin().x, ImGui::GetItemRectMax().y); + menuRowTop = ImGui::GetItemRectMin().y; menuAnchored = true; } } @@ -2896,10 +3565,11 @@ void BuildFrame(const DeviewScreen* screen) if (dividerX >= 0.0f) { const ImVec2 resume = ImGui::GetCursorScreenPos(); - ImGui::SetCursorScreenPos(ImVec2(dividerX - grabWidth, bodyMin.y)); + const float grab = grabWidth * Scale(); + ImGui::SetCursorScreenPos(ImVec2(dividerX - grab, bodyMin.y)); ImGui::InvisibleButton( "##queue-splitter", - ImVec2(grabWidth * 2.0f + 1.0f, std::max(1.0f, bodyAvail.y))); + ImVec2(grab * 2.0f + 1.0f, std::max(1.0f, bodyAvail.y))); if (ImGui::IsItemHovered() || ImGui::IsItemActive()) { ImGui::SetMouseCursor(ImGuiMouseCursor_ResizeEW); @@ -2986,13 +3656,22 @@ void BuildFrame(const DeviewScreen* screen) position = state.capturing ? ImVec2(hit.cellLeft + cell, bodyMin.y + ImGui::GetTextLineHeightWithSpacing()) : state.paneMenuAnchor; - /* Kept inside the window: a click near its right or bottom edge would otherwise hang - * most of the menu off it. */ - const ImVec2 display = ImGui::GetIO().DisplaySize; - position.x = std::max(0.0f, std::min(position.x, display.x - size.x)); - position.y = std::max(0.0f, std::min(position.y, display.y - size.y)); } + /* Kept inside the window, whichever it hangs from: a click near a pane's right or bottom + * edge would otherwise hang most of the menu off it, and under the last rows of a queue + * that fills its column there is not the height of a menu left. A row's menu goes over + * the row then rather than under it, so the row it is about can still be read. */ + const ImVec2 display = ImGui::GetIO().DisplaySize; + if (!paneMenu && + position.y + size.y > display.y) + { + position.y = menuRowTop - size.y; + } + + position.x = std::max(0.0f, std::min(position.x, display.x - size.x)); + position.y = std::max(0.0f, std::min(position.y, display.y - size.y)); + state.menuMin = position; state.menuMax = ImVec2(position.x + size.x, position.y + size.y); ImGui::SetNextWindowPos(position); @@ -3008,7 +3687,7 @@ void BuildFrame(const DeviewScreen* screen) for (int index = 0; index < screen->menuCount; index++) { ImGui::PushID(index); - if (ImGui::Selectable(labels[static_cast(index)].c_str())) + if (SelectableLabel(labels[static_cast(index)])) { state.input.clickedMenuItem = index; } @@ -3050,7 +3729,7 @@ void BuildFrame(const DeviewScreen* screen) } ImGui::PushID(index); - if (ImGui::Button(footer.labels[static_cast(index)].c_str())) + if (ButtonLabel(footer.labels[static_cast(index)])) { state.input.clickedButton = index; } @@ -3163,6 +3842,9 @@ bool Changed(const DeviewScreen* screen) * back from the managed side as a different screen. It counts all the same, because a window is * only left alone when nothing at all is happening to it. By the press rather than by what is * down, since raylib reports Caps Lock and Num Lock as held for as long as they are on. + * + * The buttons, the wheel and the keys are asked of what GLFW's callbacks kept rather than of + * raylib, for the reason they are kept: see State::presses. */ bool Arrived() { @@ -3176,26 +3858,39 @@ bool Arrived() arrived = true; } - for (const int button : {MOUSE_BUTTON_LEFT, MOUSE_BUTTON_RIGHT, MOUSE_BUTTON_MIDDLE}) + /* A press or a release waiting to be handed to ImGui, a button held, or one of a press and + * release handed over together that ImGui is keeping for the frame after. */ + if (!state.presses.empty() || + state.held[0] || + state.held[1] || + state.held[2] || + state.context->InputEventsQueue.Size > 0) { - if (IsMouseButtonDown(button) || - IsMouseButtonReleased(button)) - { - arrived = true; - } + arrived = true; + } + + if (state.wheelAcross != 0.0f || + state.wheelDown != 0.0f) + { + arrived = true; } - const Vector2 wheel = GetMouseWheelMoveV(); - if (wheel.x != 0.0f || - wheel.y != 0.0f) + /* The pointer leaving the window, or coming back to where it left from: raylib's position + * for it is the same before and after. */ + const bool gone = + !state.pointerInside && + !state.held[0] && + !state.held[1] && + !state.held[2]; + if (gone != state.pointerGone) { + state.pointerGone = gone; arrived = true; } - /* Taken off raylib's queue, which nothing else here reads: ReadKey asks about keys by name - * and takes characters from a queue of their own. */ - while (GetKeyPressed() != 0) + if (state.keyed) { + state.keyed = false; arrived = true; } @@ -3330,6 +4025,7 @@ void Rest() WaitTime(std::min(left, frameSeconds)); } + state.characterFollows = false; PollInputEvents(); state.ended = GetTime(); } @@ -3399,7 +4095,35 @@ int32_t deview_init( return 0; } + void* handle = glfwGetCurrentContext(); + + /* Not under 1: a desktop set smaller than a pixel to the pixel is not asking for text below + * the size it is legible at. And not a number that is no scale at all. */ + float across = 1.0f; + float down = 1.0f; + glfwGetWindowContentScale(handle, &across, &down); + state.scale = across > 1.0f && across <= 8.0f ? across : 1.0f; + state.cellWidth = 0.0f; + state.lineHeight = 0.0f; + state.tracked = false; + if (!sized && + state.scale != 1.0f) + { + /* The size asked for is in the pixels of an ordinary display. As much of it at this + * display's scale as its monitor has room for, and in the middle of that monitor, which is + * where raylib put the window it has just made at the size it was given. There is no + * asking the scale before there is a window to ask it of. */ + const int monitor = GetCurrentMonitor(); + const int wide = std::min(static_cast(static_cast(width) * state.scale), GetMonitorWidth(monitor)); + const int tall = std::min(static_cast(static_cast(height) * state.scale), GetMonitorHeight(monitor)); + const Vector2 origin = GetMonitorPosition(monitor); + SetWindowSize(wide, tall); + SetWindowPosition( + static_cast(origin.x) + (GetMonitorWidth(monitor) - wide) / 2, + static_cast(origin.y) + (GetMonitorHeight(monitor) - tall) / 2); + } + if (sized) { if (OnAMonitor(state.placement)) @@ -3420,7 +4144,23 @@ int32_t deview_init( /* No SetTargetFPS: raylib only holds to it inside EndDrawing, which is no longer called. The * frame is ended, and waited out, by Rest. And nothing about a window that came before this * one says anything about this one, whose clock has started again from nothing. */ - glfwSetWindowRefreshCallback(glfwGetCurrentContext(), WindowRefreshed); + glfwSetWindowRefreshCallback(handle, WindowRefreshed); + state.raylibCrossing = glfwSetCursorEnterCallback(handle, PointerCrossed); + state.pointerInside = true; + state.pointerGone = false; + state.raylibButton = glfwSetMouseButtonCallback(handle, ButtonChanged); + state.raylibScroll = glfwSetScrollCallback(handle, WheelTurned); + state.raylibKey = glfwSetKeyCallback(handle, KeyChanged); + state.raylibCharacter = glfwSetCharCallback(handle, CharacterTyped); + state.presses.clear(); + state.keys.clear(); + state.held[0] = state.held[1] = state.held[2] = false; + state.wheelAcross = 0.0f; + state.wheelDown = 0.0f; + state.wheelNotches = 0.0f; + state.scrollRemainder = 0.0f; + state.characterFollows = false; + state.keyed = false; state.presented.clear(); state.watched.clear(); state.shown = 0; @@ -3444,6 +4184,13 @@ int32_t deview_init( io.IniFilename = nullptr; io.LogFilename = nullptr; ApplyStyle(); + if (state.scale != 1.0f) + { + /* The window's context alone. A capture makes its own, which is left at 1. */ + ImGuiStyle& style = ImGui::GetStyle(); + style.ScaleAllSizes(state.scale); + style.FontScaleDpi = state.scale; + } if (fontTtf != nullptr && fontLength > 0) { @@ -3596,23 +4343,24 @@ void deview_poll_input(DeviewInput* input) if (state.initialised) { - state.input.key = ReadKey(); + bool escape = false; + state.input.key = ReadKey(escape); /* Escape with a menu up dismisses the menu. It reached the managed side as quit, which * closes the menu and then runs the command, so Esc-to-dismiss closed the viewer - and on * Linux there is no tray to open it again from, so the queue went to staging. */ if (state.menuOpen && - state.input.key == DEVIEW_KEY_QUIT && - IsKeyPressed(KEY_ESCAPE)) + escape) { state.input.key = DEVIEW_KEY_NONE; state.input.menuClosed = 1; } /* Whole notches, keeping the fraction. A touchpad sends a fraction of one per frame and - * truncating each frame on its own threw every one of them away. */ - const Vector2 wheel = GetMouseWheelMoveV(); - state.scrollRemainder += wheel.y; + * truncating each frame on its own threw every one of them away. Every wheel message since + * the last poll, added up: read off raylib it was the last of them alone. */ + state.scrollRemainder += state.wheelNotches; + state.wheelNotches = 0.0f; const int32_t notches = static_cast(state.scrollRemainder); state.scrollRemainder -= static_cast(notches); diff --git a/native/swift/Sources/Deview/Renderer.swift b/native/swift/Sources/Deview/Renderer.swift index 1e990210d..d373f9bf2 100644 --- a/native/swift/Sources/Deview/Renderer.swift +++ b/native/swift/Sources/Deview/Renderer.swift @@ -118,6 +118,21 @@ final class Renderer { var unscalable = false } + /// The pictures the last draw had no room for, each with its file as it was then. + /// + /// Nothing is decoded for one, so it is not in `pictures`, and `picturesChanged` took a picture + /// that is not there for one that has just appeared: a window too short for its picture, about + /// 176 points, was redrawn sixty times a second to draw none of it. Kept apart from `pictures` + /// because an entry there with no image is one ImageIO could not read, which is never tried + /// again, and this one is to be decoded as soon as there is room. + private var unplaced: [String: Stamp] = [:] + + /// A file as it was when it was last looked at, which is how a rewritten one is told. + private struct Stamp: Equatable { + var modified: Date + var length: UInt64 + } + /// A size in device pixels, which is what a scaled copy is made at and matched by. private struct Pixels: Equatable { var width: Int @@ -133,6 +148,10 @@ final class Renderer { private var finished: [Finished] = [] private var wanted: Set = [] + /// Something landed during a draw that painted only part of the window, or none of it, and + /// the whole of it has not been drawn since: see `draw` and `takeFinished`. + private var owed = false + private enum Finished { case decoded(path: String, modified: Date, length: UInt64, image: CGImage?) case scaled(path: String, modified: Date, length: UInt64, size: Pixels, image: CGImage?) @@ -232,9 +251,22 @@ final class Renderer { var across: CGFloat = 1 var down: CGFloat = 1 + /// The centre the frame asked for, before it was moved in to keep this space full, and + /// whether there is more of the picture than the space shows each way, by a whole point + /// or more: which is whether a drag can move it that way. + var asked: CGPoint = CGPoint(x: 0.5, y: 0.5) + var movesAcross = false + var movesDown = false + /// Where a drag of `by` points leaves the centre. The picture follows the pointer, so the /// point at the middle moves the other way, and nothing here is flipped, so a drag up the /// screen is a positive y and brings what is lower in the picture into view. + /// + /// The centre is one point for both panes, and the other pane's picture need not be this + /// one's shape. So a way this picture cannot move is reported as the frame had it, not as + /// this space holds it: all of it shows that way, which held here is the middle, and + /// reporting the middle put the other pane's picture back there on the first move of a + /// drag that was along the other axis. func dragged(by: CGSize) -> CGPoint { guard enlarged, whole.width > 0, whole.height > 0 else { return centre @@ -243,8 +275,8 @@ final class Renderer { let x = centre.x - by.width / whole.width let y = centre.y + by.height / whole.height return CGPoint( - x: min(max(x, across / 2), 1 - across / 2), - y: min(max(y, down / 2), 1 - down / 2)) + x: movesAcross ? min(max(x, across / 2), 1 - across / 2) : asked.x, + y: movesDown ? min(max(y, down / 2), 1 - down / 2) : asked.y) } } @@ -339,8 +371,18 @@ final class Renderer { /// repaint of part of it leaves out is the drawing, and a capture leaves out nothing. @discardableResult func draw(_ frame: Frame, in context: CGContext, size: CGSize, capturing: Bool = false) -> Layout { - takeFinished() + let landed = settle() dirty = capturing ? nil : context.boundingBoxOfClipPath + // Something landed in a draw that is not the whole window's, so the window is owed one. + // A turn of a spinner is clipped to the spinner, and the picture that landed as it began + // was drawn inside that clip and nowhere else: `Runtime.present` had already asked whether + // anything landed, been told no, and would not be told again. A capture is no draw of the + // window's at all. Only said here, and asked for by the next present, so a draw never + // asks for another from inside itself. + if landed, capturing || dirty?.contains(CGRect(origin: .zero, size: size)) != true { + owed = true + } + // The lines the last draw drew are the ones this one may use again, and what the draw // before it drew and it did not goes here. A draw that reached no text is passed over: // where the clip is a spinner, a turn would otherwise leave nothing kept for whatever is @@ -413,6 +455,7 @@ final class Renderer { let shown: Set = [frame.left.imagePath, frame.right.imagePath] shared = !frame.left.imagePath.isEmpty && frame.left.imagePath == frame.right.imagePath pictures = pictures.filter { shown.contains($0.key) } + unplaced = [:] gate.lock() wanted = shown gate.unlock() @@ -683,6 +726,10 @@ final class Renderer { let imageTop = top + CGFloat(pane.rows.count + 1) * line let available = CGSize(width: width - Renderer.gap, height: bottom - imageTop) guard available.width > 0, available.height > 0 else { + if hasPicture { + leftOut(pane.imagePath) + } + return } @@ -810,6 +857,14 @@ final class Renderer { layout.pictures[layout.pictures.count - 1].centre = centre layout.pictures[layout.pictures.count - 1].across = across layout.pictures[layout.pictures.count - 1].down = down + // What the frame carried, exactly, so that handing it back changes nothing. And a way + // counts as one it can move only where the space is what cut it short: a picture a + // fraction of a point wider than what shows of it has nowhere to go that can be seen. + layout.pictures[layout.pictures.count - 1].asked = CGPoint( + x: CGFloat(pane.imageCenterX), + y: CGFloat(pane.imageCenterY)) + layout.pictures[layout.pictures.count - 1].movesAcross = whole.width.rounded(.down) > shown.width + layout.pictures[layout.pictures.count - 1].movesDown = whole.height.rounded(.down) > shown.height } // Below its own size the window draws it from a copy scaled to the pixels the whole of it @@ -1043,11 +1098,21 @@ final class Renderer { return bitmap.makeImage() } + /// Whether the window has something new to draw all of itself for: a picture or a scaled copy + /// that has landed since this was last asked, whether it was put in place here or by a draw + /// that got to it first and could only paint part of the window. Asked by `Runtime.present`, + /// once a frame. + func takeFinished() -> Bool { + let landed = settle() + let late = owed + owed = false + return landed || late + } + /// Puts what `work` has finished into the pictures still waiting for it, and says whether /// anything landed, which is a reason to redraw. A result for a picture that has left the /// screen since, or for a file rewritten since, is dropped. - @discardableResult - func takeFinished() -> Bool { + private func settle() -> Bool { gate.lock() let landed = finished finished = [] @@ -1121,24 +1186,26 @@ final class Renderer { continue } - let cached = pictures[pane.imagePath] + // As the last draw saw the file: decoded or on its way, or passed over for want of room + let seen = pictures[pane.imagePath].map { Stamp(modified: $0.modified, length: $0.length) } + ?? unplaced[pane.imagePath] let attributes = try? FileManager.default.attributesOfItem(atPath: pane.imagePath) guard let modified = attributes?[.modificationDate] as? Date, let length = attributes?[.size] as? UInt64 else { // Gone: worth a redraw only to take away what was drawn - if cached != nil { + if seen != nil { return true } continue } - guard let cached else { + guard let seen else { return true } - if cached.modified != modified || cached.length != length { + if seen.modified != modified || seen.length != length { return true } } @@ -1146,6 +1213,23 @@ final class Renderer { return false } + /// Notes a picture this draw has no room for, so that `picturesChanged` knows it was seen and + /// asks for a redraw only when its file is written again. A copy decoded from the file as it + /// was before goes, since it is stale and would otherwise say so on every frame. + private func leftOut(_ path: String) { + guard let attributes = try? FileManager.default.attributesOfItem(atPath: path), + let modified = attributes[.modificationDate] as? Date, + let length = attributes[.size] as? UInt64 + else { + return + } + + unplaced[path] = Stamp(modified: modified, length: length) + if let cached = pictures[path], cached.modified != modified || cached.length != length { + pictures.removeValue(forKey: path) + } + } + /// The decoded picture at `path`, and whether it is still on its way: being decoded on `work`, /// which the pane shows a spinner for. Nil and not loading is a file ImageIO cannot read. private func picture(_ path: String, capturing: Bool) -> (image: CGImage?, loading: Bool) { diff --git a/native/swift/Sources/Deview/Runtime.swift b/native/swift/Sources/Deview/Runtime.swift index 39e0ee45d..26e1bd8ef 100644 --- a/native/swift/Sources/Deview/Runtime.swift +++ b/native/swift/Sources/Deview/Runtime.swift @@ -403,11 +403,52 @@ final class Runtime { /// Not blocked while input is still waiting to be handed over, which is the next frame's, /// now. It goes one event a poll, and a frame's wait between two of them would hand a held /// key's repeats over more slowly than a fast repeat rate makes them. + /// + /// Nor once an event has left something to hand over. The wait used to run to its deadline + /// whatever arrived during it, so a key pressed a millisecond into a frame was answered a + /// frame late. What else is already queued is still dispatched, without waiting for more. + /// + /// A window nobody can see waits longer: ordered out behind a tray, miniaturised, or wholly + /// covered. The managed loop turns only as fast as this returns, and it was turning sixty + /// times a second to present to nothing. An event still ends the wait at once, and a click + /// on the Dock icon is one. What does not is the managed side's own reason to show the + /// window, a patch arriving over the socket, which it acts on between two presents and has + /// no way to interrupt this with: `unseenWait` is how late that can be. private func pump() { - let deadline = discrete.isEmpty ? Date(timeIntervalSinceNow: 1.0 / 60.0) : Date.distantPast + let wait = unseen ? Runtime.unseenWait : 1.0 / 60.0 + var deadline = hasInput ? Date.distantPast : Date(timeIntervalSinceNow: wait) while let event = NSApp.nextEvent(matching: .any, until: deadline, inMode: .default, dequeue: true) { NSApp.sendEvent(event) + if hasInput { + deadline = Date.distantPast + } + } + } + + /// How long a pump waits for an event while nobody can see the window: ten turns of the + /// managed loop a second, and a tenth of a second at most before a window that has been + /// asked for comes forward. + private static let unseenWait: TimeInterval = 0.1 + + /// Nobody can see the window: it is ordered out, in the Dock, or has nothing of it showing. + private var unseen: Bool { + guard let window else { + return false } + + return !window.isVisible || window.isMiniaturized || !window.occlusionState.contains(.visible) + } + + /// Something is waiting for the next poll to hand over: a key or a click in line, or any of + /// what `input` gathers, by the values `resetInput` clears each to. + private var hasInput: Bool { + !discrete.isEmpty || + input.scrollDelta != 0 || + input.zoomDelta != 0 || + input.scrollTo >= 0 || + input.dragSide >= 0 || + input.panX >= 0 || + input.closeRequested != 0 } func measureGrid() { diff --git a/native/swift/Sources/Deview/ViewerView.swift b/native/swift/Sources/Deview/ViewerView.swift index 24559dd1b..7bb6d336a 100644 --- a/native/swift/Sources/Deview/ViewerView.swift +++ b/native/swift/Sources/Deview/ViewerView.swift @@ -126,6 +126,17 @@ final class ViewerView: NSView, NSViewToolTipOwner { // click that dismisses it never reaches here, which is the platform's behaviour and the // reason the first click after a menu no longer also selects a row. let point = convert(event.locationInWindow, from: nil) + + // Control held makes it the click that asks for a menu, as it is everywhere on a Mac. + // AppKit sees to that itself only for a view it can ask for an NSMenu, and this one's + // menus are the managed side's, popped a frame later: with none to give, the click came + // here as an ordinary press and selected a row or began a selection. Nothing else when + // there is no menu under it, as a right click there does nothing. + if event.modifierFlags.contains(.control) { + _ = contextClick(at: point) + return + } + if let index = layout.buttons.firstIndex(where: { $0.contains(point) }) { Runtime.shared.post(.button(Int32(index))) return @@ -179,7 +190,8 @@ final class ViewerView: NSView, NSViewToolTipOwner { if panning { // Where it is rather than how far it moved, and already inside what the space can show: - // the managed side holds one centre for both panes and knows nothing of points. + // the managed side holds one centre for both panes and knows nothing of points. A way + // this pane's picture cannot move comes back as the frame had it, for the other pane. let centre = panFrom.dragged( by: CGSize(width: point.x - panStart.x, height: point.y - panStart.y)) Runtime.shared.input.panX = Float(centre.x) @@ -277,11 +289,18 @@ final class ViewerView: NSView, NSViewToolTipOwner { } override func rightMouseDown(with event: NSEvent) { - let point = convert(event.locationInWindow, from: nil) + if !contextClick(at: convert(event.locationInWindow, from: nil)) { + super.rightMouseDown(with: event) + } + } + + /// A click that asks for a menu: the right button, or the left with control held. Says + /// whether there was anything under it to ask one for. + private func contextClick(at point: NSPoint) -> Bool { if let index = layout.queueItems.firstIndex(where: { $0.contains(point) }), index < model.queue.count { Runtime.shared.post(.rightClickedQueueItem(Int32(index))) - return + return true } // Anywhere in a pane, its text or under it: the menu is the pane's, and a file of three @@ -295,10 +314,10 @@ final class ViewerView: NSView, NSViewToolTipOwner { point.x <= layout.body.maxX { Runtime.shared.post(.rightClickedPane(point.x >= layout.panes[1].cellLeft ? 1 : 0)) Runtime.shared.paneMenuPoint = point - return + return true } - super.rightMouseDown(with: event) + return false } /// A notch of a wheel, in the points a precise device reports one movement of it as. @@ -314,11 +333,29 @@ final class ViewerView: NSView, NSViewToolTipOwner { private var scrollRemainder = 0.0 override func scrollWheel(with event: NSEvent) { - scrollRemainder += event.hasPreciseScrollingDeltas - ? event.scrollingDeltaY / ViewerView.pointsPerNotch - : event.scrollingDeltaY - let notches = scrollRemainder.rounded(.towardZero) - scrollRemainder -= notches + let notches: Double + if event.hasPreciseScrollingDeltas { + scrollRemainder += Double(event.scrollingDeltaY) / ViewerView.pointsPerNotch + notches = scrollRemainder.rounded(.towardZero) + scrollRemainder -= notches + } else { + // A wheel with notches: an event is a click of it. Its delta is in lines, scaled by + // how fast the wheel is turning, and for one click turned slowly that is a tenth of + // a line. Added up as a trackpad's are, ten such clicks went by before anything + // moved. So at least one notch the way it turned, and more only when it says more. + let lines = Double(event.scrollingDeltaY).rounded() + if event.scrollingDeltaY > 0 { + notches = max(1, lines) + } else if event.scrollingDeltaY < 0 { + notches = min(-1, lines) + } else { + // Turned sideways, which nothing here answers + notches = 0 + } + + scrollRemainder = 0 + } + guard notches != 0 else { return } @@ -409,6 +446,19 @@ final class ViewerView: NSView, NSViewToolTipOwner { break } + // The brackets by what was typed, before the letters below are matched by which key it + // was. German, French, Nordic, Spanish and Italian layouts have them behind Option, and + // with the modifiers left out the key is the digit or the letter printed on it, so those + // readers could not turn a page from the keyboard. + switch event.characters { + case "[": + return DEVIEW_KEY_PREVIOUS_PAGE.value + case "]": + return DEVIEW_KEY_NEXT_PAGE.value + default: + break + } + switch event.charactersIgnoringModifiers?.lowercased() { case "n": return DEVIEW_KEY_NEXT_CHANGE.value diff --git a/src/DiffEngine.Benchmarks/SettleBenchmarks.cs b/src/DiffEngine.Benchmarks/SettleBenchmarks.cs new file mode 100644 index 000000000..47a0b847d --- /dev/null +++ b/src/DiffEngine.Benchmarks/SettleBenchmarks.cs @@ -0,0 +1,70 @@ +using System.Net.NetworkInformation; +using BenchmarkDotNet.Attributes; + +namespace DiffEngine.Benchmarks; + +// What a passing inline verification costs with somebody owning the queue, which is a machine +// with a tray running: one settle each, told to the owner whether or not it holds anything. +// +// The time is the small part. A settle that is a connection of its own leaves that connection's +// port in TIME_WAIT once it closes, since the client is the side that closes first, and Windows +// has about 16,000 such ports and keeps each for two minutes. PortsLeftWaiting is how many one +// settle leaves behind, counted from the operating system's table once the run is over. +// +// So where every settle connects, run this with a count that stays well inside that range, or it +// uses up the ports of the machine it runs on: +// +// dotnet run -c Release --project src/DiffEngine.Benchmarks -- --filter "*SettleBenchmarks*" --invocationCount 512 --unrollFactor 16 --warmupCount 1 --iterationCount 3 +// +// Never the real port, which a tray on the machine running this owns: the owner is one this +// process binds on a port the operating system picks, and the variable is pointed at it. +[MemoryDiagnoser] +public class SettleBenchmarks +{ + static readonly ViewerMessage settle = new(ViewerVerb.Settle, InlineKey.For("Tests.cs", 1)); + + ViewerServer server = null!; + CancellationTokenSource cancel = null!; + // Null when the variable was not set, which is what putting it back has to restore + string previousPort = null!; + int settles; + + [GlobalSetup] + public void Setup() + { + if (!ViewerServer.TryBind(0, out var bound)) + { + throw new("Could not bind an ephemeral port."); + } + + server = bound; + cancel = new(); + _ = server.Listen(_ => ViewerResponse.Success(), cancel.Token); + previousPort = Environment.GetEnvironmentVariable(ViewerClient.PortVariable); + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, server.Port.ToString()); + ViewerClient.ForgetUnowned(); + } + + [GlobalCleanup] + public void Cleanup() + { + var waiting = IPGlobalProperties.GetIPGlobalProperties() + .GetActiveTcpConnections() + .Count(_ => _.RemoteEndPoint.Port == server.Port && + _.State == TcpState.TimeWait); + Console.WriteLine($"// PortsLeftWaiting: {waiting} after {settles} settles"); + Environment.SetEnvironmentVariable(ViewerClient.PortVariable, previousPort); + cancel.Cancel(); + server.Dispose(); + cancel.Dispose(); + ViewerClient.ForgetUnowned(); + } + + // DiffRunner.SettleInline's send, without its check for a build server + [Benchmark] + public bool Settle() + { + settles++; + return ViewerClient.TrySend(settle); + } +} diff --git a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs index 969180c39..f74da4776 100644 --- a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs +++ b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs @@ -69,6 +69,96 @@ static readonly (InlinePatchMode Mode, string Call)[] midLineCalls = (InlinePatchMode.Append, "Verify(\"x\").ToTask()") ]; + /// + /// Comments F# reads as one comment each, which the scanner has to read the same way or the + /// call below is taken to be inside a comment or a string. What F# does inside a comment is + /// lex it: a string is a string, a char literal is a char literal, and (*) is the + /// operator. Each is here because fsi was asked, and is asked again by + /// . + /// + internal static readonly string[] Comments = + [ + "(* returns \"*)\" when closed *)", + "(* see \"(*\" *)", + "(* the (*) operator *)", + "(* (*)*)", + // Verbatim, where a backslash escapes nothing and the string ends at the quote after it + "(* path @\"c:\\\" *)", + "(* @\"a\"\"*)\" *)", + // Regular, where it does + "(* \"a\\\"*)\" *)", + "(* \"a\\\\\" *)", + // Not interpolated: a dollar, and then whichever string follows it + "(* $\"{1}*)\" *)", + "(* $@\"c:\\\" *)", + "(* \"\"\" a \" *) \" b \"\"\" *)", + // A quote that opens nothing + "(* char '\"' *)", + "(* char '\\\"' *)", + "(* '\"' \"*)\" *)", + "(* a'\"' *)", + "(* 'a'\"'*)\" *)", + // And a tick that is no char literal, so the string after it is one + "(* it's \"*)\" *)", + "(* 'a \"*)\" *)", + "(* a (* \"*)\" *) c *)", + "(*\"*)\"*)", + "(* \"a\n*)\nb\" *)", + "(* // *)", + "(**)", + "(***)" + ]; + + /// + /// Double backticked names, which hold anything: none of these opens a string or a comment. + /// + internal static readonly string[] QuotedNames = + [ + "returns \"x", + "a (* b", + "a // b", + "it's '\"' b", + "a ` b" + ]; + + [Test] + [RequiresDotnet] + public async Task CommentsAndNamesAreReadAsTheCompilerReadsThem() + { + var expected = Convert.ToBase64String(Encoding.UTF8.GetBytes("found")); + var builder = new StringBuilder(prelude); + for (var index = 0; index < Comments.Length; index++) + { + // The comment is the first thing in the body, so a scanner that ends it early or not + // at all loses the call under it, and a compiler that reads it as anything but one + // comment has no function to call + builder.Append(Patch($"let comment{index} () =\n {Comments[index]}\n Verify(\"x\").Snapshot().ToTask()\n", 3, InlinePatchMode.Set, "found")); + builder.Append($"check \"comment{index}\" (comment{index} ()) \"{expected}\"\n\n"); + } + + for (var index = 0; index < QuotedNames.Length; index++) + { + var name = $"``{QuotedNames[index]} {index}``"; + builder.Append(Patch($"let {name} () =\n Verify(\"x\").Snapshot().ToTask()\n", 2, InlinePatchMode.Set, "found")); + builder.Append($"check \"name{index}\" ({name} ()) \"{expected}\"\n\n"); + } + + builder.Append(footer); + var path = Path.Combine(Path.GetTempPath(), $"DiffEngineFsComments_{Guid.NewGuid():N}.fsx"); + await File.WriteAllTextAsync(path, builder.ToString(), new UTF8Encoding(false)); + try + { + var (exitCode, output) = RunFsi(path); + + await Assert.That(output).Contains("ALL OK"); + await Assert.That(exitCode).IsEqualTo(0); + } + finally + { + File.Delete(path); + } + } + [Test] [RequiresDotnet] public async Task PatchedSourceCompilesAndReadsBack() @@ -90,10 +180,7 @@ public async Task PatchedSourceCompilesAndReadsBack() } } - static string BuildScript() - { - var builder = new StringBuilder(); - builder.Append( + const string prelude = """ type Chain(value: string) = member _.Snapshot(expected: string) = Chain(expected) @@ -148,7 +235,18 @@ let check (name: string) (literal: string) (expectedBase64: string) = printfn " expected %A" expected - """); + """; + + const string footer = + """ + if failures = 0 then printfn "ALL OK" else printfn "%d FAILURES" failures + exit failures + + """; + + static string BuildScript() + { + var builder = new StringBuilder(prelude); for (var index = 0; index < cases.Length; index++) { @@ -231,18 +329,22 @@ void Add(string name, string snippet, int lineHint, InlinePatchMode mode) } } - builder.Append( - """ - if failures = 0 then printfn "ALL OK" else printfn "%d FAILURES" failures - exit failures + // A Remove of a call that has a line to itself, over a line holding a Snapshot call, leaves + // its line empty, so the next apply of the same patch does not find that call where its + // own was. The empty line is between two statements of a computation expression here, + // and inside one chain, which has to still be one expression + builder.Append(Patch("let removedBang () =\n capture {\n do! Verify(\"x\")\n .Snapshot(\"dup\").ToTask()\n do! Verify(\"y\").Snapshot(\"dup\").ToTask()\n }\n", 4, InlinePatchMode.Remove, "", "dup")); + builder.Append($"check \"removedBang\" (removedBang ()) \"{Convert.ToBase64String(Encoding.UTF8.GetBytes("xdup"))}\"\n\n"); + builder.Append(Patch("let removedChain () =\n Verify(\"x\")\n .Snapshot(\"dup\")\n .Snapshot(\"kept\").ToTask()\n", 3, InlinePatchMode.Remove, "", "dup")); + builder.Append($"check \"removedChain\" (removedChain ()) \"{Convert.ToBase64String(Encoding.UTF8.GetBytes("kept"))}\"\n\n"); - """); + builder.Append(footer); return builder.ToString(); } - static string Patch(string snippet, int lineHint, InlinePatchMode mode, string content) + static string Patch(string snippet, int lineHint, InlinePatchMode mode, string content, string? originalValue = null) { - var status = InlinePatcher.TryApply(SourceLanguage.FSharp, snippet, lineHint, mode, null, null, null, null, false, content, out var patched, out var reason); + var status = InlinePatcher.TryApply(SourceLanguage.FSharp, snippet, lineHint, mode, null, originalValue, null, null, false, content, out var patched, out var reason); if (status != PatchStatus.Applied) { throw new($"{mode} patch was not applied: {reason}"); diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index efb3acff7..5050746c5 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/src/DiffEngine.Tests/InlineApplierTests.cs @@ -1403,6 +1403,128 @@ public async Task AbsentEntryPointsParseAsNull() await Assert.That(result!.EntryPoints).IsNull(); } + /// + /// An F# call is anchored by the value its literal holds, and the empty string is a value. It + /// was written as an empty field, which reads back as no value at all: the viewer then called + /// the snapshot new, and the patcher went by the line alone. + /// + [Test] + public async Task AnEmptyOriginalValueSurvives() + { + var patch = new InlinePatch("Tests.fs", 1, null, "content") + { + TestName = null, + OriginalValue = "" + }; + + var read = InlinePatchFile.TryParse(InlinePatchFile.Build(patch), out var result); + + await Assert.That(read).IsTrue(); + await Assert.That(result!.OriginalValue).IsEqualTo(""); + } + + /// + /// What a reader that predates the line is handed. The field it knows is as empty as it always + /// was, so it reads no value, as before; the line that says otherwise is one it has no name + /// for, after every line it has, and it skips those + /// (). A mark inside the field would have been + /// decoded as base64 and failed the whole payload. + /// + [Test] + public async Task AnEmptyOriginalValueIsALineAnOlderReaderSkips() + { + var patch = new InlinePatch("Tests.fs", 1, null, "a") + { + TestName = null, + Framework = "net8.0", + OriginalValue = "" + }; + + await Assert.That(InlinePatchFile.Build(patch)) + .IsEqualTo("version: 2\nsourceFile: Tests.fs\nlineHint: 1\nmode: Set\noriginalExpression: \nnewContent: YQ==\ntestName: \nframework: net8.0\noriginalValue: \nmemberName: \nentryPoints: \noriginalValueEmpty: true\n"); + } + + /// + /// Every other patch is written as it was, with no line for a reader to make anything of. + /// + [Test] + [Arguments(null)] + [Arguments("old")] + public async Task OnlyAnEmptyOriginalValueIsMarked(string? value) + { + var patch = new InlinePatch("Tests.fs", 1, null, "a") + { + TestName = null, + OriginalValue = value + }; + + var payload = InlinePatchFile.Build(patch); + + await Assert.That(payload).DoesNotContain("originalValueEmpty"); + await Assert.That(InlinePatchFile.TryParse(payload, out var result)).IsTrue(); + await Assert.That(result!.OriginalValue).IsEqualTo(value); + } + + /// + /// What a writer that predates the line sends: an empty field and nothing more, which is still + /// no value, since such a writer sends the same for both. + /// + [Test] + public async Task AnEmptyOriginalValueFieldAloneIsStillNoValue() + { + var read = InlinePatchFile.TryParse( + """ + version: 2 + sourceFile: x + lineHint: 1 + mode: Set + originalExpression: + newContent: YQ== + originalValue: + memberName: + + """, out var result); + + await Assert.That(read).IsTrue(); + await Assert.That(result!.OriginalValue).IsNull(); + } + + /// + /// The lines past the fixed six come in any order, and the line says only what an empty field + /// cannot: beside a field that holds a value, the value stands. + /// + [Test] + public async Task TheEmptyValueLineIsReadInAnyOrderAndYieldsToAValue() + { + await Assert.That(InlinePatchFile.TryParse( + """ + version: 2 + sourceFile: x + lineHint: 1 + mode: Set + originalExpression: + newContent: YQ== + originalValueEmpty: true + originalValue: + + """, out var before)).IsTrue(); + await Assert.That(before!.OriginalValue).IsEqualTo(""); + + await Assert.That(InlinePatchFile.TryParse( + """ + version: 2 + sourceFile: x + lineHint: 1 + mode: Set + originalExpression: + newContent: YQ== + originalValue: b2xk + originalValueEmpty: true + + """, out var beside)).IsTrue(); + await Assert.That(beside!.OriginalValue).IsEqualTo("old"); + } + // Matching the strictness of the other encoded fields [Test] public async Task ABadTestNameBase64Fails() diff --git a/src/DiffEngine.Tests/InlinePatcherFsTests.cs b/src/DiffEngine.Tests/InlinePatcherFsTests.cs index 309e56af8..599d23b88 100644 --- a/src/DiffEngine.Tests/InlinePatcherFsTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherFsTests.cs @@ -841,6 +841,26 @@ await Assert.That(newSource).IsEqualTo( """)); } + /// + /// A Remove applied a second time, by another framework of the run, has the same line and the + /// same value to go by. With the call's line taken out, the sibling under it had come up onto + /// that line and lost its snapshot. The line is kept, empty, which + /// asks the compiler about. + /// + [Test] + public async Task RemoveAppliedTwiceLeavesTheSiblingUnderIt() + { + var source = Test(" Verifier.Verify(a)\n .Snapshot(\"dup\")\n Verifier.Verify(b).Snapshot(\"dup\").ToTask()"); + var removed = Test(" Verifier.Verify(a)\n\n Verifier.Verify(b).Snapshot(\"dup\").ToTask()"); + + var first = TryApply(source, 6, InlinePatchMode.Remove, null, "", out var once, out _, originalValue: "dup", memberName: "MyTest"); + var second = TryApply(once, 6, InlinePatchMode.Remove, null, "", out _, out _, originalValue: "dup", memberName: "MyTest"); + + await Assert.That(first).IsEqualTo(PatchStatus.Applied); + await Assert.That(once).IsEqualTo(removed); + await Assert.That(second).IsEqualTo(PatchStatus.AlreadyApplied); + } + /// /// Bound or passed on the same line, what the call was on is still a value with the call gone, /// so the call alone is taken. The = a line above is another matter: a whole body hangs @@ -955,6 +975,48 @@ let MyTest () = await Assert.That(newSource).Contains("Snapshot(\"new\")"); } + /// + /// F# lexes inside a comment, so a close or an open written in a string there is neither, and + /// (*) there is the operator. Each of these is one comment to the compiler, which + /// asks it; read as text, the comment ended early or + /// never, and the call under it was not found. + /// + [Test] + public async Task ACommentIsLexedAsTheCompilerLexesIt() + { + foreach (var comment in FsCompilerRoundTripTests.Comments) + { + var source = Source($"module Tests\n\n{comment}\nlet MyTest () =\n Verifier.Verify(x).Snapshot().ToTask()\n"); + var commentLines = comment.Split('\n').Length; + + var status = TryApply(source, 4 + commentLines, InlinePatchMode.Set, null, "new", out var newSource, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied).Because($"{comment}: {reason}"); + await Assert.That(newSource).IsEqualTo( + Source($"module Tests\n\n{comment}\nlet MyTest () =\n Verifier.Verify(x).Snapshot(\"new\").ToTask()\n")); + } + } + + /// + /// A double backticked name holds anything, and none of it opens a string or a comment. The + /// name is still code: it is where the member a patch names is looked for. + /// + [Test] + public async Task ADoubleBacktickedNameIsSteppedOverWhole() + { + foreach (var name in FsCompilerRoundTripTests.QuotedNames) + { + var source = Source($"module Tests\n\nlet ``other {name}`` () =\n Verifier.Verify(x).Snapshot().ToTask()\n\nlet ``{name}`` () =\n Verifier.Verify(x).Snapshot().ToTask()\n"); + + // A hint that names nothing, so the member is all that finds the call + var status = TryApply(source, 1, InlinePatchMode.Set, null, "new", out var newSource, out var reason, memberName: name); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied).Because($"{name}: {reason}"); + await Assert.That(newSource).IsEqualTo( + Source($"module Tests\n\nlet ``other {name}`` () =\n Verifier.Verify(x).Snapshot().ToTask()\n\nlet ``{name}`` () =\n Verifier.Verify(x).Snapshot(\"new\").ToTask()\n")); + } + } + [Test] public async Task CallInsideAStringIsSkipped() { diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index df92b9d47..49acbb006 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -174,6 +174,104 @@ public async Task RemoveTakesTheCallTheAnchorNamesRatherThanTheNearest() await Assert.That(newSource).Contains("Snapshot(\"two\")"); } + /// + /// A Remove is applied once per framework, and once per case of a test that ignores its + /// parameters. Taking the call's line out brought the statement under it up onto the line the + /// patch names, and where that statement held the same literal the second apply had the same + /// line and the same anchor to go by: the sibling lost its snapshot. So the line stays, empty, + /// and the second apply reads it as the call already removed. + /// + [Test] + public async Task RemoveAppliedTwiceLeavesTheSiblingUnderIt() + { + var source = Method( + """ + await Verify(a) + .Snapshot("dup"); + await Verify(b).Snapshot("dup"); + """); + var removed = Method(" await Verify(a);\n\n await Verify(b).Snapshot(\"dup\");"); + + var first = TryApply(source, 6, InlinePatchMode.Remove, "\"dup\"", "", out var once, out _, memberName: "Test"); + var second = TryApply(once, 6, InlinePatchMode.Remove, "\"dup\"", "", out _, out _, memberName: "Test"); + + await Assert.That(first).IsEqualTo(PatchStatus.Applied); + await Assert.That(once).IsEqualTo(removed); + await Assert.That(second).IsEqualTo(PatchStatus.AlreadyApplied); + } + + /// + /// The same with a literal over several lines, which is what a snapshot usually is. One line + /// is kept and not one for each the call ran over: the line the call started on is the one a + /// patch names. + /// + [Test] + public async Task RemoveOfACallOverSeveralLinesAppliedTwiceLeavesTheSiblingUnderIt() + { + var literal = "\"\"\"\n dup\n \"\"\""; + var source = Method($" await Verify(a)\n .Snapshot(\n {literal});\n await Verify(b).Snapshot(\n {literal});"); + var removed = Method($" await Verify(a);\n\n await Verify(b).Snapshot(\n {literal});"); + + var first = TryApply(source, 6, InlinePatchMode.Remove, literal, "", out var once, out _, memberName: "Test"); + var second = TryApply(once, 6, InlinePatchMode.Remove, literal, "", out _, out _, memberName: "Test"); + + await Assert.That(first).IsEqualTo(PatchStatus.Applied); + await Assert.That(once).IsEqualTo(removed); + await Assert.That(second).IsEqualTo(PatchStatus.AlreadyApplied); + } + + /// + /// The line is kept only for a Snapshot call under it. Anything else that comes up onto the + /// recorded line is read as the call removed already, so the line goes, as it always did. + /// + [Test] + public async Task RemoveAppliedTwiceWithNoSnapshotCallUnderItKeepsNoLine() + { + var source = Method(" await Verify(a)\n .Snapshot(\"dup\");\n await Verify(b);\n await Verify(c).Snapshot(\"dup\");"); + var removed = Method(" await Verify(a);\n await Verify(b);\n await Verify(c).Snapshot(\"dup\");"); + + var first = TryApply(source, 6, InlinePatchMode.Remove, "\"dup\"", "", out var once, out _, memberName: "Test"); + var second = TryApply(once, 6, InlinePatchMode.Remove, "\"dup\"", "", out _, out _, memberName: "Test"); + + await Assert.That(first).IsEqualTo(PatchStatus.Applied); + await Assert.That(once).IsEqualTo(removed); + await Assert.That(second).IsEqualTo(PatchStatus.AlreadyApplied); + } + + /// + /// The kept line goes in after whatever followed the call on its line, and where that opens a + /// literal running onto the next line, a line break there would be more content. The call is + /// taken from where it stands then, and what followed it keeps the line. + /// + [Test] + public async Task RemoveKeepsTheLineWithoutBreakingALiteralThatFollowsTheCall() + { + var source = Method(" await Verify(a)\n .Snapshot(\"dup\").UseTextForParameters(@\"one\ntwo\"); await Verify(b).Snapshot(\"dup\");"); + var removed = Method(" await Verify(a)\n .UseTextForParameters(@\"one\ntwo\"); await Verify(b).Snapshot(\"dup\");"); + + var first = TryApply(source, 6, InlinePatchMode.Remove, "\"dup\"", "", out var once, out _, memberName: "Test"); + var second = TryApply(once, 6, InlinePatchMode.Remove, "\"dup\"", "", out _, out _, memberName: "Test"); + + await Assert.That(first).IsEqualTo(PatchStatus.Applied); + await Assert.That(once).IsEqualTo(removed); + await Assert.That(second).IsEqualTo(PatchStatus.AlreadyApplied); + } + + /// + /// The line breaks of a file are its own, and the kept line is written with the one that was + /// in front of the call. A comment after the call goes up with what else followed it. + /// + [Test] + public async Task RemoveKeepsTheLineWithTheFilesLineBreak() + { + var source = "await Verify(a)\r\n .Snapshot(\"dup\"); // gone\r\nawait Verify(b).Snapshot(\"dup\");\r\n"; + + var status = TryApply(source, 2, InlinePatchMode.Remove, "\"dup\"", "", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).IsEqualTo("await Verify(a); // gone\r\n\r\nawait Verify(b).Snapshot(\"dup\");\r\n"); + } + /// /// And an anchor that matches nothing is reported rather than resolved to the nearest call. /// diff --git a/src/DiffEngine.Tests/KeptConnectionTests.cs b/src/DiffEngine.Tests/KeptConnectionTests.cs new file mode 100644 index 000000000..aab1ecaec --- /dev/null +++ b/src/DiffEngine.Tests/KeptConnectionTests.cs @@ -0,0 +1,363 @@ +/// +/// The connection the telling sends share, where the owner keeps one. +/// +/// A settle was a connection of its own, and the client is the side that closes first, so each +/// left a port in TIME_WAIT: one a passing inline verification, for two minutes, out of the +/// 16,000 or so Windows gives out. What is asserted here is the count of connections an owner +/// accepted, and that nothing a send used to do is lost to sharing one: an owner that predates +/// it is still sent a connection each, and an owner that goes is still found gone. +/// +/// +// The connection is one for the process, as the memory beside it is +[NotInParallel] +public class KeptConnectionTests +{ + static readonly ViewerMessage settle = new(ViewerVerb.Settle, InlineKey.For("Tests.cs", 1)); + + [Before(Test)] + public void Forget() => + ViewerClient.ForgetUnowned(); + + [After(Test)] + public void Restore() => + ViewerClient.ForgetUnowned(); + + /// + /// The first is an ordinary exchange, since its reply is what says the owner keeps one. The + /// second connection is the kept one, and everything after goes down it. + /// + [Test] + public async Task SendsAfterTheFirstShareAConnection() + { + using var owner = new Owner(); + + for (var index = 0; index < 20; index++) + { + await Assert.That(ViewerClient.Tell(settle, owner.Port)).IsTrue(); + } + + await Assert.That(owner.Heard.Count).IsEqualTo(20); + await Assert.That(owner.Accepted).IsEqualTo(2); + } + + /// + /// What the owner said is still what the send reports, a refusal included. + /// + [Test] + public async Task ARefusalComesBackDownTheKeptConnection() + { + using var owner = new Owner(_ => _.Key == "refuse" ? ViewerResponse.Error("no") : ViewerResponse.Success()); + + await Assert.That(ViewerClient.Tell(settle, owner.Port)).IsTrue(); + await Assert.That(ViewerClient.Tell(new(ViewerVerb.Settle, "refuse"), owner.Port)).IsFalse(); + await Assert.That(ViewerClient.Tell(settle, owner.Port)).IsTrue(); + + await Assert.That(owner.Accepted).IsEqualTo(2); + } + + /// + /// Everything a message carries crosses the kept connection as it crosses its own. + /// + [Test] + public async Task AMessageArrivesWhole() + { + using var owner = new Owner(); + var message = new ViewerMessage(ViewerVerb.Settle, "key\nwith a break", "net10.0", "Member", "a value\n\nwith an empty line"); + + ViewerClient.Tell(settle, owner.Port); + await Assert.That(ViewerClient.Tell(message, owner.Port)).IsTrue(); + + await Assert.That(owner.Heard[1]).IsEqualTo(message); + } + + /// + /// A parallel run's settles take turns at the one connection, and each is answered with its + /// own answer. Several may go out as ordinary exchanges before the first of them has opened + /// it, so the bound is on the order of the threads rather than of the sends. + /// + [Test] + public async Task SendsFromManyThreadsAreEachAnswered() + { + using var owner = new Owner(_ => _.Key!.EndsWith("odd") ? ViewerResponse.Error("odd") : ViewerResponse.Success()); + var wrong = 0; + + // Threads of their own rather than the pool's. A send blocks its thread until it is + // answered, and this owner answers from the pool, being in the same process: on a machine + // of two or four cores eight blocked senders were every thread the pool had, the owner + // could not answer, and a send timed out. A real owner is another process. + var threads = Enumerable.Range(0, 8) + .Select(_ => Task.Factory.StartNew( + () => + { + for (var index = 0; index < 50; index++) + { + var odd = (_ + index) % 2 == 1; + var sent = ViewerClient.Tell(new(ViewerVerb.Settle, odd ? "odd" : "even"), owner.Port); + if (sent == odd) + { + Interlocked.Increment(ref wrong); + } + } + }, + Cancel.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default)); + await Task.WhenAll(threads); + + await Assert.That(wrong).IsEqualTo(0); + await Assert.That(owner.Heard.Count).IsEqualTo(400); + await Assert.That(owner.Accepted).IsLessThan(20); + } + + /// + /// An owner from before any of this reads a request until its sender closes, so it cannot be + /// kept a connection, and is not: its replies do not say it keeps one. + /// + [Test] + public async Task AnOwnerThatPredatesItIsSentAConnectionEach() + { + using var owner = new OlderOwner(); + + for (var index = 0; index < 5; index++) + { + await Assert.That(ViewerClient.Tell(settle, owner.Port)).IsTrue(); + } + + var requests = owner.Requests; + await Assert.That(requests.Count).IsEqualTo(5); + foreach (var request in requests) + { + await Assert.That(request).IsEqualTo(settle.Build()); + } + } + + /// + /// And a client from before it reads past the line that says so, as it reads past any name + /// it does not know. + /// + [Test] + public async Task AnOlderClientReadsPastTheLine() + { + var text = $"{ViewerResponse.Success("done").Build()}{ViewerServer.Keeps}\n"; + + await Assert.That(ViewerResponse.TryParse(text, out var response)).IsTrue(); + await Assert.That(response!.Ok).IsTrue(); + await Assert.That(response.Message).IsEqualTo("done"); + } + + /// + /// An owner that stops closes what it kept, so the next send finds the port as it is now: + /// with nobody on it. + /// + [Test] + public async Task AnOwnerThatHasGoneIsFoundGone() + { + int port; + using (var owner = new Owner()) + { + port = owner.Port; + ViewerClient.Tell(settle, port); + await Assert.That(ViewerClient.Tell(settle, port)).IsTrue(); + } + + await Assert.That(ViewerClient.Tell(settle, port)).IsFalse(); + await Assert.That(ViewerClient.FoundUnowned(port)).IsTrue(); + } + + /// + /// Or with the next owner on it, which the send reaches rather than failing on the last one's + /// connection. + /// + [Test] + public async Task TheNextOwnerOfThePortIsReached() + { + int port; + using (var first = new Owner()) + { + port = first.Port; + ViewerClient.Tell(settle, port); + await Assert.That(ViewerClient.Tell(settle, port)).IsTrue(); + } + + using var next = new Owner(port: port); + + await Assert.That(ViewerClient.Tell(settle, port)).IsTrue(); + await Assert.That(ViewerClient.Tell(settle, port)).IsTrue(); + await Assert.That(next.Heard.Count).IsEqualTo(2); + } + + /// + /// A send to another port leaves the one connection for that port's owner, and the first + /// owner is still reached afterwards. + /// + [Test] + public async Task EachPortIsSentItsOwn() + { + using var first = new Owner(); + using var second = new Owner(); + + for (var index = 0; index < 3; index++) + { + await Assert.That(ViewerClient.Tell(settle, first.Port)).IsTrue(); + await Assert.That(ViewerClient.Tell(settle, second.Port)).IsTrue(); + } + + await Assert.That(first.Heard.Count).IsEqualTo(3); + await Assert.That(second.Heard.Count).IsEqualTo(3); + } + + /// + /// An owner that is there and too busy to answer inside the timeout. The send is given up + /// on, as one on a connection of its own is, and is not then sent a second time to wait + /// again; the send after it starts over and is answered. + /// + [Test] + public async Task AnOwnerTooBusyToAnswerIsNotWaitedOnTwice() + { + using var release = new ManualResetEventSlim(); + using var owner = new Owner(_ => + { + if (_.Key == "slow") + { + release.Wait(TimeSpan.FromSeconds(30)); + } + + return ViewerResponse.Success(); + }); + ViewerClient.Tell(settle, owner.Port); + + var watch = Stopwatch.StartNew(); + var sent = ViewerClient.Tell(new(ViewerVerb.Settle, "slow"), owner.Port); + watch.Stop(); + release.Set(); + + await Assert.That(sent).IsFalse(); + // The client's timeout is three seconds, and twice that is what sending it again cost + await Assert.That(watch.Elapsed).IsLessThan(TimeSpan.FromSeconds(5.5)); + await Assert.That(owner.Heard.Count(_ => _.Key == "slow")).IsEqualTo(1); + await Assert.That(ViewerClient.Tell(settle, owner.Port)).IsTrue(); + } + + /// + /// A queue owner on a port of the test's choosing, recording what it was sent. + /// + sealed class Owner : IDisposable + { + readonly ViewerServer server; + readonly CancelSource cancel = new(); + readonly Task listening; + readonly List heard = []; + + public Owner(Func? answer = null, int port = 0) + { + if (!ViewerServer.TryBind(port, out var bound)) + { + throw new($"Could not bind port {port}."); + } + + server = bound; + listening = server.Listen( + _ => + { + lock (heard) + { + heard.Add(_); + } + + return answer?.Invoke(_) ?? ViewerResponse.Success(); + }, + cancel.Token); + } + + public int Port => server.Port; + + public int Accepted => server.Accepted; + + public IReadOnlyList Heard + { + get + { + lock (heard) + { + return heard.ToList(); + } + } + } + + public void Dispose() + { + cancel.Cancel(); + server.Dispose(); + try + { + listening.Wait(TimeSpan.FromSeconds(2)); + } + catch (AggregateException) + { + // Cancellation unwinding through the listener; nothing to report + } + + cancel.Dispose(); + } + } + + /// + /// The owner every released tray and viewer is: one request a connection, read until the + /// client closes its half, answered, and closed. + /// + sealed class OlderOwner : IDisposable + { + readonly TcpListener listener = new(IPAddress.Loopback, 0); + readonly List requests = []; + + public OlderOwner() + { + listener.Start(); + Port = ((IPEndPoint) listener.LocalEndpoint).Port; + _ = Task.Run(Serve); + } + + public int Port { get; } + + public IReadOnlyList Requests + { + get + { + lock (requests) + { + return requests.ToList(); + } + } + } + + async Task Serve() + { + try + { + while (true) + { + using var client = await listener.AcceptTcpClientAsync(); + using var stream = client.GetStream(); + using var reader = new StreamReader(stream, Encoding.UTF8); + var request = await reader.ReadToEndAsync(); + lock (requests) + { + requests.Add(request); + } + + var reply = Encoding.UTF8.GetBytes(ViewerResponse.Success().Build()); + await stream.WriteAsync(reply, 0, reply.Length); + await stream.FlushAsync(); + } + } + catch (Exception exception) + when (exception is SocketException or ObjectDisposedException or InvalidOperationException or IOException) + { + // Stopped + } + } + + public void Dispose() => + listener.Stop(); + } +} diff --git a/src/DiffEngine.Tests/PsColumnsTests.cs b/src/DiffEngine.Tests/PsColumnsTests.cs new file mode 100644 index 000000000..b2e543cf9 --- /dev/null +++ b/src/DiffEngine.Tests/PsColumnsTests.cs @@ -0,0 +1,43 @@ +#if NET10_0 +/// +/// A terminal width in the environment. Shells keep COLUMNS for themselves and some setups export +/// it, and procps then takes it over the unlimited width it gives a pipe: every command line came +/// back cut to that many characters, so a diff tool running with two long paths was never found, +/// and never killed. +/// +// The variable is the test process's own, which every ps another test starts would inherit +[NotInParallel] +[RunOn(TUnit.Core.Enums.OS.Linux | TUnit.Core.Enums.OS.MacOs)] +public class PsColumnsTests +{ + [Test] + public async Task AnExportedWidthDoesNotCutCommandLines() + { + // Past any width a terminal has, with the part that tells it from every other process at + // the far end, where a cut loses it + var marker = new string('a', 300) + Guid.NewGuid().ToString("N"); + var previous = Environment.GetEnvironmentVariable("COLUMNS"); + // A list rather than one command, so that the shell stays to run it instead of becoming + // the sleep, and the marker stays on a command line as its $0 + using var process = Process.Start( + new ProcessStartInfo("sh", $"-c \"sleep 30; true\" {marker}") + { + UseShellExecute = false + })!; + Environment.SetEnvironmentVariable("COLUMNS", "80"); + try + { + var commands = LinuxOsxProcess.FindAll(); + + var found = commands.Where(_ => _.Process == process.Id).ToList(); + await Assert.That(found.Count).IsEqualTo(1); + await Assert.That(found[0].Command).EndsWith(marker); + } + finally + { + Environment.SetEnvironmentVariable("COLUMNS", previous); + process.Kill(true); + } + } +} +#endif diff --git a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs index 8aa105661..945400d83 100644 --- a/src/DiffEngine.Tests/ViewerClientUnownedTests.cs +++ b/src/DiffEngine.Tests/ViewerClientUnownedTests.cs @@ -256,6 +256,54 @@ public async Task AnOwnerThatJustAnsweredIsNotLookedUpAgain() } } + /// + /// A send the caller cancelled says nothing about the port. Cancelling closes the socket, the + /// one thing that unblocks every framework, and what the closed socket threw was read as a + /// connect that failed: the port was remembered as unowned, with its owner listening, and + /// every settle and move after it went unsent for ten minutes. + /// + [Test] + public async Task ASendCancelledBeforeItStartsSaysNothingAboutThePort() + { + using var owner = new Owner(); + using var cancel = new CancelSource(); + cancel.Cancel(); + + await Assert.That(async () => await ViewerClient.SendAsync(settle, cancel.Token, owner.Port)) + .Throws(); + + await Assert.That(ViewerClient.FoundUnowned(owner.Port)).IsFalse(); + await Assert.That(ViewerClient.TrySend(settle, out _, owner.Port, skipIfUnowned: true)).IsTrue(); + await Assert.That(owner.Heard.Count).IsEqualTo(1); + } + + /// + /// The same while the connect is still out. A port that is bound and not listening is the + /// connect that goes unanswered: Windows spends two seconds on it, which the cancel arrives + /// inside. The table is made unreadable so that the connect is reached at all. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Windows)] + public async Task ASendCancelledWhileConnectingSaysNothingAboutThePort() + { + using var holder = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) + { + ExclusiveAddressUse = true + }; + holder.Bind(new IPEndPoint(IPAddress.Loopback, 0)); + var port = ((IPEndPoint) holder.LocalEndPoint!).Port; + using var unreadable = new Lookup(port) + { + Unreadable = true + }; + using var cancel = new CancelSource(TimeSpan.FromMilliseconds(200)); + + await Assert.That(async () => await ViewerClient.SendAsync(settle, cancel.Token, port)) + .Throws(); + + await Assert.That(ViewerClient.FoundUnowned(port)).IsFalse(); + } + /// /// Stands in front of the listener table for the ports a test names, counting how often each /// was asked about and, when told to, failing the way a table that cannot be read does. Every diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 6d9c556de..60dbf8cf4 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -1041,6 +1041,57 @@ public async Task ACancelledTokenStopsTheListenerWhateverTheCode() await Assert.That(ViewerServer.IsStop(new((int) SocketError.ConnectionReset), cancel.Token)).IsTrue(); } + /// + /// An accept that keeps failing, which is what a process out of descriptors gets: each one + /// refused at once, with the connection left in the backlog, until something is closed. + /// Retried with nothing between them that was ten thousand failed accepts a second, measured + /// on Linux under a low descriptor limit, and a core for as long as it went on. + /// + /// Nothing but failures is the first, the retry it gets at once, and then one each + /// . The bound is twice that rate and some, which + /// is thousands of times under what the loop made of it before. + /// + /// + /// The retry is waited for, and the stretch that is counted is timed, rather than either + /// being taken to fit in half a second. On a runner of four cores with the rest of the suite + /// running, the loop's first continuation had not been given a thread in that long. + /// + /// + [Test] + public async Task AnAcceptThatKeepsFailingIsWaitedOut() + { + using var cancel = new CancelSource(); + var accepts = 0; + + async Task Accept(Cancel token) + { + Interlocked.Increment(ref accepts); + // So the loop is left between failures, as a real accept leaves it, rather than + // holding the thread that started it + await Task.Yield(); + throw new SocketException((int) SocketError.TooManyOpenSockets); + } + + var serving = ViewerServer.Serve(Accept, _ => _.Dispose(), cancel.Token); + var retried = Stopwatch.StartNew(); + while (Volatile.Read(ref accepts) < 2 && + retried.Elapsed < TimeSpan.FromSeconds(30)) + { + await Task.Delay(10); + } + + var before = Volatile.Read(ref accepts); + var counted = Stopwatch.StartNew(); + await Task.Delay(500); + var during = Volatile.Read(ref accepts) - before; + var waits = counted.Elapsed.TotalMilliseconds / ViewerServer.FailedAcceptWait.TotalMilliseconds; + cancel.Cancel(); + await Wait(serving); + + await Assert.That(before).IsGreaterThan(1); + await Assert.That(during).IsLessThan((int) (waits * 2) + 5); + } + /// /// An owner that answers with an error is not an absent one. Collapsing the two into false /// meant a refused inline was read as "nobody is there", so a second viewer was launched, it diff --git a/src/DiffEngine.Tests/ViewerThatCannotStartTests.cs b/src/DiffEngine.Tests/ViewerThatCannotStartTests.cs index a7d7d6688..7f5784d71 100644 --- a/src/DiffEngine.Tests/ViewerThatCannotStartTests.cs +++ b/src/DiffEngine.Tests/ViewerThatCannotStartTests.cs @@ -38,7 +38,30 @@ public async Task AnInlineSnapshotIsNotCalledQueued() await Assert.That(result).IsEqualTo(InlineResult.NoViewerFound); // The viewer reads its payload file and deletes it, and this one never got that far, so // the file is the launcher's to take back - await Assert.That(PayloadFiles().Except(before)).IsEmpty(); + await Assert.That(await LeftBehind(before)).IsEmpty(); + } + + /// + /// The payload files that are there now and were not before, once any that are someone + /// else's have had time to go. The temp folder is shared, and this test runs in the net48 + /// process and the net10.0 one at the same time: each saw the other's file in the moment + /// between it being written and taken back, and failed for it. One this launch left behind + /// stays, however long it is waited for. + /// + static async Task> LeftBehind(List before) + { + var waited = Stopwatch.StartNew(); + while (true) + { + var left = PayloadFiles().Except(before).ToList(); + if (left.Count == 0 || + waited.Elapsed > TimeSpan.FromSeconds(40)) + { + return left; + } + + await Task.Delay(100); + } } static List PayloadFiles() => diff --git a/src/DiffEngine/Inline/FsLanguage.cs b/src/DiffEngine/Inline/FsLanguage.cs index e1f8fcb36..0d10bf98f 100644 --- a/src/DiffEngine/Inline/FsLanguage.cs +++ b/src/DiffEngine/Inline/FsLanguage.cs @@ -79,6 +79,15 @@ internal override SourceScan Scan(string source) continue; } + break; + case '`': + if (TrySkipQuotedIdentifier(source, ref index)) + { + // Stepped over and not recorded: it is a name, which a search for a + // member has to find in code + continue; + } + break; case '\'': // Only where the tick cannot be part of the name in front of it, and only @@ -181,6 +190,19 @@ static bool TrySkipLineComment(string source, ref int index) /// /// Block comments nest, so the scan counts them rather than stopping at the first close. + /// + /// And F# lexes inside one, which is what lets a comment hold commented out code: a string in + /// a comment is a string, so (* returns "*)" when closed *) and (* see "(*" *) + /// are one comment each, and (*) inside one is the operator, neither opening nor + /// closing anything. Read as text, the first ended at the quoted close with a string opening + /// after it, and the other two never ended, and either way the calls below were inside + /// something and not found. What fsi was seen to take as a token in a comment is what is + /// stepped over here: a regular string with its escapes, a verbatim one only as @", a + /// triple quoted one, and a char literal, which is where a quote that opens no string is + /// written. No interpolation, where $" is a dollar and then a regular string, and no + /// backticks, which are text in a comment. FsCompilerRoundTripTests holds each shape to + /// the compiler. + /// /// static bool TrySkipBlockComment(string source, ref int index) { @@ -193,16 +215,27 @@ static bool TrySkipBlockComment(string source, ref int index) var depth = 1; while (cursor < source.Length) { - if (StartsBlockComment(source, cursor)) + var ch = source[cursor]; + var next = cursor + 1 < source.Length ? source[cursor + 1] : '\0'; + if (ch == '(' && + next == '*') { + if (cursor + 2 < source.Length && + source[cursor + 2] == ')') + { + // The operator, as it is in code. Stepped over whole, or its last two + // characters would close the comment + cursor += 3; + continue; + } + depth++; cursor += 2; continue; } - if (source[cursor] == '*' && - cursor + 1 < source.Length && - source[cursor + 1] == ')') + if (ch == '*' && + next == ')') { depth--; cursor += 2; @@ -215,6 +248,21 @@ static bool TrySkipBlockComment(string source, ref int index) continue; } + if (ch == '"' || + (ch == '@' && next == '"')) + { + // Never false from here: both start a string. One left open runs to the end of + // the file, which the compiler refuses outright + TrySkipStringLike(source, ref cursor); + continue; + } + + if (ch == '\'' && + TrySkipCharLiteral(source, ref cursor)) + { + continue; + } + cursor++; } @@ -223,6 +271,48 @@ static bool TrySkipBlockComment(string source, ref int index) return true; } + /// + /// A double backticked identifier, which is how an F# test is usually named and may hold + /// anything a line can but a tab and two backticks together: + /// ``returns "x" (* when asked``. Nothing inside one opens a string or a comment. + /// + static bool TrySkipQuotedIdentifier(string source, ref int index) + { + if (index + 1 >= source.Length || + source[index + 1] != '`') + { + return false; + } + + var cursor = index + 2; + while (cursor < source.Length) + { + var ch = source[cursor]; + if (ch is '\n' or '\r' or '\t') + { + return false; + } + + if (ch == '`' && + cursor + 1 < source.Length && + source[cursor + 1] == '`') + { + // Two backticks with nothing between them and the opening pair name nothing + if (cursor == index + 2) + { + return false; + } + + index = cursor + 2; + return true; + } + + cursor++; + } + + return false; + } + static bool StartsBlockComment(string source, int index) => index + 1 < source.Length && source[index] == '(' && diff --git a/src/DiffEngine/Inline/InlinePatchFile.cs b/src/DiffEngine/Inline/InlinePatchFile.cs index 33dfcfd36..bf927e8bb 100644 --- a/src/DiffEngine/Inline/InlinePatchFile.cs +++ b/src/DiffEngine/Inline/InlinePatchFile.cs @@ -76,9 +76,20 @@ public static string Build(InlinePatch patch, string? framework = null) var entryPoints = patch.EntryPoints is null || patch.EntryPoints.Length == 0 ? "" : Convert.ToBase64String(Encoding.UTF8.GetBytes(string.Join(',', patch.EntryPoints))); - return $"version: 2\nsourceFile: {patch.SourceFile}\nlineHint: {patch.LineHint}\nmode: {patch.Mode}\noriginalExpression: {expression}\nnewContent: {content}\ntestName: {testName}\nframework: {framework}\noriginalValue: {value}\nmemberName: {memberName}\nentryPoints: {entryPoints}\n"; + // An empty field is read back as no value, and a snapshot whose value is the empty string + // has one: it is the anchor an F# call is matched by, and without it the call reads as a + // new snapshot and is found by its line alone. Said on a line of its own, and only when + // it is so, because nothing can be put in the field itself: a reader that predates this + // decodes whatever is there as base64 and rejects the whole payload when it is not, where + // a line it does not know it skips, leaving it with what it had before + var emptyValue = patch.OriginalValue is { Length: 0 } + ? $"{originalValueEmpty}: true\n" + : ""; + return $"version: 2\nsourceFile: {patch.SourceFile}\nlineHint: {patch.LineHint}\nmode: {patch.Mode}\noriginalExpression: {expression}\nnewContent: {content}\ntestName: {testName}\nframework: {framework}\noriginalValue: {value}\nmemberName: {memberName}\nentryPoints: {entryPoints}\n{emptyValue}"; } + const string originalValueEmpty = "originalValueEmpty"; + public static bool TryRead(string path, [NotNullWhen(true)] out InlinePatch? patch) { patch = null; @@ -133,6 +144,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa string? testName = null; string? framework = null; string? originalValue = null; + var emptyValue = false; string? memberName = null; string[]? entryPoints = null; try @@ -168,6 +180,12 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa continue; } + if (TryValue(lines[index], originalValueEmpty, out var emptyText)) + { + emptyValue = emptyText == "true"; + continue; + } + if (TryValue(lines[index], "memberName", out var memberNameBase64)) { memberName = memberNameBase64.Length == 0 @@ -190,6 +208,13 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa return false; } + // After the lines rather than as they are read, since they come in any order. A value + // that is there wins: the line only says what an empty field cannot + if (emptyValue) + { + originalValue ??= ""; + } + patch = new(sourceFile, lineHint, expression, content, mode) { TestName = testName, diff --git a/src/DiffEngine/Inline/InlinePatcher.cs b/src/DiffEngine/Inline/InlinePatcher.cs index 85af032a1..a1f2c0c03 100644 --- a/src/DiffEngine/Inline/InlinePatcher.cs +++ b/src/DiffEngine/Inline/InlinePatcher.cs @@ -602,7 +602,8 @@ static bool HoldsContent(string source, SourceScan scan, int openParen, string c /// /// Removes the Snapshot call, along with the whitespace and line break that preceded it so no - /// blank line is left behind. + /// blank line is left behind, except over a line holding another Snapshot call, where one has + /// to be (). /// /// What it was called on stays, and that has to still be something once the call has gone. A /// verify call is. A variable is not: settings.Snapshot("old"); became @@ -700,13 +701,87 @@ static PatchStatus TryRemove( // Not when the line above ends in a line comment: pulling the call up would take the // semicolon that follows it into the comment - start = scan.IsCode(lineBreak) ? lineBreak : dotStart; + if (!scan.IsCode(lineBreak)) + { + start = dotStart; + } + else if (SnapshotCallFollows(source, scan, lineStarts, closeParen)) + { + newSource = KeepingItsLine(source, scan, lineBreak, start - 1, dotStart, closeParen); + return PatchStatus.Applied; + } + else + { + start = lineBreak; + } } newSource = Splice(source, start, closeParen + 1, ""); return PatchStatus.Applied; } + /// + /// Whether the line under the one a call ends on holds a Snapshot call: the line that comes + /// up onto the call's own when the call is taken out with its line. + /// + static bool SnapshotCallFollows(string source, SourceScan scan, List lineStarts, int closeParen) + { + var next = LineOf(lineStarts, closeParen) + 1; + return next <= lineStarts.Count && + CallsOnLine(source, scan, lineStarts, next, snapshotName, false).Any(); + } + + /// + /// Takes out a call that started its line, back to the end of the line above, so what + /// followed the call carries on from there, and leaves the line the call was on empty. + /// + /// For a call with a Snapshot call on the line under it, and the empty line is for whoever + /// applies the same Remove next: every framework of a multi-targeted run does, and each case + /// of a test that ignores its parameters. With the lines under it pulled up, the recorded + /// line came to hold that call, and where it had the same literal there was nothing to tell + /// it from the one the patch was made for: the same line, the same anchor. It was taken for + /// a call still to be removed, and a sibling lost its snapshot. The file as it then stood is + /// the file an honest Remove of that sibling would meet, so nothing reading it afterwards + /// can do better, and the line has to be kept. One line, where the call started, however + /// many it ran over: that is the line a patch names, and reads + /// one with no Snapshot call, under a verify statement with none, as that call removed. + /// Anything else on the line under it is read that way already, so nothing is kept for it. + /// + /// + /// The source the call is in. + /// The map of that source. + /// Where the line break in front of the call's line starts. + /// The last character of that line break. + /// The dot the call hangs off. + /// The call's closing paren. + static string KeepingItsLine(string source, SourceScan scan, int lineBreak, int lineBreakEnd, int dot, int closeParen) + { + var restEnd = source.IndexOf('\n', closeParen + 1); + // The line ends inside a literal or a block comment that opened after the call, where a + // line break more would be content. The call goes from where it stands instead, which + // keeps its line by leaving on it what followed. A break that ends a line comment is the + // comment's last character, and is the end of the line all the same + if (restEnd < 0 || + !(scan.IsCode(restEnd) || scan.TryGetCommentEndingAt(restEnd + 1, out _))) + { + return Splice(source, dot, closeParen + 1, ""); + } + + if (restEnd > 0 && + source[restEnd - 1] == '\r') + { + restEnd--; + } + + var builder = new StringBuilder(source.Length); + builder.Append(source, 0, lineBreak); + builder.Append(source, closeParen + 1, restEnd - closeParen - 1); + // The break that was in front of the call, now behind what followed it + builder.Append(source, lineBreak, lineBreakEnd + 1 - lineBreak); + builder.Append(source, restEnd, source.Length - restEnd); + return builder.ToString(); + } + /// /// Whether taking a call out would leave nothing of its expression but what it was called on: /// the call hangs off a name rather than off another call, and nothing is chained on after it. diff --git a/src/DiffEngine/Inline/InlineStaging.cs b/src/DiffEngine/Inline/InlineStaging.cs index 2a30a2f35..a622cb239 100644 --- a/src/DiffEngine/Inline/InlineStaging.cs +++ b/src/DiffEngine/Inline/InlineStaging.cs @@ -452,11 +452,18 @@ static IEnumerable FindStaging(string root, int depth) } /// - /// Through , so two spellings of one path are judged the same way - /// the queue judges them, and on the platforms where that matters. + /// Through , so two spellings of one path are judged the same + /// way the queue judges them, and on the platforms where that matters. + /// + /// Asked of every trio in a staging directory on every clear, so the two answers that need no + /// folded copy are given first: folding changes no path's length, and a path spelled the same + /// is the same. + /// /// static bool SamePath(string left, string right) => - InlineKey.For(left, 0) == InlineKey.For(right, 0); + left.Length == right.Length && + (left == right || + InlineKey.FoldPath(left) == InlineKey.FoldPath(right)); static bool TryPersist(InlinePatch patch, IReadOnlyList origins) { diff --git a/src/DiffEngine/Inline/PendingInline.cs b/src/DiffEngine/Inline/PendingInline.cs index e11c91535..b23684a2a 100644 --- a/src/DiffEngine/Inline/PendingInline.cs +++ b/src/DiffEngine/Inline/PendingInline.cs @@ -67,7 +67,62 @@ public PendingInline(IReadOnlyList variants, string? status = nul internal string ConflictStatus => $"Conflicting snapshots ({OriginsLabel})"; - public string Key => InlineKey.For(Patch.SourceFile, Patch.LineHint); + /// + /// What a queue finds this entry by: of the primary patch's file + /// and line. + /// + /// Built once and kept. Every lookup in a queue asks it of each entry it passes, and building + /// it is a lowercased copy of the path and a formatted string: with hundreds pending, a run + /// that enqueues and settles each of them asked hundreds of thousands of times. Kept beside + /// the file and line it was built from, and built again when the patch no longer holds those, + /// because a patch's properties can be set and with copies whatever is kept here onto an + /// entry that may be given other variants. + /// + /// + public string Key + { + get + { + var patch = Patch; + var built = key.Value; + if (built is null || + built.Line != patch.LineHint || + !ReferenceEquals(built.SourceFile, patch.SourceFile)) + { + built = new(patch.SourceFile, patch.LineHint, InlineKey.For(patch.SourceFile, patch.LineHint)); + // One reference, so a reader on another thread sees a whole key or builds its own + key = new(built); + } + + return built.Key; + } + } + + KeptKey key; + + sealed class BuiltKey(string sourceFile, int line, string key) + { + public string SourceFile => sourceFile; + public int Line => line; + public string Key => key; + } + + /// + /// The kept key, as a field a record can hold without it counting: the equality a record + /// generates compares every field, and two entries are not different for one of them having + /// been asked its key. + /// + readonly struct KeptKey(BuiltKey? value) : + IEquatable + { + public BuiltKey? Value => value; + + public bool Equals(KeptKey other) => true; + + public override bool Equals(object? other) => other is KeptKey; + + public override int GetHashCode() => 0; + } public string Name => $"{Path.GetFileName(Patch.SourceFile)}:{Patch.LineHint}"; } diff --git a/src/DiffEngine/Process/LinuxOsxProcess.cs b/src/DiffEngine/Process/LinuxOsxProcess.cs index 5598d4894..1c6e1f53c 100644 --- a/src/DiffEngine/Process/LinuxOsxProcess.cs +++ b/src/DiffEngine/Process/LinuxOsxProcess.cs @@ -89,7 +89,12 @@ static bool TryRunPs([NotNullWhen(true)] out string? result) { var errorBuilder = new StringBuilder(); var outputBuilder = new StringBuilder(); - const string? arguments = "-o pid,command -x"; + // -ww for a command line of any length. Into a pipe ps already prints one whole, but + // procps takes an exported COLUMNS over that, so a shell with COLUMNS=80 in its + // environment cut every line here to 80 characters and no running tool was matched + // against its command again. Asked for twice, the width is unlimited whatever the + // environment says, to procps and to the BSD ps macOS has + const string? arguments = "-ww -o pid,command -x"; using var process = new Process { StartInfo = new() diff --git a/src/DiffEngine/Protocol/ViewerClient.cs b/src/DiffEngine/Protocol/ViewerClient.cs index 1b604377d..46f317c78 100644 --- a/src/DiffEngine/Protocol/ViewerClient.cs +++ b/src/DiffEngine/Protocol/ViewerClient.cs @@ -191,6 +191,10 @@ internal static void ForgetUnowned() { lastFound.Clear(); reportedForeign.Clear(); + lock (keptGate) + { + DropKept(); + } } /// @@ -283,10 +287,191 @@ public static bool IsOwned(int? port = null) /// settle, a retire, a move or a delete to track - so a port recently found unowned is taken /// at its word rather than connected to again: see . /// + /// + /// And they are the sends there can be thousands of, a settle for every passing inline + /// verification, so they go down one connection where the owner keeps one: see + /// . The first is an ordinary exchange, whose reply says + /// whether the owner does, and so is every one to an owner that predates it. + /// /// public static bool TrySend(ViewerMessage message) => - TrySend(message, out var response, skipIfUnowned: true) && - response.Ok; + Tell(message, Port); + + /// + /// with the port given, for the tests, which use a port + /// of their own rather than change what reads for everything beside them. + /// + internal static bool Tell(ViewerMessage message, int port) + { + switch (SendKept(message, port, out var ok)) + { + case KeptSend.Answered: + return ok; + case KeptSend.Unanswered: + return false; + } + + if (!Exchange(message, out var response, out var keeps, port, null, skipIfUnowned: true)) + { + return false; + } + + if (keeps) + { + KeepConnection(port); + } + + return response.Ok; + } + + enum KeptSend + { + /// + /// No connection is kept to that port, or the one that was has gone, as it does when its + /// owner exits. The ordinary exchange is what finds out who is there now. + /// + NotKept, + + Answered, + + /// + /// Sent, and not answered inside the timeout: an owner that is there and busy, which the + /// ordinary exchange would only wait on for as long again. + /// + Unanswered + } + + sealed class KeptConnection(TcpClient client, int port) : + IDisposable + { + public int Port { get; } = port; + public NetworkStream Stream { get; } = client.GetStream(); + public StreamReader Reader { get; } = new(client.GetStream(), Encoding.UTF8); + + public void Dispose() => + client.Close(); + } + + /// + /// Held for the whole of an exchange on the kept connection, which is one request and its + /// answer at a time. A parallel run's settles take turns at it, each for about as long as + /// the owner takes to answer. + /// + static readonly object keptGate = new(); + + static KeptConnection? kept; + + static KeptSend SendKept(ViewerMessage message, int port, out bool ok) + { + ok = false; + lock (keptGate) + { + if (kept is null) + { + return KeptSend.NotKept; + } + + if (kept.Port != port) + { + DropKept(); + return KeptSend.NotKept; + } + + try + { + var bytes = Encoding.UTF8.GetBytes($"{message.Build()}\n"); + kept.Stream.Write(bytes, 0, bytes.Length); + kept.Stream.Flush(); + var reply = new StringBuilder(); + string? line; + while ((line = kept.Reader.ReadLine()) is { Length: > 0 }) + { + reply.Append(line); + reply.Append('\n'); + } + + if (line is not null && + ViewerResponse.TryParse(reply.ToString(), out var response)) + { + Found(port, true); + ok = response.Ok; + return KeptSend.Answered; + } + + // Closed by the owner, which is an owner that stopped or exited since the last + // send. Whoever holds the port now is for the ordinary exchange to find + DropKept(); + return KeptSend.NotKept; + } + catch (Exception exception) + when (Ignorable(exception)) + { + DropKept(); + return TimedOut(exception) ? KeptSend.Unanswered : KeptSend.NotKept; + } + } + } + + static bool TimedOut(Exception exception) => + exception is IOException + { + InnerException: SocketException + { + SocketErrorCode: SocketError.TimedOut + } + }; + + /// + /// Opens the connection the telling sends after this one go down, to an owner whose reply + /// has just said it keeps one. Failing to is nothing: the next send is an ordinary exchange, + /// and tries again on the strength of its own reply. + /// + static void KeepConnection(int port) + { + lock (keptGate) + { + if (kept is not null) + { + if (kept.Port == port) + { + return; + } + + DropKept(); + } + + var client = new TcpClient(); + try + { + if (!Connect(client, port, ShortTimeout)) + { + client.Close(); + return; + } + + Configure(client, timeout); + // Each request is one small write waiting on one small answer, which is the + // pattern Nagle's algorithm holds back + client.NoDelay = true; + var connection = new KeptConnection(client, port); + var bytes = Encoding.UTF8.GetBytes($"{ViewerServer.Keep}\n"); + connection.Stream.Write(bytes, 0, bytes.Length); + connection.Stream.Flush(); + kept = connection; + } + catch (Exception exception) + when (Ignorable(exception)) + { + client.Close(); + } + } + } + + static void DropKept() + { + kept?.Dispose(); + kept = null; + } /// /// True when a reply arrived and parsed, whatever it says. Callers that need the body, such as @@ -308,9 +493,25 @@ public static bool TrySend( [NotNullWhen(true)] out ViewerResponse? response, int? port = null, TimeSpan? wait = null, - bool skipIfUnowned = false) + bool skipIfUnowned = false) => + Exchange(message, out response, out _, port, wait, skipIfUnowned); + + /// + /// The ordinary exchange: a connection of its own, the request ended by closing the sending + /// half, and the reply read until the owner closes. is whether the + /// reply said its owner would take requests one after another on a connection that stays + /// open, which is . + /// + static bool Exchange( + ViewerMessage message, + [NotNullWhen(true)] out ViewerResponse? response, + out bool keeps, + int? port, + TimeSpan? wait, + bool skipIfUnowned) { response = null; + keeps = false; var endpointPort = port ?? Port; if (skipIfUnowned && RecentlyUnowned(endpointPort)) @@ -347,6 +548,9 @@ public static bool TrySend( var text = reader.ReadToEnd(); if (ViewerResponse.TryParse(text, out response)) { + // A line of its own, and the last: nothing a field holds can look like it, since + // whatever could hold a line break is base64 + keeps = text.EndsWith($"\n{ViewerServer.Keeps}\n", StringComparison.Ordinal); return true; } @@ -404,6 +608,10 @@ public static async Task SendAsync( TimeSpan? wait = null, bool skipIfUnowned = false) { + // Said here, before anything is made. Left to the connect, a send already cancelled was + // a client closed by the abort below before it was ever connected, and on the modern + // frameworks what that threw was read as a port with nobody on it + cancel.ThrowIfCancellationRequested(); var endpointPort = port ?? Port; if (skipIfUnowned && RecentlyUnowned(endpointPort)) @@ -411,10 +619,7 @@ public static async Task SendAsync( return SendOutcome.NoOwner; } - // A send the caller has already cancelled is left to the connect, which is where each - // framework says so in its own way - if (!cancel.IsCancellationRequested && - NothingListening(endpointPort)) + if (NothingListening(endpointPort)) { Found(endpointPort, false); return SendOutcome.NoOwner; @@ -493,6 +698,21 @@ public static async Task SendAsync( exception.GetType().Name); return SendOutcome.NoOwner; } + // The caller cancelling, which reaches the exchange as its socket closing under it, and + // so as whatever a closed socket throws where the call in hand takes no token: all of + // them on .NET Framework, the read before net7. It says nothing about the port. Taken + // for a connect that failed, it was remembered as unowned with the owner still + // listening, and every settle and move after it went unsent until the memory ran out + catch (Exception exception) + when (cancel.IsCancellationRequested && + exception is not OperationCanceledException && + Ignorable(exception)) + { + throw new OperationCanceledException( + $"The send to the inline queue owner on port {endpointPort} was cancelled.", + exception, + cancel); + } // Cancellation is the caller's business; a missing owner is not. catch (Exception exception) when (exception is not OperationCanceledException && Ignorable(exception)) diff --git a/src/DiffEngine/Protocol/ViewerServer.cs b/src/DiffEngine/Protocol/ViewerServer.cs index d8bf3ca3b..14cfc07c2 100644 --- a/src/DiffEngine/Protocol/ViewerServer.cs +++ b/src/DiffEngine/Protocol/ViewerServer.cs @@ -49,12 +49,37 @@ public static bool TryBind(int port, [NotNullWhen(true)] out ViewerServer? serve return true; } + /// + /// How long a failed accept is waited out before the next, once two have failed in a row. + /// See . + /// + internal static readonly TimeSpan FailedAcceptWait = TimeSpan.FromMilliseconds(100); + public async Task Listen(Func handle, Cancel cancel = default) { // Sync dispose: CancellationTokenRegistration is only IAsyncDisposable from net6, and // waiting for an in flight Stop callback buys nothing here. // ReSharper disable once UseAwaitUsing - using var registration = cancel.Register(listener.Stop); + using var registration = cancel.Register(Stop); + await Serve( + Accept, + _ => + { + Interlocked.Increment(ref accepted); + Task.Run(() => Handle(_, handle, cancel), Cancel.None); + }, + cancel) + .ConfigureAwait(false); + } + + /// + /// The accept loop, with the accept and what is done with a connection handed in: a test has + /// no way to make a real listener's accept fail, and how the loop takes a failure is the + /// part of it that has gone wrong. + /// + internal static async Task Serve(Func> accept, Action serve, Cancel cancel) + { + var failedInARow = 0; while (!cancel.IsCancellationRequested) { TcpClient client; @@ -65,7 +90,8 @@ public async Task Listen(Func handle, Cancel canc // waited for the render loop to pump: every connection went unanswered for as long // as that thread was busy, which an accept holding InlineApplier's mutex makes up // to ten seconds. Both awaits, because the first Accept runs on the caller's thread. - client = await Accept(cancel).ConfigureAwait(false); + client = await accept(cancel).ConfigureAwait(false); + failedInARow = 0; } catch (OperationCanceledException) { @@ -92,14 +118,44 @@ public async Task Listen(Func handle, Cancel canc // its connection sits in the backlog surfaces exactly this way - WSAECONNRESET on // Windows, ECONNABORTED on BSD and macOS - and returning gave the queue away for // the life of the process: the socket stays bound, so nobody else can take it, - // and every later client lands in a backlog nothing is draining + // and every later client lands in a backlog nothing is draining. + // + // That one is over as soon as it is reported, so the first failure is retried at + // once. One that fails again is not that. A process out of descriptors is refused + // every accept, at once, with the connection left waiting in the backlog, until + // something is closed: retried straight away that was ten thousand failed accepts + // a second and a whole core, for as long as it lasted. So from the second on there + // is a wait between them + failedInARow++; + if (failedInARow > 1 && + !await Pause(cancel).ConfigureAwait(false)) + { + return; + } + continue; } // Each connection on its own task, so one slow exchange does not stop the next from // being answered. Accepting an inline snapshot legitimately takes seconds, and a // client whose listing goes unanswered for that long concludes the owner has died. - _ = Task.Run(() => Handle(client, handle, cancel), Cancel.None); + serve(client); + } + } + + /// + /// False when the listener was stopped during the wait. + /// + static async Task Pause(Cancel cancel) + { + try + { + await Task.Delay(FailedAcceptWait, cancel).ConfigureAwait(false); + return true; + } + catch (OperationCanceledException) + { + return false; } } @@ -125,7 +181,51 @@ async Task Accept(Cancel cancel) #endif } - static async Task Handle(TcpClient client, Func handle, Cancel cancel) + /// + /// The first line of a connection that is kept: one request after another, each ended by an + /// empty line and answered the same way, until either side closes. + /// + /// The ordinary exchange is a connection each, ended by the client closing its half, and the + /// side that closes first is the side whose port then waits out TIME_WAIT. Windows has about + /// 16,000 ports to give out and keeps each for two minutes, a passing inline verification + /// settles once, and so a large enough green run with a tray answering used up the machine's + /// ports on telling the tray nothing. A client that has many of them to send keeps one + /// connection instead: see . + /// + /// + /// No request starts with this line, since every one starts with its version, so an owner + /// that predates it reads it as an unreadable request and says so. It is never sent to one: + /// a client keeps a connection only to an owner that has just said it . + /// + /// + internal const string Keep = "keep: 1"; + + /// + /// The line an owner that takes ends every ordinary reply with, which is + /// how a client knows to ask. A reader that predates it skips the line, as it skips any name + /// it does not know. + /// + internal const string Keeps = "keeps: 1"; + + /// + /// The kept connections, so that stopping closes them. Nothing else would: each is a read + /// waiting on a client that has nothing to say yet, and before net7 that read takes no token. + /// A client still being answered on one by an owner that had stopped listening would be told + /// about a queue that is no longer the port's. + /// + readonly ConcurrentDictionary kept = new(); + + volatile bool stopped; + + int accepted; + + /// + /// How many connections have been accepted. For the tests and the benchmark, to which a + /// connection kept and a connection each look the same from the answers. + /// + internal int Accepted => Volatile.Read(ref accepted); + + async Task Handle(TcpClient client, Func handle, Cancel cancel) { try { @@ -134,20 +234,22 @@ static async Task Handle(TcpClient client, Func h // ReSharper disable once UseAwaitUsing using var stream = client.GetStream(); using var reader = new StreamReader(stream, Encoding.UTF8); + var first = await ReadLine(reader, cancel); + if (first == Keep) + { + await HandleKept(client, stream, reader, handle, cancel); + return; + } + #if NET7_0_OR_GREATER - var text = await reader.ReadToEndAsync(cancel); + var rest = await reader.ReadToEndAsync(cancel); #else - var text = await reader.ReadToEndAsync(); + var rest = await reader.ReadToEndAsync(); #endif - var response = Respond(handle, text); - var bytes = Encoding.UTF8.GetBytes(response.Build()); -#if NET6_0_OR_GREATER - await stream.WriteAsync(bytes, cancel); -#else - await stream.WriteAsync(bytes, 0, bytes.Length, cancel); -#endif - await stream.FlushAsync(cancel); + // Put back together with the line that was read to tell the two kinds apart + var response = Respond(handle, first is null ? rest : $"{first}\n{rest}"); + await Write(stream, $"{response.Build()}{Keeps}\n", cancel); } } catch (Exception exception) @@ -162,6 +264,73 @@ ObjectDisposedException or } } + /// + /// One request after another on a connection the client keeps. Answered in turn rather than + /// each on a task of its own, which is the order the client sent them in and all it can use: + /// it waits for each answer before it sends the next. + /// + async Task HandleKept( + TcpClient client, + NetworkStream stream, + StreamReader reader, + Func handle, + Cancel cancel) + { + // Each request is one small write waiting on one small answer, which is the pattern + // Nagle's algorithm holds back + client.NoDelay = true; + kept[client] = 0; + try + { + // Checked after it is listed, so that a stop on either side of the listing closes it + while (!stopped) + { + var request = new StringBuilder(); + string? line; + while ((line = await ReadLine(reader, cancel)) is { Length: > 0 }) + { + request.Append(line); + request.Append('\n'); + } + + if (line is null) + { + // The client has gone. Anything it had half sent is nothing to answer + return; + } + + var response = Respond(handle, request.ToString()); + await Write(stream, $"{response.Build()}\n", cancel); + } + } + finally + { + kept.TryRemove(client, out _); + } + } + + // ReSharper disable once ReplaceAsyncWithTaskReturn + static async Task ReadLine(StreamReader reader, Cancel cancel) + { +#if NET7_0_OR_GREATER + return await reader.ReadLineAsync(cancel); +#else + cancel.ThrowIfCancellationRequested(); + return await reader.ReadLineAsync(); +#endif + } + + static async Task Write(NetworkStream stream, string text, Cancel cancel) + { + var bytes = Encoding.UTF8.GetBytes(text); +#if NET6_0_OR_GREATER + await stream.WriteAsync(bytes, cancel); +#else + await stream.WriteAsync(bytes, 0, bytes.Length, cancel); +#endif + await stream.FlushAsync(cancel); + } + static ViewerResponse Respond(Func handle, string text) { if (!ViewerMessage.TryParse(text, out var message)) @@ -181,6 +350,17 @@ static ViewerResponse Respond(Func handle, string } } - public void Dispose() => + void Stop() + { + stopped = true; listener.Stop(); + foreach (var client in kept.Keys) + { + // Unblocks the read it is waiting in, which ends its task + client.Close(); + } + } + + public void Dispose() => + Stop(); } diff --git a/src/DiffEngineTray.Tests/KeyRegisterTests.cs b/src/DiffEngineTray.Tests/KeyRegisterTests.cs new file mode 100644 index 000000000..c0a3debb8 --- /dev/null +++ b/src/DiffEngineTray.Tests/KeyRegisterTests.cs @@ -0,0 +1,68 @@ +using System.Windows.Forms; + +/// +/// A hot key's action runs inside a message filter, and what is thrown from one of those comes out +/// of Application.Run() rather than going to Application.ThreadException: the tray +/// ended, with everything it was tracking, because one press went wrong. +/// +/// The filter is called directly here and never through a message loop, so nothing thrown can +/// reach WinForms' own handling of it. +/// +/// +[NotInParallel] +public class KeyRegisterTests +{ + const int id = 9731; + + // Every modifier and a key no keyboard has, since registering is for the whole desktop for + // as long as the test takes + const KeyModifiers modifiers = KeyModifiers.Control | KeyModifiers.Alt | KeyModifiers.Shift; + + [Test] + public async Task AnActionThatThrowsIsReportedAndTheKeyIsStillHandled() + { + // No awaiting until the register is disposed: a hot key registered with no window belongs + // to the thread that registered it, which is the one that has to take it back + bool bound; + var handled = false; + using (var register = new KeyRegister(IntPtr.Zero)) + { + bound = register.TryAddBinding( + id, + modifiers, + Keys.F24, + () => throw new InvalidOperationException("TheHotKeyFailure")); + if (bound) + { + handled = register.PreFilterMessage(ref HotKeyPressed); + } + } + + await Assert.That(bound).IsTrue(); + await Assert.That(handled).IsTrue(); + await Assert.That(ModuleInitializer.IssuesAsked.Where(_ => _.Contains("TheHotKeyFailure"))).HasSingleItem(); + } + + [Test] + public async Task AnActionRuns() + { + bool bound; + var handled = false; + var ran = false; + using (var register = new KeyRegister(IntPtr.Zero)) + { + bound = register.TryAddBinding(id, modifiers, Keys.F24, () => ran = true); + if (bound) + { + handled = register.PreFilterMessage(ref HotKeyPressed); + } + } + + await Assert.That(bound).IsTrue(); + await Assert.That(handled).IsTrue(); + await Assert.That(ran).IsTrue(); + } + + // WM_HOTKEY, with the binding's id where the message carries it + static Message HotKeyPressed = Message.Create(IntPtr.Zero, 0x0312, id, IntPtr.Zero); +} diff --git a/src/DiffEngineTray.Tests/LinkLauncherTests.cs b/src/DiffEngineTray.Tests/LinkLauncherTests.cs new file mode 100644 index 000000000..ffd43520c --- /dev/null +++ b/src/DiffEngineTray.Tests/LinkLauncherTests.cs @@ -0,0 +1,20 @@ +public class LinkLauncherTests +{ + /// + /// Opening a link can fail - no browser registered, or one that will not start - and it is + /// asked for from a click in the options form and from the handler that reports an error. A + /// throw from either is on the UI thread with nothing to catch it, and from the second it + /// replaces the error being reported. + /// + /// A file that does not exist is the one thing certain to fail to open without anything being + /// opened. + /// + /// + [Test] + public async Task ALinkThatCannotBeOpenedDoesNotThrow() + { + var missing = Path.Combine(Path.GetTempPath(), $"{Guid.NewGuid():N}.missing"); + + await Assert.That(() => LinkLauncher.LaunchUrl(missing)).ThrowsNothing(); + } +} diff --git a/src/DiffEngineTray.Tests/ModuleInitializer.cs b/src/DiffEngineTray.Tests/ModuleInitializer.cs index ad343a374..1363ffbfe 100644 --- a/src/DiffEngineTray.Tests/ModuleInitializer.cs +++ b/src/DiffEngineTray.Tests/ModuleInitializer.cs @@ -3,6 +3,8 @@ public static class ModuleInitializer [ModuleInitializer] public static void Initialize() { + ThrowRatherThanAsk(); + DeclineToOpenAnIssue(); MachineSettings.Ignore(); VerifyWinForms.Initialize(); VerifierSettings.UseSsimForPng(PngSsimThreshold); @@ -10,6 +12,38 @@ public static void Initialize() PointAtAClosedPort(); } + /// + /// An exception thrown inside a window procedure is, by default, caught by WinForms and put + /// to whoever is at the machine in a dialog with Continue and Quit on it. A test that throws + /// there then waits on a click, on the desktop of someone doing something else. Thrown, it + /// fails the test that caused it. + /// + /// For every thread rather than this one, since a test builds its controls on whichever thread + /// it is given, and first here because the mode cannot be changed once a window exists. + /// + /// + static void ThrowRatherThanAsk() => + System.Windows.Forms.Application.SetUnhandledExceptionMode( + System.Windows.Forms.UnhandledExceptionMode.ThrowException, + threadScope: false); + + /// + /// The tray follows an error it did not expect with a modal "Open an issue on GitHub?" box, + /// and a yes opens a browser. A test that reaches one is declined here, with nobody asked, and + /// what it reached is kept for the tests that are about an error being reported. + /// + static void DeclineToOpenAnIssue() => + IssueLauncher.Declined = text => + { + IssuesAsked.Enqueue(text); + return true; + }; + + /// + /// Every question has declined, oldest first. + /// + internal static ConcurrentQueue IssuesAsked { get; } = new(); + /// /// Tests must not write the user environment of the machine running them. The test projects /// run as parallel processes over the one registry key, so a capture in one and a restore in diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 9c049fbb1..57a7ca1ba 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -636,6 +636,12 @@ sealed class FakeTracked : ITrackedFiles public IReadOnlyList Deletes() => DeleteList; + // The lists are replaced when a test changes them, so which lists they are says it + public long Version() => + HashCode.Combine( + System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(MoveList), + System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(DeleteList)); + public bool Has(string key) => MoveList.Any(_ => _.Key == key) || DeleteList.Any(_ => _.Key == key); diff --git a/src/DiffEngineTray.Tests/StubInlineHost.cs b/src/DiffEngineTray.Tests/StubInlineHost.cs index 215281aae..cc14a9e47 100644 --- a/src/DiffEngineTray.Tests/StubInlineHost.cs +++ b/src/DiffEngineTray.Tests/StubInlineHost.cs @@ -12,8 +12,17 @@ class StubInlineHost(params PendingSnapshot[] snapshots) : { public string Description => "stub"; - public IReadOnlyList List() => - snapshots; + /// + /// Run each time the listing is read, which is once as a tracker starts and once a scan after + /// that. Throwing from it is a queue that fails the scan. + /// + public Action? Listed { get; init; } + + public IReadOnlyList List() + { + Listed?.Invoke(); + return snapshots; + } /// /// Whether the queue says what it holds when asked. False stands in for a viewer that holds @@ -87,6 +96,8 @@ public bool AcceptAll(out string? message, out bool refused) public bool DiscardAll(out string? message) { message = null; + DiscardStarted.Set(); + DiscardBlock?.Wait(TimeSpan.FromSeconds(10)); return true; } diff --git a/src/DiffEngineTray.Tests/TrackerClearTest.cs b/src/DiffEngineTray.Tests/TrackerClearTest.cs index b09c7d520..17cd69ee5 100644 --- a/src/DiffEngineTray.Tests/TrackerClearTest.cs +++ b/src/DiffEngineTray.Tests/TrackerClearTest.cs @@ -7,7 +7,7 @@ public async Task Simple() await using var tracker = new RecordingTracker(); tracker.AddDelete(file1); tracker.AddMove(file2, file2, "theExe", "theArguments", true, null); - tracker.Clear(); + await tracker.Clear(); await tracker.AssertEmpty(); } diff --git a/src/DiffEngineTray.Tests/TrackerLockedMoveTest.cs b/src/DiffEngineTray.Tests/TrackerLockedMoveTest.cs index 7ece94cfc..6cf3fa875 100644 --- a/src/DiffEngineTray.Tests/TrackerLockedMoveTest.cs +++ b/src/DiffEngineTray.Tests/TrackerLockedMoveTest.cs @@ -118,7 +118,9 @@ public async Task Ignore_DropsTheKilledProcessFromThePendingMove() var toolProcess = FileLockUtils.StartFileLockProcess(temp2); try { - var tracked = tracker.AddMove(temp1, target1, "theExe", "theArguments", true, toolProcess.Id); + // Named as the tool, since a process running something else is not tracked + var tracked = tracker.AddMove(temp1, target1, toolProcess.MainModule!.FileName, "theArguments", true, toolProcess.Id); + await Assert.That(tracked.Process).IsNotNull(); tracker.Accept(tracked); var pending = tracker.Moves.Single(); diff --git a/src/DiffEngineTray.Tests/TrackerMoveTest.cs b/src/DiffEngineTray.Tests/TrackerMoveTest.cs index f34e1ddde..69e8b4cd7 100644 --- a/src/DiffEngineTray.Tests/TrackerMoveTest.cs +++ b/src/DiffEngineTray.Tests/TrackerMoveTest.cs @@ -27,7 +27,7 @@ public async Task AddSame() tracker.AddMove(file1, file1, "theExe", "theArguments", true, null); using var process = Process.GetCurrentProcess(); var processId = process.Id; - var tracked = tracker.AddMove(file1, file1, "theExe", "theArguments", false, processId); + var tracked = tracker.AddMove(file1, file1, Environment.ProcessPath, "theArguments", false, processId); await Assert.That(tracker.Moves).HasSingleItem(); await Assert.That(tracked.Process!.Id).IsEqualTo(process.Id); await Assert.That(tracker.TrackingAny).IsTrue(); @@ -124,11 +124,16 @@ public async Task OpenDiffToolFromAMenuBuiltBeforeTheMoveWasUpdated() var tool = FileLockUtils.StartFileLockProcess(toolLock); try { - // Nothing by this name exists, so if the launcher gets past the process it starts nothing - var exe = Path.Combine(Path.GetTempPath(), $"ReviewReproNoSuchTool_{Guid.NewGuid()}.exe"); + // Nothing at this path exists, so if the launcher gets past the process it starts + // nothing. Named as the tool is, since a process running something else is not tracked + var exe = Path.Combine( + Path.GetTempPath(), + $"ReviewReproNoSuchTool_{Guid.NewGuid()}", + Path.GetFileName(tool.MainModule!.FileName)); // What the menu captured when it opened var shown = tracker.AddMove(temp, target, exe, "theArguments", false, tool.Id); + await Assert.That(shown.Process).IsNotNull(); // The re-run's move, landing while that menu is open tracker.AddMove(temp, target, exe, "theArguments", false, tool.Id); diff --git a/src/DiffEngineTray.Tests/TrackerProcessImageTest.cs b/src/DiffEngineTray.Tests/TrackerProcessImageTest.cs new file mode 100644 index 000000000..a8f1b9ae2 --- /dev/null +++ b/src/DiffEngineTray.Tests/TrackerProcessImageTest.cs @@ -0,0 +1,136 @@ +/// +/// A move's process id is a claim by another process about which one is the diff tool, and a +/// library from before ProcessCleanup.StillRunning can send the id of a tool closed since +/// its test run began, which Windows has handed to something else. The tray holds whatever has +/// the id and, on accept, ends it. So the id is believed only when the process holding it runs +/// the executable the move names. +/// +/// Every process here is started by the test, and stands in both for a diff tool and for the +/// stranger that got a diff tool's id. +/// +/// +[NotInParallel] +public class TrackerProcessImageTest : + IDisposable +{ + [Test] + public async Task AProcessRunningSomethingElseIsNotTrackedAndNotEnded() + { + await using var tracker = new RecordingTracker(); + + var tracked = tracker.AddMove(temp, target, @"C:\Tools\TheDiffTool\TheDiffTool.exe", "theArguments", true, stranger.Id); + + await Assert.That(tracked.Process).IsNull(); + await Assert.That(tracked.IsOpen).IsFalse(); + + tracker.Accept(tracked); + + await Assert.That(tracker.Moves).IsEmpty(); + await Assert.That(stranger.WaitForExit(1000)).IsFalse(); + } + + [Test] + public async Task AProcessRunningTheToolIsTrackedAndEnded() + { + await using var tracker = new RecordingTracker(); + + var tracked = tracker.AddMove(temp, target, Image, "theArguments", true, stranger.Id); + + await Assert.That(tracked.Process!.Id).IsEqualTo(stranger.Id); + + tracker.Accept(tracked); + + await Assert.That(stranger.WaitForExit(5000)).IsTrue(); + } + + /// + /// By file name. One executable has several paths - a junction, a substituted drive, a short + /// name, another case - and which of them the sender resolved is not which the system reports, + /// so a comparison of whole paths would stop closing tools it should close. + /// + [Test] + public async Task TheToolUnderAnotherSpellingOfItsPathIsTracked() + { + await using var tracker = new RecordingTracker(); + var spelled = Path.Combine(@"X:\elsewhere", Path.GetFileName(Image).ToUpperInvariant()); + + var tracked = tracker.AddMove(temp, target, spelled, "theArguments", true, stranger.Id); + + await Assert.That(tracked.Process!.Id).IsEqualTo(stranger.Id); + } + + /// + /// A tool started through a script runs under the command interpreter, so the id is that of + /// cmd.exe and not of anything named by the move. It is left alone: ending the interpreter + /// closes no diff window, and believing any cmd.exe to be the tool is how somebody's shell + /// would be ended. + /// + [Test] + public async Task AToolStartedThroughAScriptIsNotTracked() + { + await using var tracker = new RecordingTracker(); + var script = Path.Combine(Path.GetDirectoryName(Image)!, Path.ChangeExtension(Path.GetFileName(Image), ".cmd")); + + var tracked = tracker.AddMove(temp, target, script, "theArguments", true, stranger.Id); + + await Assert.That(tracked.Process).IsNull(); + } + + [Test] + public async Task AReRunNamingAProcessRunningSomethingElseLeavesTheMoveWithNone() + { + await using var tracker = new RecordingTracker(); + tracker.AddMove(temp, target, Image, "theArguments", true, stranger.Id); + + var tracked = tracker.AddMove(temp, target, @"C:\Tools\TheDiffTool\TheDiffTool.exe", "theArguments", true, stranger.Id); + + await Assert.That(tracked.Process).IsNull(); + tracker.Accept(tracked); + await Assert.That(stranger.WaitForExit(1000)).IsFalse(); + } + + [Test] + public async Task AMoveNamingNoToolIsNotGivenAProcess() + { + await using var tracker = new RecordingTracker(); + + var tracked = tracker.AddMove(temp, target, null, null, true, stranger.Id); + + await Assert.That(tracked.Process).IsNull(); + } + + string Image => stranger.MainModule!.FileName; + + public TrackerProcessImageTest() + { + directory = Path.Combine(Path.GetTempPath(), "DiffEngineTray.Tests", Guid.NewGuid().ToString("N")); + var received = Path.Combine(directory, "received"); + Directory.CreateDirectory(received); + // An extension no diff tool is registered for, so a move naming no tool resolves none + temp = Path.Combine(received, "file.trackerprocessimage"); + target = Path.Combine(directory, "file.trackerprocessimage"); + File.WriteAllText(temp, "received"); + File.WriteAllText(target, "verified"); + var locked = Path.Combine(directory, "locked.txt"); + File.WriteAllText(locked, ""); + stranger = FileLockUtils.StartFileLockProcess(locked); + } + + public void Dispose() + { + FileLockUtils.Cleanup(stranger); + try + { + Directory.Delete(directory, true); + } + catch (IOException) + { + // The process that held a file in it is still on its way out + } + } + + readonly string directory; + readonly string temp; + readonly string target; + readonly Process stranger; +} diff --git a/src/DiffEngineTray.Tests/TrackerProcessReleaseTest.cs b/src/DiffEngineTray.Tests/TrackerProcessReleaseTest.cs new file mode 100644 index 000000000..93d5417d4 --- /dev/null +++ b/src/DiffEngineTray.Tests/TrackerProcessReleaseTest.cs @@ -0,0 +1,135 @@ +/// +/// A move that cannot be killed still has a process: DiffRunner sends the id for an MDI tool too, +/// and holds a handle on it. Nothing let go of +/// that handle when the move left, since the only place a tracked process was disposed was the +/// kill these moves are passed over for. One handle a tracked move, until a finaliser ran. +/// +/// Every process here is one the test started, and none of them may be ended by the tray: the +/// move says it cannot be killed. +/// +/// +[NotInParallel] +public class TrackerProcessReleaseTest : + IDisposable +{ + [Test] + public async Task AnAcceptedMoveLetsGoOfItsProcess() + { + await using var tracker = new RecordingTracker(); + var tracked = Track(tracker); + var held = tracked.Process!; + + tracker.Accept(tracked); + + await AssertReleased(tracked, held); + } + + [Test] + public async Task ADiscardedMoveLetsGoOfItsProcess() + { + await using var tracker = new RecordingTracker(); + var tracked = Track(tracker); + var held = tracked.Process!; + + tracker.Discard(tracked); + + await AssertReleased(tracked, held); + } + + [Test] + public async Task ASettledMoveLetsGoOfItsProcess() + { + await using var tracker = new RecordingTracker(); + var tracked = Track(tracker); + var held = tracked.Process!; + + await Assert.That(((ITrackedFiles) tracker).Untrack(TrackedKeys.ForMove(temp))).IsTrue(); + + await AssertReleased(tracked, held); + } + + [Test] + public async Task AMoveStillTrackedWhenTheTrayExitsLetsGoOfItsProcess() + { + var tracker = new RecordingTracker(); + var tracked = Track(tracker); + var held = tracked.Process!; + + await tracker.DisposeAsync(); + + await AssertReleased(tracked, held); + } + + /// + /// A move kept pending keeps its process, which is what "Accept all open" and "Open diff tool" + /// read to find the window. + /// + [Test] + public async Task AMoveKeptPendingKeepsItsProcess() + { + await using var tracker = new RecordingTracker(); + var tracked = Track(tracker); + // Nowhere to move the file to, so the accept is refused and the move stays + Directory.Delete(Path.GetDirectoryName(target)!, true); + + tracker.Accept(tracked); + + await Assert.That(tracker.Moves).HasSingleItem(); + await Assert.That(tracked.Process).IsNotNull(); + await Assert.That(tracked.Process!.HasExited).IsFalse(); + } + + TrackedMove Track(Tracker tracker) + { + var tracked = tracker.AddMove(temp, target, tool.MainModule!.FileName, "theArguments", false, tool.Id); + if (tracked.Process is null) + { + throw new("The process was not tracked."); + } + + return tracked; + } + + async Task AssertReleased(TrackedMove tracked, Process held) + { + await Assert.That(tracked.Process).IsNull(); + // What a disposed Process says of anything asked about the process it held + await Assert.That(() => held.HasExited).Throws(); + // Let go of, and not ended + await Assert.That(tool.HasExited).IsFalse(); + } + + public TrackerProcessReleaseTest() + { + directory = Path.Combine(Path.GetTempPath(), "DiffEngineTray.Tests", Guid.NewGuid().ToString("N")); + var received = Path.Combine(directory, "received"); + var verified = Path.Combine(directory, "verified"); + Directory.CreateDirectory(received); + Directory.CreateDirectory(verified); + temp = Path.Combine(received, "file.txt"); + target = Path.Combine(verified, "file.txt"); + File.WriteAllText(temp, "received"); + File.WriteAllText(target, "verified"); + var locked = Path.Combine(directory, "locked.txt"); + File.WriteAllText(locked, ""); + tool = FileLockUtils.StartFileLockProcess(locked); + } + + public void Dispose() + { + FileLockUtils.Cleanup(tool); + try + { + Directory.Delete(directory, true); + } + catch (IOException) + { + // The process that held a file in it is still on its way out + } + } + + readonly string directory; + readonly string temp; + readonly string target; + readonly Process tool; +} diff --git a/src/DiffEngineTray.Tests/TrackerScanTest.cs b/src/DiffEngineTray.Tests/TrackerScanTest.cs new file mode 100644 index 000000000..fdf8fb86b --- /dev/null +++ b/src/DiffEngineTray.Tests/TrackerScanTest.cs @@ -0,0 +1,127 @@ +/// +/// The two second scan, which drops a move whose received file has gone or has come to equal its +/// target, and ends that move's diff tool. +/// +[NotInParallel] +public class TrackerScanTest : + IDisposable +{ + /// + /// The scan decides about the move it read and then compares two files, which takes as long + /// as they are large. A re-run that lands in between replaces the move, and the removal was by + /// key alone: it took the fresh move, which nothing had found equal to anything, and ended the + /// tool just opened for it. + /// + [Test] + public async Task AMoveReplacedWhileItWasBeingComparedIsKept() + { + await using var tracker = new RecordingTracker(); + File.WriteAllText(temp, "same"); + File.WriteAllText(target, "same"); + File.WriteAllText(other, "different"); + // What the scan read, and then what a re-run made of it before the scan had finished + var scanned = tracker.AddMove(temp, target, "theExe", "theArguments", true, null); + var fresh = tracker.AddMove(temp, other, "theExe", "theArguments", true, null); + + await tracker.HandleScanMove(new(temp, scanned)); + + await Assert.That(tracker.FindMove(temp)).IsSameReferenceAs(fresh); + } + + [Test] + public async Task AMoveThatCameToEqualItsTargetIsDropped() + { + await using var tracker = new RecordingTracker(); + File.WriteAllText(temp, "same"); + File.WriteAllText(target, "same"); + var scanned = tracker.AddMove(temp, target, "theExe", "theArguments", true, null); + + await tracker.HandleScanMove(new(temp, scanned)); + + await Assert.That(tracker.Moves).IsEmpty(); + } + + /// + /// A file that may not be read is no more a failure of the scan than one that is locked: the + /// pair cannot be compared this round. Only the locked one was caught, so this one failed the + /// whole scan, every two seconds, for as long as the move stayed pending. + /// + [Test] + public async Task ATargetThatMayNotBeReadIsPassedOver() + { + await using var tracker = new RecordingTracker(); + File.WriteAllText(temp, "same"); + File.WriteAllText(target, "same"); + var scanned = tracker.AddMove(temp, target, "theExe", "theArguments", true, null); + + var info = new FileInfo(target); + var security = info.GetAccessControl(); + var rule = new FileSystemAccessRule( + WindowsIdentity.GetCurrent().User!, + FileSystemRights.ReadData, + AccessControlType.Deny); + security.AddAccessRule(rule); + info.SetAccessControl(security); + try + { + await tracker.HandleScanMove(new(temp, scanned)); + + await Assert.That(tracker.FindMove(temp)).IsSameReferenceAs(scanned); + } + finally + { + security.RemoveAccessRule(rule); + info.SetAccessControl(security); + } + } + + /// + /// A scan that fails for a reason nobody foresaw is logged and the next one runs. It used to + /// ask whether to open an issue, in a modal box put up from the timer's thread, and no scan + /// ran again until somebody answered it. + /// + [Test] + public async Task AScanThatFailsIsFollowedByTheNextWithNobodyAsked() + { + var listed = 0; + var host = new StubInlineHost + { + Listed = () => + { + // The first is the tracker starting, and the rest are scans + if (Interlocked.Increment(ref listed) > 1) + { + throw new InvalidOperationException("TheScanFailure"); + } + } + }; + await using var tracker = new RecordingTracker(inline: host); + + var timeout = Stopwatch.StartNew(); + while (Volatile.Read(ref listed) < 3 && + timeout.Elapsed < TimeSpan.FromSeconds(30)) + { + await Task.Delay(100); + } + + await Assert.That(Volatile.Read(ref listed)).IsGreaterThanOrEqualTo(3); + await Assert.That(ModuleInitializer.IssuesAsked.Where(_ => _.Contains("Failed to scan files"))).IsEmpty(); + } + + public TrackerScanTest() + { + directory = Path.Combine(Path.GetTempPath(), "DiffEngineTray.Tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(directory); + temp = Path.Combine(directory, "file.received.txt"); + target = Path.Combine(directory, "file.verified.txt"); + other = Path.Combine(directory, "other.verified.txt"); + } + + public void Dispose() => + Directory.Delete(directory, true); + + readonly string directory; + readonly string temp; + readonly string target; + readonly string other; +} diff --git a/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs b/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs index cf17226b0..b7aae0203 100644 --- a/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs +++ b/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs @@ -97,6 +97,29 @@ public async Task DiscardDoesNotWaitOnTheQueue() await discarding; } + /// + /// And so does the click on "Discard (n)", which was left asking the queue from the thread the + /// click came in on when the single discard was moved off it: up to fifteen seconds of a tray + /// that draws nothing, when a viewer holds the queue and is slow to answer. + /// + [Test] + public async Task ClearDoesNotWaitOnTheQueue() + { + using var block = new ManualResetEventSlim(); + var host = new StubInlineHost(new PendingSnapshot("c:\\repo\\sample.cs|12", "Sample.cs:12", null)) + { + DiscardBlock = block + }; + await using var tracker = new RecordingTracker(inline: host); + + var clearing = tracker.Clear(); + + await Assert.That(host.DiscardStarted.Wait(TimeSpan.FromSeconds(5))).IsTrue(); + await Assert.That(clearing.IsCompleted).IsFalse(); + block.Set(); + await clearing; + } + [Test] public async Task AcceptAllForwardsOnce() { @@ -336,7 +359,7 @@ public async Task ClearDiscardsSnapshots() using var viewer = new FakeViewer("Sample.cs:1"); await using var tracker = new RecordingTracker(); - tracker.Clear(); + await tracker.Clear(); await Assert.That(viewer.Verbs).Contains("discardall"); await Assert.That(viewer.Queue).IsEmpty(); diff --git a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs index d665f1a06..cc1280ef5 100644 --- a/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs +++ b/src/DiffEngineTray.Tests/TrackerTrackedFilesTest.cs @@ -291,12 +291,14 @@ public async Task AMoveArrivingAgainOverTheViewerPortKeepsAnotherToolAndItsProce var tool = FileLockUtils.StartFileLockProcess(file); try { - tracker.AddMove(temp, target, "theExe", "theArguments", true, tool.Id); + // Named as the tool, since a process running something else is not tracked + var exe = tool.MainModule!.FileName; + tracker.AddMove(temp, target, exe, "theArguments", true, tool.Id); ((ITrackedFiles) tracker).AddMove(temp, target); var move = tracker.Moves.Single(); - await Assert.That(move.Exe).IsEqualTo("theExe"); + await Assert.That(move.Exe).IsEqualTo(exe); await Assert.That(move.Arguments).IsEqualTo("theArguments"); await Assert.That(move.CanKill).IsTrue(); await Assert.That(move.IsViewer).IsFalse(); diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 71775da77..3a72e6de5 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -107,7 +107,7 @@ public async Task TrayDiscardAllEmptiesTheAttachedViewer() var move = pair.AddMove(); pair.Pump(); - pair.Tracker.Clear(); + await pair.Tracker.Clear(); var viewer = pair.Pump(); await Assert.That(viewer.Queue).IsEmpty(); @@ -390,6 +390,66 @@ public async Task ATrackedFileOrAStashedFocusIsNeverAnsweredUnchanged() await Assert.That(pair.Send(new(ViewerVerb.ListFull, Body: focused.Tag)).Unchanged).IsTrue(); } + /// + /// What the tag says of the tracked files has to move with everything a listing carries of + /// them, and a move keeps its key through a change of target: counting them, or going by + /// their keys, would answer this one unchanged. + /// + [Test] + public async Task ATrackedMoveThatChangedOrLeftIsNeverAnsweredUnchanged() + { + await using var pair = new TrayOwned(); + var move = pair.AddMove(); + var tag = pair.Send(new(ViewerVerb.ListFull)).Tag; + await Assert.That(pair.Send(new(ViewerVerb.ListFull, Body: tag)).Unchanged).IsTrue(); + + var elsewhere = move.Target + ".elsewhere"; + pair.Tracker.AddMove(move.Temp, elsewhere, null, null, false, null); + var retargeted = pair.Send(new(ViewerVerb.ListFull, Body: tag)); + await Assert.That(retargeted.Unchanged).IsFalse(); + await Assert.That(retargeted.Moves.Single().Target).IsEqualTo(elsewhere); + await Assert.That(pair.Send(new(ViewerVerb.ListFull, Body: retargeted.Tag)).Unchanged).IsTrue(); + + pair.Tracker.Discard(pair.Tracker.Moves.Single()); + var gone = pair.Send(new(ViewerVerb.ListFull, Body: retargeted.Tag)); + await Assert.That(gone.Unchanged).IsFalse(); + await Assert.That(gone.Moves).IsEmpty(); + } + + /// + /// An attached viewer asks whether the listing changed five times a second, and the answer + /// used to be made by describing every tracked move and delete and hashing the lot: half a + /// megabyte of garbage a poll for a few hundred pending files, to say that nothing happened. + /// + [Test] + public async Task AskingWhetherTheListingChangedDoesNotDescribeEveryTrackedFile() + { + await using var pair = new TrayOwned(); + for (var i = 0; i < 100; i++) + { + pair.AddMove(); + pair.AddDelete(); + } + + IQueueOwner owner = pair.Host; + var tag = owner.ListingTag(); + + // No awaiting until the count is read, since the count is this thread's + const int asks = 100; + var before = GC.GetAllocatedBytesForCurrentThread(); + var unchanged = true; + for (var i = 0; i < asks; i++) + { + unchanged &= owner.ListingTag() == tag; + } + + var each = (GC.GetAllocatedBytesForCurrentThread() - before) / asks; + + await Assert.That(unchanged).IsTrue(); + // The tag's own string and two enumerators, with room to spare + await Assert.That(each).IsLessThan(2000); + } + [Test] public async Task ViewerDiscardOfOneSnapshotReachesTheTray() { @@ -626,7 +686,7 @@ public async Task TrayDiscardAllEmptiesTheOwningViewer() pair.Queue(sample, 1); pair.Queue(other, 7); - pair.Tracker.Clear(); + await pair.Tracker.Clear(); await Assert.That(pair.Viewer.Queue).IsEmpty(); await Assert.That(pair.Listing).IsEmpty(); diff --git a/src/DiffEngineTray/HotKey/KeyRegister.cs b/src/DiffEngineTray/HotKey/KeyRegister.cs index 553e3236f..39cc80720 100644 --- a/src/DiffEngineTray/HotKey/KeyRegister.cs +++ b/src/DiffEngineTray/HotKey/KeyRegister.cs @@ -85,7 +85,17 @@ public bool PreFilterMessage(ref Message message) return false; } - action(); + // Caught here because nothing further out will. What a message filter throws does not go + // to Application.ThreadException, as a throw from a click does: it comes out of + // Application.Run(), and the tray ends with everything it was tracking. + try + { + action(); + } + catch (Exception exception) + { + ExceptionHandler.Handle("Failed to run a hot key", exception); + } // true to filter message and stop it from being dispatched return true; diff --git a/src/DiffEngineTray/ITrackedFiles.cs b/src/DiffEngineTray/ITrackedFiles.cs index 792280c91..cfba3d35c 100644 --- a/src/DiffEngineTray/ITrackedFiles.cs +++ b/src/DiffEngineTray/ITrackedFiles.cs @@ -15,6 +15,15 @@ interface ITrackedFiles IReadOnlyList Deletes(); + /// + /// A number that is another one whenever or would + /// list anything differently from the last time it was asked for, and cheap to ask for when + /// they would not: the owner asks on every poll of an attached viewer, to tell it nothing + /// changed without listing anything. It may also move when nothing listed did, which costs one + /// listing. + /// + long Version(); + bool Has(string key); /// diff --git a/src/DiffEngineTray/IssueLauncher.cs b/src/DiffEngineTray/IssueLauncher.cs index 69ca628ac..0fa389e5c 100644 --- a/src/DiffEngineTray/IssueLauncher.cs +++ b/src/DiffEngineTray/IssueLauncher.cs @@ -37,7 +37,7 @@ public static void LaunchForException(string message, Exception exception) Open an issue on GitHub? """; - if (AskIfOpenIssue(text)) + if (Declined(text)) { return; } @@ -68,7 +68,7 @@ public static void LaunchForException(string message) Open an issue on GitHub? """; - if (AskIfOpenIssue(text)) + if (Declined(text)) { return; } @@ -81,6 +81,13 @@ Open an issue on GitHub? LinkLauncher.LaunchUrl(BuildUrl(message, extraBody)); } + /// + /// Puts the question, and answers whether it was declined. A field so that a test process can + /// decline without anybody being asked: the box is modal, on the desktop of whoever is at the + /// machine, and a test run that reached it waited there for a click. + /// + internal static Func Declined = AskIfOpenIssue; + static bool AskIfOpenIssue(string text) { var result = MessageBox.Show( diff --git a/src/DiffEngineTray/LinkLauncher.cs b/src/DiffEngineTray/LinkLauncher.cs index 0b11ecef9..2695d1eb5 100644 --- a/src/DiffEngineTray/LinkLauncher.cs +++ b/src/DiffEngineTray/LinkLauncher.cs @@ -1,5 +1,11 @@ static class LinkLauncher { + /// + /// Logged when it cannot be opened, and nothing more: no browser registered, or one that will + /// not start, is nothing the tray can do anything about. Thrown, it went onto the UI thread + /// from a click in the options form, and out of the handler that reports an error, in place + /// of the error being reported. + /// public static void LaunchUrl(string url) { var startInfo = new ProcessStartInfo @@ -7,6 +13,13 @@ public static void LaunchUrl(string url) UseShellExecute = true, FileName = url }; - using var process = Process.Start(startInfo); + try + { + using var process = Process.Start(startInfo); + } + catch (Exception exception) + { + Log.Error(exception, "Failed to open {Url}", url); + } } -} \ No newline at end of file +} diff --git a/src/DiffEngineTray/MenuBuilder.cs b/src/DiffEngineTray/MenuBuilder.cs index a23203e4f..5ccf82d82 100644 --- a/src/DiffEngineTray/MenuBuilder.cs +++ b/src/DiffEngineTray/MenuBuilder.cs @@ -99,7 +99,7 @@ static IEnumerable BuildTrackingMenuItems(Tracker tracker) yield return new ToolStripSeparator(); } - yield return new MenuButton($"Discard ({count})", tracker.Clear, Images.Discard); + yield return new MenuButton($"Discard ({count})", () => tracker.Clear(),Images.Discard); yield return new MenuButton($"Accept all ({count})", () => tracker.AcceptAll(), Images.AcceptAll); } diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index a33d8b62c..68726e27c 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -332,10 +332,11 @@ ViewerResponse IQueueOwner.Listing(bool withPatches) string IQueueOwner.ListingTag() { - // The tracked files by what the listing would carry of them rather than by a count, since - // the tracker changes them on its own scan as well as through here. A handful of paths, - // where the listing it saves is every patch. - var files = TrackedFiles is { } tracked ? Fingerprint(tracked) : ""; + // The tracked files by the tracker's own word on whether what it would list has changed, + // rather than by a count of them, since it changes them on its own scan as well as through + // here. Asked of it rather than worked out from the lists, which was every tracked file + // described and hashed on every poll to find that none had changed. + var files = TrackedFiles?.Version(); lock (gate) { if (!ReferenceEquals(queue, taggedQueue)) @@ -348,22 +349,6 @@ string IQueueOwner.ListingTag() } } - static string Fingerprint(ITrackedFiles tracked) - { - var builder = new StringBuilder(); - foreach (var move in tracked.Moves()) - { - builder.Append(move).Append('\n'); - } - - foreach (var delete in tracked.Deletes()) - { - builder.Append(delete).Append('\n'); - } - - return Convert.ToHexString(System.Security.Cryptography.SHA256.HashData(Encoding.UTF8.GetBytes(builder.ToString()))); - } - bool IQueueOwner.Has(string key) { if (TrackedKeys.IsTracked(key)) diff --git a/src/DiffEngineTray/ProcessEx.cs b/src/DiffEngineTray/ProcessEx.cs index 6ca118e24..19a8ebe5b 100644 --- a/src/DiffEngineTray/ProcessEx.cs +++ b/src/DiffEngineTray/ProcessEx.cs @@ -50,6 +50,96 @@ public static bool TryGet(int id, [NotNullWhen(true)] out Process? process) } } + [DllImport("kernel32.dll", EntryPoint = "QueryFullProcessImageNameW", ExactSpelling = true, CharSet = CharSet.Unicode, SetLastError = true)] + static extern bool QueryFullProcessImageName(SafeProcessHandle process, int flags, [Out] char[] name, ref int size); + + /// + /// The process a move names, when it is running the tool the move names. + /// + /// The id arrives from another process and is a claim. A library from before + /// ProcessCleanup.StillRunning lists the diff tools once, as its test run begins, and + /// sends the id of one closed since then, which Windows may have handed to anything. Held and + /// tracked on the id alone, that stranger was what an accept ended. + /// + /// + /// A process that is not known to be the tool is tracked as no process, so it is not ended + /// and the pair does not count as open. That is every case the comparison cannot settle, each + /// on purpose: + /// + /// + /// A move naming no tool. There is nothing to compare with, and no library sends an id + /// without one. + /// A tool started through a script (code.cmd, rider.cmd). The id is the + /// command interpreter's. Ending that closes no diff window, the script having handed over to + /// the real program, and taking any cmd.exe for the tool is how somebody's shell would be + /// ended. + /// An image that cannot be read. One this account cannot open was never held by + /// either, and one that has exited has nothing left to end. + /// + /// + /// A launcher that is itself an executable - a shim that starts the real tool and waits - is + /// the image the sender started and the id it sent, so it matches as it always did. + /// + /// + public static bool TryGetTool(int id, string? exe, [NotNullWhen(true)] out Process? process) + { + if (!TryGet(id, out process)) + { + return false; + } + + var image = ImagePath(process); + if (IsSameExecutable(image, exe)) + { + return true; + } + + Log.Warning( + "Process {Id} is not tracked as the diff tool: it is running `{Image}` and the move names `{Exe}`", + id, + image ?? "an image that could not be read", + exe ?? "no tool"); + process.Dispose(); + process = null; + return false; + } + + /// + /// By file name and not by path. One executable has several paths - through a junction, a + /// substituted drive, a short name, another case - and the one the sender resolved need not be + /// the one the system reports, so comparing whole paths would stop closing tools that should + /// be closed. What that gives up is telling one copy of a tool from another copy of it. + /// + internal static bool IsSameExecutable(string? image, string? exe) => + image is not null && + exe is not null && + string.Equals(Path.GetFileName(image), Path.GetFileName(exe), StringComparison.OrdinalIgnoreCase); + + /// + /// Asked of the handle that is held, so it is the process that would be ended that answers, + /// and through the call that needs the least access to it. + /// + static string? ImagePath(Process process) + { + try + { + // The longest a path can be + var name = new char[32768]; + var size = name.Length; + if (QueryFullProcessImageName(process.SafeHandle, 0, name, ref size)) + { + return new(name, 0, size); + } + } + catch (Exception exception) + when (exception is Win32Exception or InvalidOperationException) + { + // Exited, or no longer this account's to ask + } + + return null; + } + public static void KillAndDispose(this Process process) { // Capture identity up front. Once the process has exited, Id/MainModule can throw, diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index 90be35e43..5043ce50b 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -190,7 +190,7 @@ internal static IEnumerable BuildKeyBindings(Settings settings, Trac { if (settings.DiscardAllHotKey is { } discardAll) { - yield return new(KeyBindingIds.DiscardAll, discardAll, tracker.Clear); + yield return new(KeyBindingIds.DiscardAll, discardAll, () => tracker.Clear()); } if (settings.AcceptAllHotKey is { } acceptAll) diff --git a/src/DiffEngineTray/Settings/OptionsFormLauncher.cs b/src/DiffEngineTray/Settings/OptionsFormLauncher.cs index 7bdaff9ce..3e58e2790 100644 --- a/src/DiffEngineTray/Settings/OptionsFormLauncher.cs +++ b/src/DiffEngineTray/Settings/OptionsFormLauncher.cs @@ -75,7 +75,7 @@ internal static List ReBind(KeyRegister keyRegister, Tracker tracker, Se static void Bind(KeyRegister keyRegister, Tracker tracker, Settings settings, List saveErrors) { AddHotKey(keyRegister, settings.AcceptAllHotKey, KeyBindingIds.AcceptAll, () => tracker.AcceptAll(), saveErrors); - AddHotKey(keyRegister, settings.DiscardAllHotKey, KeyBindingIds.DiscardAll, tracker.Clear, saveErrors); + AddHotKey(keyRegister, settings.DiscardAllHotKey, KeyBindingIds.DiscardAll, () => tracker.Clear(), saveErrors); AddHotKey(keyRegister, settings.AcceptOpenHotKey, KeyBindingIds.AcceptOpen, () => tracker.AcceptOpen(), saveErrors); } diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 6328d0555..814841bf5 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -28,10 +28,10 @@ public Tracker(Action active, Action inactive, LockedFilesResolver? lockedFilesR timer = new( ScanFiles, TimeSpan.FromSeconds(2), - exception => - { - ExceptionHandler.Handle("Failed to scan files", exception); - }); + // Logged and no more. This is the timer's own thread, and the handler everything else + // uses follows the log with a modal box: no scan ran again until somebody answered it, + // and a scan is nothing anybody asked for, so the box arrived out of nowhere + exception => Log.Error(exception, "Failed to scan files")); // Seeded rather than left empty until the first scan two seconds later. The menu reads // this cache now, so without it a tray that has just started shows none of what a viewer @@ -61,20 +61,25 @@ Task ScanFiles(Cancel cancel) return Task.WhenAll(moves.Select(HandleScanMove)); } - async Task HandleScanMove(KeyValuePair pair) + internal async Task HandleScanMove(KeyValuePair pair) { - void RemoveAndKill(TrackedMove tacked) + // The move this scan looked at, and no other. Everything below is about that one, and a + // re-run can replace it while the two files are being compared: taken out by key alone, + // the move that went was the fresh one, which nothing had found equal to anything, and the + // tool just opened for it was ended. A move that was replaced is left to the next scan. + void RemoveAndKill() { - if (moves.TryRemove(tacked.Temp, out var removed)) + if (moves.TryRemove(pair)) { - KillProcesses(removed); + KillProcesses(pair.Value); + Release(pair.Value); } } var move = pair.Value; if (!File.Exists(move.Temp)) { - RemoveAndKill(pair.Value); + RemoveAndKill(); return; } @@ -106,14 +111,15 @@ void RemoveAndKill(TrackedMove tacked) return; } } - catch (IOException) + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) { - // File is missing, or locked by a diff tool or a running test. - // Skip this scan round + // File is missing, locked by a diff tool or a running test, or not this account's to + // read. Skip this scan round return; } - RemoveAndKill(pair.Value); + RemoveAndKill(); } readonly ConcurrentDictionary differing = new(StringComparer.OrdinalIgnoreCase); @@ -190,7 +196,7 @@ public TrackedMove AddMove( Process? process = null; if (processId != null) { - ProcessEx.TryGet(processId.Value, out process); + ProcessEx.TryGetTool(processId.Value, exe, out process); } var move = BuildTrackedMove(temp, exe, arguments, canKill, target, process); @@ -219,7 +225,9 @@ public TrackedMove AddMove( // a menu built before this update - reaches a disposed process through it existing.Process?.Dispose(); existing.Process = null; - ProcessEx.TryGet(processId.Value, out process); + // Against the tool the pair was tracked with when this move names none, as + // Retarget keeps that one + ProcessEx.TryGetTool(processId.Value, exe ?? existing.Exe, out process); } var move = exe == null @@ -659,13 +667,44 @@ void AcceptMove(TrackedMove move, AcceptBatch batch) return; } - if (!InnerMove(removed, batch)) + if (InnerMove(removed, batch)) + { + Release(removed); + return; + } + + // Keep the move pending so accepting can be retried + Restore(removed); + } + + /// + /// Puts back a move that was taken out to be accepted and could not be. One that arrived for + /// the same received file meanwhile stands, and this one has then left for good. + /// + void Restore(TrackedMove removed) + { + if (!moves.TryAdd(removed.Temp, removed)) { - // Keep the move pending so accepting can be retried - moves.TryAdd(removed.Temp, removed); + Release(removed); } } + /// + /// Lets go of the process a move was tracked with, without ending it, once the move has left + /// for good. + /// + /// holds a handle on every process a move + /// names, and DiffRunner names one for an MDI tool too. Only + /// disposed any, and it passes over a move that cannot be killed, so each of those kept a + /// handle, and with it a process id Windows could not hand out again, until a finaliser ran. + /// + /// + static void Release(TrackedMove move) + { + move.Process?.Dispose(); + move.Process = null; + } + public void Discard(TrackedMove move) { if (moves.TryRemove(move.Temp, out var removed)) @@ -851,6 +890,7 @@ static void DeleteTempDirectory(TrackedMove move) static void InnerDiscard(TrackedMove move) { KillProcesses(move); + Release(move); if (!FileEx.SafeDeleteFile(move.Temp)) { @@ -897,22 +937,42 @@ static void KillProcesses(TrackedMove move) /// make the button lie twice over — it discarded fewer things than it said, and the ones it /// skipped came back on the next scan two seconds later. /// + /// + /// The tracked files here, on the calling thread, and the snapshots on a worker, for the + /// reason gives: a queue a viewer owns is asked over a + /// socket, and one slow to answer held the thread drawing everything for as long as that + /// took. The menu and the hot key discard the task; tests await it. + /// /// - public void Clear() + public Task Clear() { ((ITrackedFiles) this).DiscardAll(); - // Only forget the cached snapshots when the owner actually discarded them. It used to be - // cleared regardless, so a discard the owner never received still emptied the menu - and - // everything came back on the next scan two seconds later - if (inline.DiscardAll(out var message)) - { - snapshots = []; - } - else + return Task.Run(() => { - Log.Error("{Message}", message ?? "Could not discard the pending snapshots."); - } + try + { + // Only forget the cached snapshots when the owner actually discarded them. It used + // to be cleared regardless, so a discard the owner never received still emptied the + // menu - and everything came back on the next scan two seconds later + if (inline.DiscardAll(out var message)) + { + snapshots = []; + } + else + { + Log.Error("{Message}", message ?? "Could not discard the pending snapshots."); + } + + // Nothing waits for the next scan to say so: the files went above, whatever the + // queue answered + ToggleActive(); + } + catch (Exception exception) + { + ExceptionHandler.Handle("Failed to discard the pending snapshots", exception); + } + }); } /// @@ -1025,6 +1085,77 @@ IReadOnlyList ITrackedFiles.Deletes() => _.File)) .ToList(); + readonly Lock versionGate = new(); + readonly List versioned = []; + long version; + + /// + /// By which objects are tracked, compared with the ones tracked the last time this was asked. + /// + /// Everything a listing carries of a move or a delete is fixed when the object is made, and a + /// change to either is another object in its place, so the same objects are the same listing. + /// Walking the two dictionaries and comparing references allocates nothing but the walk, where + /// describing every entry to hash the descriptions was a megabyte for a couple of hundred of + /// them. A dictionary nothing has touched is walked in the same order each time. One that was + /// touched and put back as it was may not be, which reads as a change and costs a listing. + /// + /// + /// Rather than a count of changes, kept wherever the dictionaries are written: there are a + /// score of such places, and one missed is a viewer that goes on showing a file that left. + /// + /// + long ITrackedFiles.Version() + { + lock (versionGate) + { + if (Unchanged()) + { + return version; + } + + versioned.Clear(); + foreach (var move in moves) + { + versioned.Add(move.Value); + } + + foreach (var delete in deletes) + { + versioned.Add(delete.Value); + } + + return ++version; + } + } + + bool Unchanged() + { + var index = 0; + foreach (var move in moves) + { + if (index == versioned.Count || + !ReferenceEquals(versioned[index], move.Value)) + { + return false; + } + + index++; + } + + foreach (var delete in deletes) + { + if (index == versioned.Count || + !ReferenceEquals(versioned[index], delete.Value)) + { + return false; + } + + index++; + } + + return index == versioned.Count; + } + void ITrackedFiles.AddMove(string temp, string target) { // No exe, arguments or process: the sender's diff tool details do not cross the viewer @@ -1054,7 +1185,13 @@ bool ITrackedFiles.Untrack(string key) { if (TrackedKeys.TryStrip(key, TrackedKeys.MovePrefix, out var temp)) { - return moves.TryRemove(temp, out _); + if (!moves.TryRemove(temp, out var removed)) + { + return false; + } + + Release(removed); + return true; } return TrackedKeys.TryStrip(key, TrackedKeys.DeletePrefix, out var file) && @@ -1223,10 +1360,11 @@ int ITrackedFiles.DiscardAll() if (InnerMove(removed, batch)) { + Release(removed); return (true, $"Accepted {removed.Name}"); } - moves.TryAdd(removed.Temp, removed); + Restore(removed); return (false, $"Files for '{removed.Name}' are locked. Accept from the tray menu to resolve."); } @@ -1261,6 +1399,7 @@ public ValueTask DisposeAsync() foreach (var move in moves.Values) { KillProcesses(move); + Release(move); } moves.Clear(); diff --git a/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so b/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so index ba559ab21..9a8747160 100644 Binary files a/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so and b/src/DiffEngineViewer.Linux/runtimes/linux-arm64/native/libdiffengine_viewer.so differ diff --git a/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so b/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so index 42421a863..072ba2222 100644 Binary files a/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so and b/src/DiffEngineViewer.Linux/runtimes/linux-x64/native/libdiffengine_viewer.so differ diff --git a/src/DiffEngineViewer.Mac/runtimes/osx-arm64/native/libdiffengine_viewer.dylib b/src/DiffEngineViewer.Mac/runtimes/osx-arm64/native/libdiffengine_viewer.dylib index 48d338336..c80b369d6 100644 Binary files a/src/DiffEngineViewer.Mac/runtimes/osx-arm64/native/libdiffengine_viewer.dylib and b/src/DiffEngineViewer.Mac/runtimes/osx-arm64/native/libdiffengine_viewer.dylib differ diff --git a/src/DiffEngineViewer.Mac/runtimes/osx-x64/native/libdiffengine_viewer.dylib b/src/DiffEngineViewer.Mac/runtimes/osx-x64/native/libdiffengine_viewer.dylib index 48d338336..c80b369d6 100644 Binary files a/src/DiffEngineViewer.Mac/runtimes/osx-x64/native/libdiffengine_viewer.dylib and b/src/DiffEngineViewer.Mac/runtimes/osx-x64/native/libdiffengine_viewer.dylib differ diff --git a/src/DiffEngineViewer.Tests/FileSideTests.cs b/src/DiffEngineViewer.Tests/FileSideTests.cs index 0e8a83548..9439c4fd4 100644 --- a/src/DiffEngineViewer.Tests/FileSideTests.cs +++ b/src/DiffEngineViewer.Tests/FileSideTests.cs @@ -79,6 +79,31 @@ public async Task UnreadableImageDegrades() await Assert.That(side.Image!.Value.Hash).IsNull(); } + /// + /// One array of the file's length and little else. Read through a stream that grew and then + /// copied out of it, a megabyte came to more than three. + /// + [Test] + public async Task AFileIsReadIntoOneArray() + { + var content = new byte[1024 * 1024]; + Random.Shared.NextBytes(content); + var file = Write("large.bin", content); + // Once before it is measured, so nothing counted is the first call's own + FileSide.ReadBytes(file); + + var before = GC.GetAllocatedBytesForCurrentThread(); + var bytes = FileSide.ReadBytes(file); + var allocated = GC.GetAllocatedBytesForCurrentThread() - before; + + await Assert.That(bytes.AsSpan().SequenceEqual(content)).IsTrue(); + await Assert.That(allocated).IsLessThan(content.Length + 256 * 1024); + } + + [Test] + public async Task AnEmptyFileIsNoBytes() => + await Assert.That(FileSide.ReadBytes(Write("empty.bin", []))).IsEmpty(); + static string Write(string name, byte[] content) { var path = Path.Combine(Directory(), name); diff --git a/src/DiffEngineViewer.Tests/PixelTests.ContextMenuOnTheLastRow.Linux.verified.png b/src/DiffEngineViewer.Tests/PixelTests.ContextMenuOnTheLastRow.Linux.verified.png new file mode 100644 index 000000000..9d3a6a949 Binary files /dev/null and b/src/DiffEngineViewer.Tests/PixelTests.ContextMenuOnTheLastRow.Linux.verified.png differ diff --git a/src/DiffEngineViewer.Tests/PixelTests.ImageTooLargeForATexture.Linux.verified.png b/src/DiffEngineViewer.Tests/PixelTests.ImageTooLargeForATexture.Linux.verified.png new file mode 100644 index 000000000..86935c0aa Binary files /dev/null and b/src/DiffEngineViewer.Tests/PixelTests.ImageTooLargeForATexture.Linux.verified.png differ diff --git a/src/DiffEngineViewer.Tests/PixelTests.ImagesReduced.Linux.verified.png b/src/DiffEngineViewer.Tests/PixelTests.ImagesReduced.Linux.verified.png new file mode 100644 index 000000000..57665a73a Binary files /dev/null and b/src/DiffEngineViewer.Tests/PixelTests.ImagesReduced.Linux.verified.png differ diff --git a/src/DiffEngineViewer.Tests/PixelTests.NamesWithHashes.Linux.verified.png b/src/DiffEngineViewer.Tests/PixelTests.NamesWithHashes.Linux.verified.png new file mode 100644 index 000000000..0186ec96d Binary files /dev/null and b/src/DiffEngineViewer.Tests/PixelTests.NamesWithHashes.Linux.verified.png differ diff --git a/src/DiffEngineViewer.Tests/PixelTests.cs b/src/DiffEngineViewer.Tests/PixelTests.cs index ef0da762e..b933809ea 100644 --- a/src/DiffEngineViewer.Tests/PixelTests.cs +++ b/src/DiffEngineViewer.Tests/PixelTests.cs @@ -345,6 +345,176 @@ await OnShimThread( await Capture(state); } + /// + /// The context menu opened on the last row of a queue that fills its column, where hung under + /// its row it would run off the bottom of the window, its last item with it: it goes over the + /// row instead. Linux only, for the reason is. + /// + /// At the rows the Linux head measures for a window this size, which is three more than the + /// other scenes are pinned to: its lines are 17 pixels and not 18, and it is that grid that + /// puts the last row 96 pixels above the window's bottom edge. The entry is a conflicted one + /// because its menu is the longest a row has, six items and 114 pixels. + /// + /// + [Test] + [PixelTest] + [NotInParallel(nameof(PixelTests), Order = 16)] + [SkipOnMac("The macOS head pops a real NSMenu, which a capture has no window to show.")] + public Task ContextMenuOnTheLastRow() + { + const int measuredRows = height / 17; + InlinePatch[] patches = + [ + .. Enumerable.Range(1, 32).Select(_ => Fixtures.Patch($"Tests{_:D2}.cs", _)), + Fixtures.Patch("Tests33.cs", 33, content: "eight", framework: "net8.0"), + Fixtures.Patch("Tests33.cs", 33, content: "nine", framework: "net9.0") + ]; + var state = ViewerSession.Resize(Fixtures.Inline(patches), columns, measuredRows); + return Capture(ViewerSession.OpenMenu(state, ScreenBuilder.BodyRows(state) - 1), measuredRows); + } + + /// + /// Names with ## in them, everywhere the Linux head hands a name to ImGui as an item's + /// label: a queue row, a group's heading, the two pane headers and the items of a menu. ImGui + /// takes everything from ## on as the item's identity and does not draw it, so each of + /// these stopped there. Linux only: no other head has a toolkit that reads a label that way. + /// + [Test] + [PixelTest] + [NotInParallel(nameof(PixelTests), Order = 17)] + [SkipOnMac("There is no macOS baseline for this scene: it is about how Dear ImGui reads a label, which that head does not use.")] + public Task NamesWithHashes() + { + var state = ViewerSession.EnqueueTracked( + SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows), + QueueEntry.ForMove( + "move:temp/Notes##2.received.txt", + "Notes##2 (txt)", + null, + "temp/Notes##2.received.txt", + "code/Notes##2.verified.txt", + FileSide.OfText(Fixtures.Received), + FileSide.OfText(Fixtures.Expected))); + state = ViewerSession.EnqueueInline( + state, + Fixtures.Patch(Fixtures.SolutionFile("Solution##A", "Tests", "A##Tests.cs"), 10)); + state = ViewerSession.EnqueueInline( + state, + Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 12)); + // The fifth row is the move, under the two solutions and their one entry each: the menu + // names its files, and opening it selects it, which puts them in the pane headers + return Capture(ViewerSession.OpenMenu(state, 4)); + } + + /// + /// Two pictures fitted at a third of their size: white, with a black line one pixel wide every + /// sixteen, across and down. Sampled between its own pixels and no others, which is all a + /// picture near its own size needs, a line survives only where a sample lands on it, so a grid + /// came out with some of its lines faint and some gone. The Linux head now draws a picture + /// under half its size from reduced copies of it, in which every line is there and fainter. + /// + /// Linux only. How a picture is reduced is each head's own toolkit's, so this one says + /// nothing about the others. + /// + /// + [Test] + [PixelTest] + [NotInParallel(nameof(PixelTests), Order = 18)] + [SkipOnMac("There is no macOS baseline for this scene: how a picture is reduced is each head's own.")] + public Task ImagesReduced() + { + var left = WriteBitmap("grid.received.bmp", 1600, 1200, 255, 16); + var right = WriteBitmap("grid.verified.bmp", 1200, 1600, 255, 16); + return Capture( + ViewerSession.EnqueueFile( + SessionState.Start(ViewerMode.File, Fixtures.Columns, Fixtures.Rows), + QueueEntry.ForFiles(left, right, FileSide.Read(left), FileSide.Read(right)))); + } + + /// + /// A picture longer on one side than a texture can be, beside one that is not. The rows say + /// what it is, as they do for a format the head has no decoder for, and nothing is drawn under + /// them: it was a black box the shape of the picture, which is what GL makes of a texture it + /// was handed and would not take. + /// + /// Linux only, and only where the limit is what it is under Mesa's software rasteriser, which + /// is what these baselines are pinned to: 16384 pixels, one fewer than this picture is wide. + /// + /// + [Test] + [PixelTest] + [NotInParallel(nameof(PixelTests), Order = 19)] + [SkipOnMac("There is no macOS baseline for this scene: it is about the largest texture the Linux head's GL takes.")] + public Task ImageTooLargeForATexture() + { + var left = WriteBitmap("wide.received.bmp", 16385, 512, 160, 0); + var right = WriteBitmap("wide.verified.bmp", 160, 120, 160, 0); + return Capture( + ViewerSession.EnqueueFile( + SessionState.Start(ViewerMode.File, Fixtures.Columns, Fixtures.Rows), + QueueEntry.ForFiles(left, right, FileSide.Read(left), FileSide.Read(right)))); + } + + /// + /// An eight bit greyscale bitmap of one shade, with a black line one pixel wide every + /// pixels across and down when that is not zero. A bitmap because + /// it is its header, a palette and its pixels, with nothing to compress, so its bytes are the + /// same on every runtime: the pane prints how many there are. + /// + static string WriteBitmap(string name, int width, int height, byte shade, int spacing) + { + const int headers = 14 + 40; + const int palette = 256 * 4; + // Each row is padded to a multiple of four bytes + var stride = (width + 3) / 4 * 4; + var bytes = new byte[headers + palette + stride * height]; + bytes[0] = (byte) 'B'; + bytes[1] = (byte) 'M'; + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(2), bytes.Length); + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(10), headers + palette); + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(14), 40); + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(18), width); + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(22), height); + // One plane, eight bits a pixel, uncompressed + BinaryPrimitives.WriteInt16LittleEndian(bytes.AsSpan(26), 1); + BinaryPrimitives.WriteInt16LittleEndian(bytes.AsSpan(28), 8); + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(34), stride * height); + BinaryPrimitives.WriteInt32LittleEndian(bytes.AsSpan(46), 256); + for (var index = 0; index < 256; index++) + { + bytes.AsSpan(headers + index * 4, 3).Fill((byte) index); + } + + for (var y = 0; y < height; y++) + { + // Rows are stored from the bottom one up + var row = bytes.AsSpan(headers + palette + (height - 1 - y) * stride, width); + row.Fill(shade); + if (spacing == 0) + { + continue; + } + + if (y % spacing == 0) + { + row.Clear(); + continue; + } + + for (var x = 0; x < width; x += spacing) + { + row[x] = 0; + } + } + + // A fixed directory and a fixed name, as Fixtures.Images has and for its reason + var directory = Path.Combine(Path.GetTempPath(), "deview-fixture-images"); + Directory.CreateDirectory(directory); + var path = Path.Combine(directory, name); + File.WriteAllBytes(path, bytes); + return path; + } + /// /// raylib does three things at the end of a frame, behind one flag: puts it on the screen, /// reads input, and waits for the next frame. raylib 6.0's CMake turned that flag on, so @@ -357,7 +527,7 @@ await OnShimThread( /// [Test] [PixelTest] - [NotInParallel(nameof(PixelTests), Order = 16)] + [NotInParallel(nameof(PixelTests), Order = 20)] [SkipOnMac("A capture host never creates the macOS window, and that head waits for the next frame in its event pump rather than after drawing one.")] public async Task PresentWaitsForTheNextFrame() { @@ -381,9 +551,9 @@ public async Task PresentWaitsForTheNextFrame() await Assert.That(elapsed).IsGreaterThan(TimeSpan.FromMilliseconds(750)); } - static async Task Capture(SessionState state) + static async Task Capture(SessionState state, int gridRows = rows) { - var screen = ScreenBuilder.Build(ViewerSession.Resize(state, columns, rows)); + var screen = ScreenBuilder.Build(ViewerSession.Resize(state, columns, gridRows)); var path = Path.Combine(Path.GetTempPath(), $"deview-{Guid.NewGuid():N}.png"); try { diff --git a/src/DiffEngineViewer.Tests/SelectionTests.cs b/src/DiffEngineViewer.Tests/SelectionTests.cs index 9751c6e15..3cae4d9a6 100644 --- a/src/DiffEngineViewer.Tests/SelectionTests.cs +++ b/src/DiffEngineViewer.Tests/SelectionTests.cs @@ -436,6 +436,23 @@ public async Task A_drag_after_a_non_bmp_character_copies_what_was_highlighted() /// /// Select all ends at the last cell of the last row, not a cell further per wide character. /// + // What a menu asks before it offers to copy a side, which has to be what copying it would find + [Test] + [Arguments("", "one")] + [Arguments("\n", "one")] + [Arguments("one", "")] + [Arguments("one\ntwo", "one")] + [Arguments("", "")] + public async Task Whether_a_side_has_anything_to_copy_is_what_copying_it_finds(string left, string right) + { + var entry = Fixtures.Move(left: left, right: right); + + foreach (var side in new[] { PaneSide.Left, PaneSide.Right }) + { + await Assert.That(SelectionText.Any(entry, side)).IsEqualTo(SelectionText.All(entry, side).Length > 0); + } + } + [Test] public async Task Select_all_ends_on_the_last_cell() { diff --git a/src/DiffEngineViewer.Tests/TrackedWatchTests.cs b/src/DiffEngineViewer.Tests/TrackedWatchTests.cs index d276c7c83..b7b0175b4 100644 --- a/src/DiffEngineViewer.Tests/TrackedWatchTests.cs +++ b/src/DiffEngineViewer.Tests/TrackedWatchTests.cs @@ -306,6 +306,56 @@ public async Task SnapshotsTakeNothingFromWhatAPassLooksAt() await Assert.That(host.State.Queue.Count(_ => _.Kind == QueueEntryKind.Inline)).IsEqualTo(2); } + /// + /// A run that fails the same way writes its received file again with what it held. The entry + /// is the one it was with a new stamp: its rows are not built again, since building them is + /// the diff, and the pass after has nothing to do. + /// + [Test] + public async Task AFileWrittenAgainWithWhatItHeldIsNotDiffedAgain() + { + var (temp, target) = Pair("Sample.Test"); + var host = Owned(TrackedEntry.ForMove(temp, target)); + var before = host.State.Queue.Single(); + File.SetLastWriteTimeUtc(temp, DateTime.UtcNow.AddMinutes(1)); + var watch = new TrackedWatch(host); + + watch.Pump(); + + var after = host.State.Queue.Single(); + await Assert.That(ReferenceEquals(after.LeftRows, before.LeftRows)).IsTrue(); + await Assert.That(after.LeftStamp).IsNotEqualTo(before.LeftStamp); + + var settled = host.State; + watch.Pump(); + await Assert.That(ReferenceEquals(host.State, settled)).IsTrue(); + } + + /// + /// The same over the socket, which is how the run itself says the pair is pending again. + /// + [Test] + public async Task APairSentAgainUnchangedIsNotDiffedAgain() + { + var (temp, target) = Pair("Sample.Test"); + var host = new SessionHost(SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows)); + IQueueOwner owner = new MessageHandler(host, Fixtures.Applied, _ => { }); + owner.TrackMove(temp, target); + var before = host.State.Queue.Single(); + File.SetLastWriteTimeUtc(temp, DateTime.UtcNow.AddMinutes(1)); + + owner.TrackMove(temp, target); + + var after = host.State.Queue.Single(); + await Assert.That(ReferenceEquals(after.LeftRows, before.LeftRows)).IsTrue(); + await Assert.That(after.LeftStamp).IsNotEqualTo(before.LeftStamp); + + await File.WriteAllTextAsync(temp, "what a later run received instead"); + owner.TrackMove(temp, target); + + await Assert.That(host.State.Queue.Single().LeftText).IsEqualTo("what a later run received instead"); + } + SessionHost OwnedAll(int count) { var state = SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows); diff --git a/src/DiffEngineViewer.Windows.Benchmarks/ModuleInitializer.cs b/src/DiffEngineViewer.Windows.Benchmarks/ModuleInitializer.cs new file mode 100644 index 000000000..8aea69b45 --- /dev/null +++ b/src/DiffEngineViewer.Windows.Benchmarks/ModuleInitializer.cs @@ -0,0 +1,17 @@ +using System.Runtime.CompilerServices; + +static class ModuleInitializer +{ + /// + /// Nothing here makes a window, and this is for the day something does. WinForms answers an + /// exception thrown inside a window message with a dialog offering Continue and Quit, and a + /// benchmark run would sit behind it until somebody clicked. Thrown instead, it ends the run. + /// Here rather than in Program because it is refused once any window exists, and a + /// module initializer runs before anything in the assembly can have made one. For the + /// application, since the overload without threadScope sets it for the calling thread alone + /// and a benchmark runs on whichever thread BenchmarkDotNet gives it. + /// + [ModuleInitializer] + public static void Initialize() => + Application.SetUnhandledExceptionMode(UnhandledExceptionMode.ThrowException, threadScope: false); +} diff --git a/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs b/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs index dd6949112..b7b2bd457 100644 --- a/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/FormsHeadTests.cs @@ -571,6 +571,52 @@ public async Task AKeyThenAClickInOnePumpAreAppliedInTheirOrder() await Assert.That(left).Contains(clicked); } + /// + /// d held down past the repeat delay, with three snapshots queued. The press discards the one + /// on screen. The repeats would each have discarded whichever took its place, which nobody had + /// read. + /// + [Test] + public async Task AHeldDiscardIsOneDiscard() + { + using var host = new FormHost( + Fixtures.Inline( + Fixtures.Patch("ATests.cs", 10, content: "one"), + Fixtures.Patch("BTests.cs", 20, content: "two"), + Fixtures.Patch("CTests.cs", 30, content: "three"))); + host.Settle(); + var onScreen = host.State.Current!.Key; + + host.PostHeld(Keys.D, repeats: 4); + for (var frame = 0; frame < 6; frame++) + { + host.Frame(); + } + + var left = host.State.Queue.Select(_ => _.Key).ToList(); + await Assert.That(left.Count).IsEqualTo(2); + await Assert.That(left).DoesNotContain(onScreen); + } + + /// + /// Down held: every repeat is a row, which is what holding it is for. + /// + [Test] + public async Task AHeldScrollKeepsItsRepeats() + { + using var host = new FormHost(Fixtures.File(Lines(300, 3), Lines(300))); + host.Settle(); + var before = ScreenBuilder.Build(host.State).Left.ScrollTop; + + host.PostHeld(Keys.Down, repeats: 4); + for (var frame = 0; frame < 6; frame++) + { + host.Frame(); + } + + await Assert.That(ScreenBuilder.Build(host.State).Left.ScrollTop - before).IsEqualTo(5); + } + /// /// Right click a row, which opens its menu, then right click the same row again. /// @@ -613,7 +659,60 @@ public async Task RightClickingTheRowWhoseMenuIsOpen() /// /// [Test] - public async Task DraggingTheThumbStillRunsFrames() + public Task DraggingTheThumbStillRunsFrames() => + WithTheLeftButtonHeld(() => TrackTheBar(BarPart.Thumb)); + + /// + /// The arrow at the foot of the bar, pressed and held for half a second. user32 tracks that in + /// the loop it tracks the thumb in, sending a line down and then one for every repeat, and the + /// panes follow only if frames come from inside it. They were entered for the thumb alone, so + /// the panes stood still for as long as the arrow was held and jumped when it was let go. + /// + [Test] + public Task HoldingTheArrowStillRunsFrames() => + WithTheLeftButtonHeld(() => TrackTheBar(BarPart.Arrow)); + + /// + /// The trough under the thumb, held: a page down and then one for every repeat, from the same + /// loop. + /// + [Test] + public Task HoldingTheTroughStillRunsFrames() => + WithTheLeftButtonHeld(() => TrackTheBar(BarPart.Trough)); + + /// + /// A line down sent to the bar with no press behind it, and so with no EndScroll after it: + /// nothing is tracking, and the frames started for a loop that never was stop when the real + /// loop next presents. + /// + [Test] + public async Task AScrollWithNoEndStopsItsFramesAtTheNextPresent() + { + using var form = new ViewerForm("title", 800, 600) + { + Frame = () => ScreenBuilder.Build(Fixtures.File()) + }; + var bar = Field(form, "scrollBar"); + var frames = Field(form, "modalFrames"); + + typeof(ScrollBar) + .GetMethod("OnScroll", BindingFlags.Instance | BindingFlags.NonPublic)! + .Invoke(bar, [new ScrollEventArgs(ScrollEventType.SmallIncrement, 1)]); + var entered = frames.Enabled; + form.LoopReturned(); + + await Assert.That(entered).IsTrue(); + await Assert.That(frames.Enabled).IsFalse(); + } + + enum BarPart + { + Thumb, + Arrow, + Trough + } + + static async Task WithTheLeftButtonHeld(Func track) { var keys = new byte[256]; GetKeyboardState(keys); @@ -622,7 +721,7 @@ public async Task DraggingTheThumbStillRunsFrames() SetKeyboardState(keys); try { - await DragTheThumb(); + await track(); } finally { @@ -650,7 +749,7 @@ public async Task TheBarCountsTheRowsThePaneShows() await Assert.That(bar.LargeChange).IsEqualTo(ScreenBuilder.PaneRows(host.State)); } - static async Task DragTheThumb() + static async Task TrackTheBar(BarPart part) { using var host = new FormHost(Fixtures.File(Lines(400, 3), Lines(400))); host.Settle(); @@ -661,7 +760,17 @@ static async Task DragTheThumb() }; GetScrollBarInfo(bar.Handle, objectClient, ref info); var x = bar.Width / 2; - var y = (info.ThumbTop + info.ThumbBottom) / 2; + // The arrow is a square at the foot of the bar, and the trough is what lies between the + // thumb and it + var arrowTop = bar.Height - info.LineButton; + var y = part switch + { + BarPart.Thumb => (info.ThumbTop + info.ThumbBottom) / 2, + BarPart.Arrow => arrowTop + info.LineButton / 2, + _ => (info.ThumbBottom + arrowTop) / 2 + }; + // Only the thumb is dragged. An arrow or the trough is held where it was pressed. + var (dragged, released) = part == BarPart.Thumb ? (y + 40, y + 80) : (y, y); var clock = Stopwatch.StartNew(); var scrolls = new List(); @@ -690,9 +799,9 @@ static async Task DragTheThumb() var release = new Thread(() => { Thread.Sleep(500); - PostMessage(handle, mouseMove, leftButtonFlag, Point(x, y + 80)); + PostMessage(handle, mouseMove, leftButtonFlag, Point(x, released)); Thread.Sleep(50); - PostMessage(handle, leftButtonUp, IntPtr.Zero, Point(x, y + 80)); + PostMessage(handle, leftButtonUp, IntPtr.Zero, Point(x, released)); // Only if the bar never let go, so a failure here cannot hang the run. for (var wait = 0; wait < 60 && !Volatile.Read(ref done); wait++) { @@ -701,7 +810,7 @@ static async Task DragTheThumb() if (!Volatile.Read(ref done)) { - PostMessage(handle, leftButtonUp, IntPtr.Zero, Point(x, y + 80)); + PostMessage(handle, leftButtonUp, IntPtr.Zero, Point(x, released)); PostMessage(handle, cancelMode, IntPtr.Zero, IntPtr.Zero); } }) @@ -713,7 +822,7 @@ static async Task DragTheThumb() try { PostMessage(handle, leftButtonDown, leftButtonFlag, Point(x, y)); - PostMessage(handle, mouseMove, leftButtonFlag, Point(x, y + 40)); + PostMessage(handle, mouseMove, leftButtonFlag, Point(x, dragged)); release.Start(); var pump = Stopwatch.StartNew(); Application.DoEvents(); @@ -728,10 +837,13 @@ static async Task DragTheThumb() } Console.WriteLine( - $"one DoEvents took {pumped}ms; scroll bar's own loop filtered {filtered} messages; " + + $"{part}: one DoEvents took {pumped}ms; scroll bar's own loop filtered {filtered} messages; " + $"Scroll events: {string.Join(", ", scrolls)}; frames during it: {tops.Count}, scroll tops {string.Join(" ", tops.Distinct())}"); await Assert.That(tops.Count).IsGreaterThan(5); await Assert.That(tops.Max()).IsGreaterThan(0); + // Left with the loop: frames that went on coming from the timer afterwards would be a + // second loop beside the real one + await Assert.That(Field(host.Form, "modalFrames").Enabled).IsFalse(); } /// @@ -1223,6 +1335,21 @@ public void PostKey(Keys key) PostMessage(Canvas.Handle, keyUp, new((int) key), new(unchecked((int) 0xC0000001))); } + /// + /// A key pressed and held: the press, then what the keyboard sends for as long as it stays + /// down, which is the same message with bit 30 saying the key was already down. + /// + public void PostHeld(Keys key, int repeats) + { + PostMessage(Canvas.Handle, keyDown, new((int) key), new(1)); + for (var index = 0; index < repeats; index++) + { + PostMessage(Canvas.Handle, keyDown, new((int) key), new(0x40000001)); + } + + PostMessage(Canvas.Handle, keyUp, new((int) key), new(unchecked((int) 0xC0000001))); + } + public void PostClick(int queueRow, bool right) { var cell = Canvas.CellSize(); diff --git a/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs b/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs new file mode 100644 index 000000000..ed5778b8d --- /dev/null +++ b/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs @@ -0,0 +1,40 @@ +/// +/// How long the loop waits between frames: a sixtieth of a second for a window somebody can see, +/// a tenth for one nobody can. +/// +[NotInParallel] +[TUnit.Core.Executors.STAThreadExecutor] +public class FrameWaitTests +{ + [Test] + [Arguments(true, FormWindowState.Normal, 15)] + [Arguments(true, FormWindowState.Maximized, 15)] + [Arguments(true, FormWindowState.Minimized, 100)] + [Arguments(false, FormWindowState.Normal, 100)] + [Arguments(false, FormWindowState.Minimized, 100)] + public async Task Waits(bool visible, FormWindowState state, int milliseconds) => + await Assert.That(FormsViewerWindow.FrameWait(visible, state)).IsEqualTo(milliseconds); + + /// + /// Why the state is asked as well: a minimised window is still Visible, which was the whole of + /// the test, so it was taken for one on screen. + /// + [Test] + public async Task AMinimisedWindowIsStillVisible() + { + // Shown, since a window never shown is not Visible whatever its state, and transparent + // and parked, as ViewerFormRaiseTests shows one, so nothing appears on the desktop + using var form = new ViewerForm("title", 800, 600) + { + Opacity = 0, + ShowInTaskbar = false, + StartPosition = FormStartPosition.Manual, + Location = new(-4000, -2000) + }; + form.Show(); + form.WindowState = FormWindowState.Minimized; + + await Assert.That(form.Visible).IsTrue(); + await Assert.That(FormsViewerWindow.FrameWait(form.Visible, form.WindowState)).IsEqualTo(100); + } +} diff --git a/src/DiffEngineViewer.Windows.Tests/KeyMapTests.cs b/src/DiffEngineViewer.Windows.Tests/KeyMapTests.cs new file mode 100644 index 000000000..1168f305f --- /dev/null +++ b/src/DiffEngineViewer.Windows.Tests/KeyMapTests.cs @@ -0,0 +1,52 @@ +/// +/// Which command a key is, by : the key with the modifiers held, as +/// ProcessCmdKey is handed it. +/// +public class KeyMapTests +{ + /// + /// Every key that is a command on its own, with Alt held. None of them is one then: the three + /// that change something were Alt+A accepting, Alt+D discarding and Alt+Q quitting. + /// + [Test] + public async Task AnAltChordIsNoCommand() + { + foreach (var key in Enum.GetValues().Distinct()) + { + if (ViewerForm.Map(key) == CommandKind.None) + { + continue; + } + + await Assert.That(ViewerForm.Map(Keys.Alt | key)).IsEqualTo(CommandKind.None).Because($"Alt+{key}"); + await Assert.That(ViewerForm.Map(Keys.Alt | Keys.Shift | key)).IsEqualTo(CommandKind.None).Because($"Alt+Shift+{key}"); + } + } + + /// + /// Alt Gr is Control and Alt together on the layouts that have it, and what it types is a + /// character: Alt Gr+C is not copy and Alt Gr+0, a closing brace on a German keyboard, is not + /// a zoom reset. + /// + [Test] + [Arguments(Keys.C)] + [Arguments(Keys.A)] + [Arguments(Keys.D0)] + [Arguments(Keys.Oemplus)] + public async Task AltGrIsNoCommand(Keys key) => + await Assert.That(ViewerForm.Map(Keys.Control | Keys.Alt | key)).IsEqualTo(CommandKind.None); + + /// + /// What the chords were before, which Alt being answered first must not have moved. + /// + [Test] + public async Task TheOtherChordsAreWhatTheyWere() + { + await Assert.That(ViewerForm.Map(Keys.A)).IsEqualTo(CommandKind.Accept); + await Assert.That(ViewerForm.Map(Keys.Shift | Keys.A)).IsEqualTo(CommandKind.AcceptAll); + await Assert.That(ViewerForm.Map(Keys.Control | Keys.A)).IsEqualTo(CommandKind.SelectAll); + await Assert.That(ViewerForm.Map(Keys.Control | Keys.C)).IsEqualTo(CommandKind.Copy); + await Assert.That(ViewerForm.Map(Keys.D)).IsEqualTo(CommandKind.Discard); + await Assert.That(ViewerForm.Map(Keys.Q)).IsEqualTo(CommandKind.Quit); + } +} diff --git a/src/DiffEngineViewer.Windows.Tests/ModuleInitializer.cs b/src/DiffEngineViewer.Windows.Tests/ModuleInitializer.cs index 1c727ae6f..545870eaf 100644 --- a/src/DiffEngineViewer.Windows.Tests/ModuleInitializer.cs +++ b/src/DiffEngineViewer.Windows.Tests/ModuleInitializer.cs @@ -3,6 +3,13 @@ public static class ModuleInitializer [ModuleInitializer] public static void Initialize() { + // First, since it is refused once any window exists. WinForms answers an exception thrown + // inside a window message with a dialog offering Continue and Quit, which on a desktop + // somebody is working at is a test run waiting for a click nobody knows to make. Thrown + // instead, it comes out of the DoEvents that dispatched the message and fails the test. + // For the application: the overload without threadScope sets it for the calling thread + // alone, and no test runs on the thread a module initializer does. + Application.SetUnhandledExceptionMode(UnhandledExceptionMode.ThrowException, threadScope: false); MachineSettings.Ignore(); VerifyWinForms.Initialize(); // Effectively "the same pixels", rather than Verify's 0.98 default. A viewer screen is diff --git a/src/DiffEngineViewer.Windows.Tests/UnhandledExceptionTests.cs b/src/DiffEngineViewer.Windows.Tests/UnhandledExceptionTests.cs new file mode 100644 index 000000000..c493b1449 --- /dev/null +++ b/src/DiffEngineViewer.Windows.Tests/UnhandledExceptionTests.cs @@ -0,0 +1,30 @@ +/// +/// What an exception thrown inside a window message does in this test host. WinForms' own answer is +/// a dialog offering Continue and Quit, and a run then waits on the desktop of whoever started it +/// for a click. asks for it to be thrown instead. +/// +[NotInParallel] +[TUnit.Core.Executors.STAThreadExecutor] +public class UnhandledExceptionTests +{ + /// + /// Asked of WinForms rather than found out by throwing, because being wrong about it is the + /// dialog. On the thread a test runs on, which is not the one the module initializer ran on: + /// set for that thread alone, the mode left every test thread with the dialog. + /// + [Test] + public async Task AWindowOnATestThreadLetsAnExceptionThrough() + { + var debuggable = typeof(NativeWindow).GetProperty( + "WndProcShouldBeDebuggable", + BindingFlags.Static | BindingFlags.Instance | BindingFlags.NonPublic); + // WinForms' own name for it. If that goes, this has to find the answer some other way + // that does not involve throwing. + await Assert.That(debuggable).IsNotNull(); + + var window = new NativeWindow(); + var lets = (bool) debuggable!.GetValue(debuggable.GetMethod!.IsStatic ? null : window)!; + + await Assert.That(lets).IsTrue(); + } +} diff --git a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.StatusThatDoesNotFit.verified.png b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.StatusThatDoesNotFit.verified.png new file mode 100644 index 000000000..619e42a18 Binary files /dev/null and b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.StatusThatDoesNotFit.verified.png differ diff --git a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs index 0445ff5ca..8706f7e05 100644 --- a/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/WindowsPixelTests.cs @@ -182,9 +182,30 @@ public Task InlineAccepted() return Capture(ViewerSession.Apply(state, CommandKind.Accept, Fixtures.Applied)); } - static async Task Capture(SessionState state) + /// + /// A status wider than the footer has left beside a document's buttons: one line, from its + /// start, ending in an ellipsis where it is cut. The status line is where the model says what + /// no picture can, and it says the thing that matters first. Wrapped into a label two lines + /// high and centred, what showed was the middle of it. + /// + [Test] + public Task StatusThatDoesNotFit() + { + var screen = ScreenBuilder.Build(ViewerSession.Resize(Fixtures.Document(), columns, rows)); + return Capture( + screen with + { + // Five lines of the label's width, where it has the height for two + Status = "START lines 1-14 of 40, page 1 of 1, page 1 differs, 200% zoom, selected 3 lines of sample.received.pdf, " + + "which is 112 characters, and could not be drawn: Not a readable PDF document: it was cut short END" + }); + } + + static Task Capture(SessionState state) => + Capture(ScreenBuilder.Build(ViewerSession.Resize(state, columns, rows))); + + static async Task Capture(Screen screen) { - var screen = ScreenBuilder.Build(ViewerSession.Resize(state, columns, rows)); var path = Path.Combine(Path.GetTempPath(), $"deview-{Guid.NewGuid():N}.png"); try { diff --git a/src/DiffEngineViewer.Windows/FormsViewerWindow.cs b/src/DiffEngineViewer.Windows/FormsViewerWindow.cs index 43663cea9..3e22ac987 100644 --- a/src/DiffEngineViewer.Windows/FormsViewerWindow.cs +++ b/src/DiffEngineViewer.Windows/FormsViewerWindow.cs @@ -59,6 +59,7 @@ public bool Present(Screen screen) return false; } + form.LoopReturned(); form.Apply(screen); // Every frame rather than only on a changed screen: a spinner turns while nothing about the // screen changes, which is the whole time a page is being drawn @@ -89,10 +90,21 @@ void Wait() return; } - var timeout = form.Visible ? frameMilliseconds - 1 : hiddenMilliseconds; + var timeout = FrameWait(form.Visible, form.WindowState); MsgWaitForMultipleObjectsEx(0, IntPtr.Zero, (uint) timeout, allInput, inputAvailable); } + /// + /// How long to wait for the next frame. A minimised window waits as a hidden one does: it is + /// still Visible to WinForms, so it went on at sixty frames a second with nothing of it on + /// screen, for as long as it sat in the taskbar. Not a third state, because what a hidden + /// window has to hear is what a minimised one has to: the wait ends on any message, which is + /// the click that restores it, and the loop reads what the listener queued each time it wakes, + /// so a snapshot that arrives raises the window within a tenth of a second either way. + /// + internal static int FrameWait(bool visible, FormWindowState state) => + visible && state != FormWindowState.Minimized ? frameMilliseconds - 1 : hiddenMilliseconds; + const int hiddenMilliseconds = 100; const uint allInput = 0x04FF; const uint inputAvailable = 0x0004; diff --git a/src/DiffEngineViewer.Windows/ViewerForm.cs b/src/DiffEngineViewer.Windows/ViewerForm.cs index c68e56233..e29fa09ef 100644 --- a/src/DiffEngineViewer.Windows/ViewerForm.cs +++ b/src/DiffEngineViewer.Windows/ViewerForm.cs @@ -25,6 +25,12 @@ sealed class ViewerForm : Form TextAlign = ContentAlignment.MiddleRight, ForeColor = Palette.Dim, AutoSize = false, + // A status too long for what the buttons leave wraps, and the label has the height for two + // lines. The rest was cut with nothing to say so, and the status line is where the model + // says what no picture can. With this the cut ends in an ellipsis, and the label shows the + // whole status as a tip when the pointer rests on it. It is cut from the end either way: + // the lines kept are the first two, so right aligned still keeps the start. + AutoEllipsis = true, // The status line is built from paths, solution names and whatever the applier said, and a // Label reads an ampersand in any of those as a mnemonic: "R&D" drew as "R_D" with D live // as an accelerator. @@ -176,18 +182,18 @@ public ViewerForm(string title, int width, int height, WindowPlacement? placemen scrollBar.Scroll += (_, e) => { scrollTo = e.NewValue; - // The thumb is tracked in the scroll bar's own modal loop, from the first ThumbTrack - // until the release - if (e.Type == ScrollEventType.ThumbTrack) - { - EnterModal(); - return; - } - + // Every part of the bar is tracked in its own modal loop, from the press until the + // release: an arrow or the trough held down as much as the thumb dragged, which was + // the only one this entered for, so holding an arrow moved nothing until it was let + // go. EndScroll is what the bar sends as that loop ends, whichever part it was, and + // ThumbPosition comes just ahead of it. if (e.Type is ScrollEventType.ThumbPosition or ScrollEventType.EndScroll) { ExitModal(); + return; } + + EnterModal(); }; modalFrames.Tick += (_, _) => { @@ -454,6 +460,15 @@ void EnterModal() void ExitModal() => modalFrames.Stop(); + /// + /// The loop is presenting a frame of its own, so nothing is holding the thread: whatever modal + /// loop was entered has returned. Its exit is normally what says so. A scroll sent to the bar + /// by something other than a press - an accessibility tool, say - need not be followed by an + /// EndScroll, and frames would then come from the timer as well as the loop from there on. + /// + public void LoopReturned() => + ExitModal(); + const int enterSizeMove = 0x0231; const int exitSizeMove = 0x0232; @@ -751,12 +766,37 @@ protected override bool ProcessCmdKey(ref Message message, Keys keyData) return base.ProcessCmdKey(ref message, keyData); } + // A key held past the repeat delay arrives again thirty times a second, and each one is + // queued. That is what a held Down is for. A held a accepted the entry on screen and then + // every one that took its place, into source, none of them read. So what changes the + // queue takes a press each. Swallowed rather than passed on: it is still this key. + if (ViewerSession.ChangesQueue(command) && + IsRepeat(message)) + { + return true; + } + discrete.Enqueue(new(Key: command)); return true; } - static CommandKind Map(Keys keyData) + /// + /// Bit 30 of a key message's LParam: the key was already down when this was sent. + /// + static bool IsRepeat(Message message) => + (message.LParam.ToInt64() & 1L << 30) != 0; + + internal static CommandKind Map(Keys keyData) { + // No command is an Alt chord, and the switch below reads only the key code, so Alt+A was + // accept, Alt+D discard and Alt+Q quit: a reach for a menu that is not there wrote a + // snapshot into source. Ahead of Control, because Alt Gr arrives as both, and a character + // typed with it is not a Control chord either. + if ((keyData & Keys.Alt) == Keys.Alt) + { + return CommandKind.None; + } + var shift = (keyData & Keys.Shift) == Keys.Shift; var code = keyData & Keys.KeyCode; // Answered on its own rather than folded into the switch, which reads only the key code: diff --git a/src/DiffEngineViewer/FileSide.cs b/src/DiffEngineViewer/FileSide.cs index c4da7a87b..b4720e007 100644 --- a/src/DiffEngineViewer/FileSide.cs +++ b/src/DiffEngineViewer/FileSide.cs @@ -102,12 +102,39 @@ static FileSide ReadDocument(string path, FileStamp stamp, DocumentPlugin docume static FileStream OpenShared(string path) => new(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + /// + /// Into one array of the file's own length. Through a growing MemoryStream and out of it + /// again, a picture or a document was copied twice over on its way in. + /// + /// The length is what it was when asked, and the file is shared with whoever is writing it, + /// so both ways it can have changed by the time it is read are answered as a copy to the end + /// answered them: one cut short is what was there, and one that grew is read to its new end. + /// + /// public static byte[] ReadBytes(string path) { using var stream = OpenShared(path); - using var memory = new MemoryStream(); - stream.CopyTo(memory); - return memory.ToArray(); + var bytes = new byte[stream.Length]; + var read = 0; + while (read < bytes.Length) + { + var count = stream.Read(bytes, read, bytes.Length - read); + if (count == 0) + { + return bytes[..read]; + } + + read += count; + } + + using var grown = new MemoryStream(); + stream.CopyTo(grown); + if (grown.Length == 0) + { + return bytes; + } + + return [..bytes, ..grown.ToArray()]; } /// diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index f81991ada..ad7fbc2c9 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -50,19 +50,36 @@ void IQueueOwner.Settle(string key, string? origin, string? member, string? valu /// seam materializes the tray's tracked files through. Before the lock /// rather than inside it: building an entry reads both files and diffs them, and the render /// loop takes the same lock every frame. + /// + /// A pair that is queued already is asked whether it still says what it said before it is + /// built again (). The queued entry is read outside the + /// lock too, and may have gone or changed by the time this one is put in. Either way what is + /// put in is what the files were read as, which is all an arrival ever was. + /// /// void IQueueOwner.TrackMove(string temp, string target) { - var entry = TrackedEntry.ForMove(temp, target, documents); + var entry = Queued(TrackedKeys.ForMove(temp)) is { } queued + ? TrackedEntry.MoveAgain(queued, temp, target, documents) + : TrackedEntry.ForMove(temp, target, documents); RefuseWhenClosing(host.Mutate(_ => ViewerSession.EnqueueTracked(_, entry))); } void IQueueOwner.TrackDelete(string file) { - var entry = TrackedEntry.ForDelete(file, documents); + var entry = Queued(TrackedKeys.ForDelete(file)) is { } queued + ? TrackedEntry.DeleteAgain(queued, file, documents) + : TrackedEntry.ForDelete(file, documents); RefuseWhenClosing(host.Mutate(_ => ViewerSession.EnqueueTracked(_, entry))); } + QueueEntry? Queued(string key) + { + var state = host.State; + var index = IndexOf(state, key); + return index < 0 ? null : state.Queue[index]; + } + /// /// Thrown rather than returned, because has no refusal to return for /// these verbs, and a throwing handler is answered with an error: the sender then stages or diff --git a/src/DiffEngineViewer/MenuState.cs b/src/DiffEngineViewer/MenuState.cs index a0f7afcc5..c9fdc43aa 100644 --- a/src/DiffEngineViewer/MenuState.cs +++ b/src/DiffEngineViewer/MenuState.cs @@ -95,7 +95,7 @@ public static IReadOnlyList ForPane(QueueEntry entry, PaneSide side, b items.Add(new("Copy selection", CommandKind.Copy)); } - if (SelectionText.All(entry, side).Length > 0) + if (SelectionText.Any(entry, side)) { items.Add(new("Copy all", side == PaneSide.Left ? CommandKind.CopyLeft : CommandKind.CopyRight)); if (selectable) @@ -114,7 +114,7 @@ public static IReadOnlyList ForPane(QueueEntry entry, PaneSide side, b /// static void AddCopy(List items, QueueEntry entry, PaneSide side, CommandKind kind) { - if (SelectionText.All(entry, side).Length > 0) + if (SelectionText.Any(entry, side)) { items.Add(new($"Copy {SelectionText.Header(entry, side)}", kind)); } diff --git a/src/DiffEngineViewer/QueueEntry.cs b/src/DiffEngineViewer/QueueEntry.cs index 8b136dca2..21f53f5d5 100644 --- a/src/DiffEngineViewer/QueueEntry.cs +++ b/src/DiffEngineViewer/QueueEntry.cs @@ -174,6 +174,34 @@ public bool ShowsProperties(DrawingView drawing) => public bool Conflicted => Variants.Count > 1; + /// + /// Whether one side of an entry holds what a side that arrived, or was read again, holds. + /// + public static bool SameSide( + string text, + ImageFile? image, + DocumentFile? document, + string arrivedText, + ImageFile? arrivedImage, + DocumentFile? arrivedDocument) + { + if (document is { } held && + arrivedDocument is { } sent) + { + // By its bytes. Its text follows from them, and is read after the entry arrives, so + // one of the two may hold it while the other is still waiting for it. + return held.Path == sent.Path && + held.Format == sent.Format && + held.Length == sent.Length && + held.Hash == sent.Hash; + } + + return document is null && + arrivedDocument is null && + image == arrivedImage && + text == arrivedText; + } + public static string KeyForInline(string sourceFile, int line) => InlineKey.For(sourceFile, line); diff --git a/src/DiffEngineViewer/QueueProjection.cs b/src/DiffEngineViewer/QueueProjection.cs index a21d59c4c..cd7dc246e 100644 --- a/src/DiffEngineViewer/QueueProjection.cs +++ b/src/DiffEngineViewer/QueueProjection.cs @@ -39,7 +39,7 @@ public static IReadOnlyList Order(IReadOnlyList entries) } // Each entry's group worked out once, and its mates collected in one pass, rather than every - // entry asking every other one for a key built from two new strings. This runs twice per + // entry asking every other one for a key built from two new strings. This runs for every // change to the queue, under the lock the render loop takes, and at a few hundred entries // the pairwise version was tens of milliseconds and megabytes of garbage each time. var groups = new string?[entries.Count]; diff --git a/src/DiffEngineViewer/SelectionText.cs b/src/DiffEngineViewer/SelectionText.cs index 24eff5f34..3789fa939 100644 --- a/src/DiffEngineViewer/SelectionText.cs +++ b/src/DiffEngineViewer/SelectionText.cs @@ -132,6 +132,32 @@ public static string All(QueueEntry entry, PaneSide side) => .Where(_ => _.Kind != RowKind.Filler) .Select(_ => _.Text)); + /// + /// Whether would hand anything over, asked without building it. A menu asks + /// this of both sides as it opens, to leave out an item that would copy nothing, and joining + /// a large file's rows into one string to measure it was megabytes for every right click. + /// + public static bool Any(QueueEntry entry, PaneSide side) + { + var lines = 0; + foreach (var row in Rows(entry, side)) + { + if (row.Kind == RowKind.Filler) + { + continue; + } + + // A second line is a newline between the two, whatever either holds + if (row.Text.Length > 0 || + ++lines > 1) + { + return true; + } + } + + return false; + } + /// /// What the status line says while something is selected. The universal statement about a /// selection: the heads that can draw a highlight also draw this, and the one that cannot diff --git a/src/DiffEngineViewer/TrackedEntry.cs b/src/DiffEngineViewer/TrackedEntry.cs index 6e069edb9..b4286178f 100644 --- a/src/DiffEngineViewer/TrackedEntry.cs +++ b/src/DiffEngineViewer/TrackedEntry.cs @@ -30,6 +30,57 @@ public static QueueEntry ForDelete(string file, DocumentPlugin? documents = null file, FileSide.Read(file, documents)); + /// + /// The entry for a pair whose files have been read again: the queued one with the files' new + /// stamps when the two sides hold what it shows, and one built from them when they do not. + /// + /// Building an entry diffs its two sides, which for a large pair is most of what an arrival + /// costs, and a test that keeps failing the same way writes its received file again and sends + /// the pair on every run. So the sides are asked as they were read, before anything is built + /// from them. A with keeps the rows the queued entry already has. + /// + /// + public static QueueEntry MoveAgain(QueueEntry queued, string temp, string target, DocumentPlugin? documents = null) + { + var tempSide = FileSide.Read(temp, documents); + var targetSide = FileSide.Read(target, documents); + if (queued.Kind == QueueEntryKind.Move && + queued.LeftFile == temp && + queued.TargetFile == target && + queued.Warning == (tempSide.Warning ?? targetSide.Warning) && + Shows(queued.LeftText, queued.LeftImage, queued.LeftDocument, tempSide) && + Shows(queued.RightText, queued.RightImage, queued.RightDocument, targetSide)) + { + return queued with + { + LeftStamp = tempSide.Stamp, + RightStamp = targetSide.Stamp + }; + } + + return QueueEntry.ForMove(queued.Key, queued.Name, queued.Solution, temp, target, tempSide, targetSide); + } + + /// + /// As , for a pending delete, whose one file is its right side. + /// + public static QueueEntry DeleteAgain(QueueEntry queued, string file, DocumentPlugin? documents = null) + { + var current = FileSide.Read(file, documents); + if (queued.Kind == QueueEntryKind.Delete && + queued.LeftFile == file && + queued.Warning == current.Warning && + Shows(queued.RightText, queued.RightImage, queued.RightDocument, current)) + { + return queued with { LeftStamp = current.Stamp }; + } + + return QueueEntry.ForDelete(queued.Key, queued.Name, queued.Solution, file, current); + } + + static bool Shows(string text, ImageFile? image, DocumentFile? document, FileSide side) => + QueueEntry.SameSide(text, image, document, SourceLanguage.NormalizeNewlines(side.Text), side.Image, side.Document); + /// /// Twice, because a verified file carries two: Sample.Test.verified.txt is the test /// Sample.Test. Exactly what TrackedMove does with the same path. diff --git a/src/DiffEngineViewer/TrackedWatch.cs b/src/DiffEngineViewer/TrackedWatch.cs index f2fc67a01..50a9b08df 100644 --- a/src/DiffEngineViewer/TrackedWatch.cs +++ b/src/DiffEngineViewer/TrackedWatch.cs @@ -177,14 +177,8 @@ void Move(QueueEntry entry, List gone, List<(QueueEntry Seen, QueueE Changed( entry, - () => QueueEntry.ForMove( - entry.Key, - entry.Name, - entry.Solution, - temp, - target, - FileSide.Read(temp, documents), - FileSide.Read(target, documents)), + // A file written again with what it held is the entry it was, with a new stamp + () => TrackedEntry.MoveAgain(entry, temp, target, documents), changed); } @@ -205,7 +199,7 @@ void Delete(QueueEntry entry, List gone, List<(QueueEntry Seen, Queu Changed( entry, - () => QueueEntry.ForDelete(entry.Key, entry.Name, entry.Solution, file, FileSide.Read(file, documents)), + () => TrackedEntry.DeleteAgain(entry, file, documents), changed); } diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index 514bba579..25e93d7b4 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -233,33 +233,8 @@ static bool SameContent(QueueEntry queued, QueueEntry arrived) => queued.LeftHeader == arrived.LeftHeader && queued.RightHeader == arrived.RightHeader && queued.Warning == arrived.Warning && - SameSide(queued.LeftText, queued.LeftImage, queued.LeftDocument, arrived.LeftText, arrived.LeftImage, arrived.LeftDocument) && - SameSide(queued.RightText, queued.RightImage, queued.RightDocument, arrived.RightText, arrived.RightImage, arrived.RightDocument); - - static bool SameSide( - string text, - ImageFile? image, - DocumentFile? document, - string arrivedText, - ImageFile? arrivedImage, - DocumentFile? arrivedDocument) - { - if (document is { } held && - arrivedDocument is { } sent) - { - // By its bytes. Its text follows from them, and is read after the entry arrives, so - // one of the two may hold it while the other is still waiting for it. - return held.Path == sent.Path && - held.Format == sent.Format && - held.Length == sent.Length && - held.Hash == sent.Hash; - } - - return document is null && - arrivedDocument is null && - image == arrivedImage && - text == arrivedText; - } + QueueEntry.SameSide(queued.LeftText, queued.LeftImage, queued.LeftDocument, arrived.LeftText, arrived.LeftImage, arrived.LeftDocument) && + QueueEntry.SameSide(queued.RightText, queued.RightImage, queued.RightDocument, arrived.RightText, arrived.RightImage, arrived.RightDocument); /// /// Replaces the queue with what its owner reports, for a viewer that is displaying rather @@ -279,7 +254,7 @@ public static SessionState Sync( string? message, AcceptProgress? progress = null) { - var entries = new List(Project(state, pending)); + var entries = Project(state, pending); entries.AddRange(changes); var queue = QueueProjection.Order(entries); @@ -1632,16 +1607,21 @@ static IEnumerable Tracked(SessionState state) => state.Queue.Where(_ => _.Kind is QueueEntryKind.Move or QueueEntryKind.Delete); /// - /// And back onto the display list, in display order. Building an entry runs the diff, so an - /// entry already built for the same variants is reused, keeping its selected variant, and - /// only its status carried across. + /// And back onto the display list. Building an entry runs the diff, so an entry already built + /// for the same variants is reused, keeping its selected variant, and only its status carried + /// across. + /// + /// In the queue's order, not display order: both callers put the tracked files beside these + /// and order the whole list, and ordering here as well was the same work twice for every + /// change to the queue, under the lock the render loop takes. + /// /// /// Compared by value rather than by reference, because an attached viewer parses fresh patch /// instances out of every refresh and would otherwise re-diff the whole queue five times a /// second. /// /// - static IReadOnlyList Project(SessionState state, InlineQueue queue) + static List Project(SessionState state, InlineQueue queue) { var existing = state.Queue .Where(_ => _.Kind == QueueEntryKind.Inline) @@ -1666,7 +1646,7 @@ static IReadOnlyList Project(SessionState state, InlineQueue queue) entries.Add(QueueEntry.ForInline(pending)); } - return QueueProjection.Order(entries); + return entries; } static bool VariantsMatch(IReadOnlyList left, IReadOnlyList right) diff --git a/todo.md b/todo.md index 5b96d1d1e..5c44894a3 100644 --- a/todo.md +++ b/todo.md @@ -2,7 +2,7 @@ Findings from a review of `main` at 991bc480 (2026-10-03). The list from the review at 4244ebe6 is closed, so this one weights what has landed since: documents and maps, the text diff, zoom and pan, the remembered window and views, and pictures decoded off the UI thread. -Most of it came from six reviews run alongside, one per area: the library, the inline patcher, the tray, and the Windows, Linux and macOS heads. Every bug found is fixed and gone from this list: the five in the viewer's model, and the twenty seven the six reviews found. So is every performance item, all fourteen. What is left is what those fixes did not reach, and the smaller ones. +Most of it came from six reviews run alongside, one per area: the library, the inline patcher, the tray, and the Windows, Linux and macOS heads. Every bug found is fixed and gone from this list: the five in the viewer's model, and the twenty seven the six reviews found. So is every performance item, all fourteen, and every smaller item but one. What is left is what those fixes did not reach. File and line references are as of a01dfc1d, before the second round of fixes and before the performance ones. The code they point at is unchanged, but lines below an edit have moved, so go by the names. @@ -58,7 +58,7 @@ Nothing that was found as a bug is open. What follows is what the fixes left. - Ligatures: `!= <= => -> == ...` in a text file are each drawn as separate characters. - [ ] A live resize still draws the rows sliced for the old size until the mouse comes up. That half of the tracking loop bug needs a frame callback in the C ABI. A press in the scroller's slot followed by a drag is still AppKit's loop too. (left by the fix) - [ ] The managed side still slices the body for a footer of one row. This head has 64 pt to spare, which is three rows of buttons or two and a status line; past that the last one or two body rows are not drawn. (left by the fix) -- [ ] Every auto-repeat of a held `a` or `d` is now handed over, including ones queued during a stall. `event.isARepeat` would drop them. The Windows item under Smaller is the same hazard. (left by the fix) +- [ ] Every auto-repeat of a held `a` or `d` is now handed over, including ones queued during a stall. `event.isARepeat` would drop them. The Windows head now drops them for the commands that change the queue. (left by the fix) - [ ] The title row has the footer's old shape: the subtitle is drawn over a long title. (left by the fix) @@ -73,7 +73,7 @@ Nothing that was found as a performance item is open. Each was measured before a - [ ] A screen is built when the state changes, which a scroll or a drag does every frame, and `QueueProjection.Rows` describes every row of the queue to draw the forty that fit: 0.55 ms and 1.1 MB at 2,000 entries, for each such frame. Slicing before describing would make it the visible rows'. (left by the fix) - [ ] `ScreenPayload` clips a row to the window's width in cells rather than the pane's, so about twice what a pane can show is encoded for the macOS and Linux heads: 3.6 ms a changed frame for a 4K window of 300 character CJK lines. `RowText.Shown`, which the WinForms head now cuts with, would serve, once the model knows how many cells a pane has. (left by the fix) - [ ] A queue of more than a hundred pending files is looked at a hundred a pass, so a row that is not on screen follows its file within `count / 100` passes: two seconds for a thousand, and five times that while the window is hidden. The entry on screen is still looked at every pass. (left by the fix) -- [ ] Only a batch's record step stopped rebuilding the whole list from the whole queue. Every arrival (`EnqueueInline`), settle and single accept still does, under the lock: a dictionary of the queue, two orderings and a key an entry. The two `Smaller` items on `QueueProjection.Order` and `PendingInline.Key` are parts of it. (left by the fix) +- [ ] Only a batch's record step stopped rebuilding the whole list from the whole queue. Every arrival (`EnqueueInline`), settle and single accept still does, under the lock: a dictionary of the queue, two orderings and a key an entry. The order is now worked out once a change rather than twice, and a key once an entry. (left by the fix) - [ ] The bulk discards are still one transition on the render thread: `DiscardGroup` and `DiscardAll` delete each received file under the lock. A discard waits on nothing, so they were left. (left by the fix) - [ ] A snapshot discarded, or settled by a test that started passing, while its own source file is being written by a bulk accept was handed over with the rest of the file and is written with them. It is not counted, and a discard still takes it out of the queue. Before, that moment was the snapshot's own apply rather than its file's. Closing it would take the applier asking, before its one write, which of the patches are still wanted. (left by the fix) - [ ] Both sides of a document are drawn at once, and four things about that are as they are for a reason and could be better: a drawing is not stopped when the reader leaves its entry, though between two pages of a PDF it now could be; the pages of a PDF that is put back because the other side stopped inside PDFium are dropped, and drawn again once PDFium is free; a PDF pair's right side waits for the left's first page, which is what lets the two be told apart when both stop; and `Withdrawn`, which takes a rendering back out of the state, lives in `DocumentWatch` where it belongs beside `ViewerSession.Rendered`. (left by the fix) @@ -124,60 +124,63 @@ Nothing that was found as a performance item is open. Each was measured before a ## Smaller +One of the smaller items is open: the macOS viewer opened with no console session. The rest are fixed and gone from this list. What follows is that item and what the fixes left. + ### Viewer model -- [ ] A pair sent again unchanged is still read and diffed before it is found to be unchanged: `MessageHandler.TrackMove` builds the whole entry, and `ViewerSession.EnqueueTracked` compares after. The reader is no longer moved, but a large pair pays the diff on every run. Fix: read the two sides, compare them with the queued entry's, and build an entry only when they differ. (read) -- [ ] `QueueProjection.Order` runs twice per transition: `Project` orders (`ViewerSession.cs:1530`) and `Rebuild` (`:1490`) and `Sync` (`:279`) order its result again. (read) -- [ ] Opening a context menu builds each side's whole text to ask whether it is empty: `SelectionText.All(entry, side).Length > 0` in `src/DiffEngineViewer/MenuState.cs:98` and `:117`. Megabytes per right-click on a large file. (read) -- [ ] `FileSide.ReadBytes` copies every file twice, through a growing `MemoryStream` and then `ToArray` (`src/DiffEngineViewer/FileSide.cs:105`). The length is known. (read) +- [ ] The two tests that show a pair is not diffed again, and the one that bounds what reading a file allocates, were not run against the code from before the fix. (left by the fix) +- [ ] `TrackedWatch` still closes an open context menu when a file is written again with what it held, since the entry is replaced by its restamped copy through `ViewerSession.Refresh`. The reader is not moved. (left by the fix) ### Library -- [ ] Linux: an exported `COLUMNS` truncates every command line `ProcessCleanup` reads. `LinuxOsxProcess.cs:92` runs `ps -o pid,command -x`, and procps lets `COLUMNS` override the unlimited width it uses when stdout is not a terminal, so with `COLUMNS=80` a running tool is never detected or killed. Fix: add `-ww`, which procps and Apple's `ps` both accept. (reported: read, against the procps source) -- [ ] A `SendAsync` the caller cancelled is recorded as "port unowned": `ViewerClient.cs:372-384`, `:433-444`. `token.Register(() => Abort(client))` closes the client, the exception that follows is not an `OperationCanceledException`, and `Found(endpointPort, false)` silences settles and moves against a live owner for ten minutes. Not reachable from Verify today, which passes no token. Fix: `cancel.ThrowIfCancellationRequested()` on entry, and no `Found(false)` when the token is cancelled. (reported: plausible) -- [ ] With an owner present, every passing inline verification is a TCP connection that leaves a port in TIME_WAIT for two minutes (`DiffRunner_Inline.cs:136-145`, `ViewerClient.cs:276-292`): 0.284 ms each, but about 16,000 settles in two minutes across test processes exhaust the dynamic range, and the failed connect is then remembered as unowned. Fix, only if suites that size matter: list once and skip settles while the owner holds nothing for this framework, or keep one connection open. (reported: measured) -- [ ] `ViewerServer.Listen` (`ViewerServer.cs:89-97`) has `catch (SocketException) { continue; }` with no delay, so an accept failure that persisted would spin a core. Whether one can persist was not established. (reported: plausible) +- [ ] Kept connections were not run on macOS: neither the rebind of a port whose last owner left a client holding a connection, nor the receive timeout surfacing as `SocketError.TimedOut`. And they were tested against a stand-in for an older owner, not a released tray or viewer. (left by the fix) +- [ ] A client talking to an owner from before `keeps` still makes a connection per settle, so the port exhaustion stands against an older tray. (left by the fix) +- [ ] The async inline send and every asking send (`TrySend` with a response, `InlineQueueClient`, `OwnerLink`'s five a second listing) are still a connection each. (left by the fix) +- [ ] A connect that fails because the machine has no ports left is still recorded as an unowned port. Which `SocketError` Windows gives there was not established. (left by the fix) +- [ ] Parallel settles take turns at the one connection under a lock, so a slow owner now holds them in a line, where each used to wait on a connection of its own. (left by the fix) +- [ ] A test process holds a socket to the queue's owner for its whole life. On .NET Framework sockets are inheritable, so a child a test starts without ShellExecute can hold that connection open after the test host exits. Not run. (left by the fix) +- [ ] The accept wait is a fixed 100 ms from the second failure in a row. The case where a failure persists was run on Linux only, by using up the descriptors. (left by the fix) +- [ ] Apple's `ps` was not run with `-ww`; its manual says a second `-w` uses as many columns as it needs. (left by the fix) ### Inline snapshots -- [ ] A Remove applied twice can take a sibling's literal: `InlinePatcher.cs:652-656`. With `await Verify(a)` over `.Snapshot("dup");` and `await Verify(b).Snapshot("dup");` under it, the first apply pulls the next line up, and the second, from another framework or another case of an `IgnoreParameters` test, finds a Snapshot call on the hint line, so `RemovedAtHint` is false and `Verify(b)` loses its literal. Fix: do not pull the following text up, or have Verify skip the Remove when the source file is newer than the test assembly. (reported: read) -- [ ] The F# lexer disagrees with the compiler inside block comments: `FsLanguage.cs:183-229`. `(* returns "*)" when closed *)`, `(* see "(*" *)` and `(* the (*) operator *)` are each one comment to F#, and the scanner closes or nests on what is inside the string. The usual result is NotFound for calls below. Fix: inside a comment step over string literals and `(*)`, and step over a double backticked identifier whole. (reported: read; what F# does was run under `dotnet fsi`) -- [ ] An F# snapshot whose value is the empty string loses its anchor over the wire: `InlinePatchFile.Build` writes `OriginalValue == ""` as an empty field (`:53-55`), and `TryParse` reads an empty field back as null (`:148-152`). The viewer then heads the pane "expected (new snapshot)", and the patcher falls back to the hint alone. Fix: write a marker for "present and empty", or a separate line saying a value is present. (read) -- [ ] Keys are recomputed per comparison: `PendingInline.Key` lowercases the path and formats a string on every `FindIndex` step (`PendingInline.cs:70`, `InlineQueue.cs:48,244,453,565`, `InlineStaging.cs:366-367`). A few hundred milliseconds across a run with hundreds pending. Fix: compute the key once per entry. (reported: read) +- [ ] Verify's half of the Remove applied twice is not done: skipping the Remove when the source file is newer than the test assembly. DiffEngine's half keeps the removed call's line, empty, when a Snapshot call is under it, which is a rule about the next line and not a guarantee about line numbers. (left by the fix) +- [ ] A `Remove` that takes a whole statement line (`settings.Snapshot("dup");`) still brings the next line up, so applied twice over a sibling `other.Snapshot("dup");` the second apply takes the sibling's statement. `RemovedAtHint` only recognises a removal under a verify call. (left by the fix: read, not run) +- [ ] An empty `TestName` or `MemberName` still reads back as null from `InlinePatchFile`; only `OriginalValue` gained a line saying it is present and empty. (left by the fix) +- [ ] The F# scanner outside a comment still takes a tick after an identifier character as part of the name, and still reads the holes of `$"..."` its own way. Only comments and backticked names were held to `dotnet fsi`. (left by the fix) ### Tray -- [ ] `Process` objects are never disposed for moves that cannot be killed: `Tracker.cs:799-806`, `:945-950`, `ProcessEx.cs:19-32`. `DiffRunner` sends a process id for MDI tools too, `TryGet` forces a handle open, and `KillProcesses` returns at `if (!move.CanKill)` without disposing. One handle per tracked move until a gen2 finaliser. Fix: dispose, without killing, wherever a move finally leaves the dictionary. (reported: read) -- [ ] The scan can drop the wrong move, and one unexpected exception stalls it: `Tracker.cs:66-72`, `:97-114`, `AsyncTimer.cs:25-43`. `moves.TryRemove(tacked.Temp, out var removed)` removes by key, so a re-run that replaced the move between the scan's check and the removal loses its fresh entry and has its tool killed. Only `IOException` is caught around `FilesAreEqual`, and the handler shows a `MessageBox` on the timer thread. Fix: remove by key and value, and catch `UnauthorizedAccessException` beside it. (reported: plausible) -- [ ] "Discard (n)" waits up to 15 s on the UI thread when a viewer owns the queue: `Tracker.cs:836-851` calls `inline.DiscardAll(out var message)` inline, where `Discard` was moved to a worker for this reason. (reported: read) -- [ ] An exception thrown by a hot key action ends the tray: `HotKey/KeyRegister.cs:70-92`. One thrown from `IMessageFilter.PreFilterMessage` comes out of `Application.Run()` rather than reaching `Application.ThreadException`. Fix: try and catch around `action()`, and a catch in `LinkLauncher`. (reported: ran for the mechanism; no trigger found) -- [ ] The process id in a piper payload is trusted as the diff tool: `Tracker.cs:179-183`, `:205-211`. Libraries from before the `ProcessCleanup.StillRunning` fix can send a reused id, and the tray kills whatever holds it now on accept. Fix: compare the process image against the payload's `Exe` first. (reported: plausible) -- [ ] `ListingTag`'s `Fingerprint` (`OwnedInlineHost.cs:317-331`) rebuilds and hashes every tracked move per poll, about 0.5 MB of garbage five times a second while a viewer is attached. (reported: read) +- [ ] A process id is matched to the payload's `Exe` by file name only, so a reused id now held by another copy of the same tool is still tracked, and killed on accept if the move can be killed. (left by the fix) +- [ ] A tool started through a `.cmd` (VS Code, Rider) is never tracked as a process, so it never counts for "Accept all open" and is never closed by the tray. Before, the id held for it was the command interpreter's. Nor is a tool whose launcher hands over to an executable of another name and exits. The tool definitions were not surveyed for either. (left by the fix) +- [ ] "Discard (n)" still discards tracked moves on the UI thread, with up to 500 ms per killable tool waiting for it to exit. Only the queue's half moved to a worker, and a bulk discard that fails is only logged. (left by the fix) +- [ ] A scan that keeps failing is now only in the log. Nothing tells the user. (left by the fix) +- [ ] `ListingTag` can answer "unchanged" for a listing built while a move was briefly out of the dictionary during an accept and then restored. The fingerprint it replaced had the same gap. (left by the fix) +- [ ] `Tracker.differing` is never pruned when a move leaves. (left by the fix: noticed, not touched) +- [ ] `KeyRegisterTests` registers a real global hot key, Ctrl+Alt+Shift+F24, for the length of each test, and fails where something else holds it. (left by the fix) ### Viewer, Windows head -- [ ] Holding the scroll bar's arrow or its trough freezes the panes until release: `ViewerForm.cs:176-191` enters the modal frame only for `ThumbTrack`, and user32 tracks every part of the bar in the same loop. Fix: `EnterModal()` for any type other than `EndScroll` and `ThumbPosition`. (reported: ran) -- [ ] Alt chords fall through to the plain key commands: `ViewerForm.Map` (`:703-770`) gives Alt+A accept, Alt+D discard and Alt+Q quit. The same leak was closed for Control. Fix: return `CommandKind.None` when `Keys.Alt` is held, ahead of the Control branch. (reported: ran) -- [ ] Accept and Discard auto-repeat, and the repeats are queued: `ViewerForm.cs:703-721`, `:96-105`. Holding `a` past the repeat delay accepts entries the reader has not seen. Fix: drop repeats (bit 30 of `LParam`) for the commands `ViewerSession.ChangesQueue` names. (reported: plausible) -- [ ] The status label shows the middle of a status that does not fit: `ViewerForm.cs:21-32`, `:582-607`. A pdf in a queue leaves it 151 px, and a status of 344 px wraps to three lines in a 30 px label centred vertically. Fix: `AutoEllipsis = true`, with an alignment that keeps the start. (reported: measured) -- [ ] A minimised window still runs the loop at sixty frames a second: `FormsViewerWindow.cs:92` tests `form.Visible`, which stays true when minimised. Fix: `form.Visible && form.WindowState != FormWindowState.Minimized`. (reported: plausible) +- [ ] A minimised window slows only its own frame wait. `OwnerLink`, `TrackedWatch` and the document reader go by `Hidden`, which only a Hide command sets, so they keep their on screen cadence while minimised. The head would have to tell the loop. (left by the fix) +- [ ] A status that does not fit still loses what is past two lines; it now ends in an ellipsis and shows whole in a tooltip. The footer does not wrap its buttons or give the status a line of its own as the other two heads do. Whether a scaled display shows the middle of a wrapped status, which the item claimed and unscaled captures did not show, is unchecked. (left by the fix) +- [ ] Nothing proves by throwing that an exception comes out of a message pump in the test hosts rather than WinForms' dialog. The test reads the mode back instead, through an internal of WinForms, because the throwing one is what put the dialog on screen. (left by the fix) +- [ ] `TwoKeysInOnePumpAreTwoCommands`, `ATextEntryHasNoPictureForTheWheelToFind` and `TheWheelOverAPictureZoomsAndOverTheRowsScrolls` failed once in a run of the whole solution and passed in the next and twice alone. They post keys and wheel turns to a form, and someone was using the machine. (seen once) +- [ ] Footer buttons leave `UseMnemonic` on, so an ampersand in a label would become an Alt access key. (left by the fix: noticed, not looked into) ### Viewer, Linux head -- [ ] Input is sampled as state once a frame, so a press and release that arrive together are not seen and several wheel events collapse into one: `deview.cpp:909-923`, `:2217-2220`. Sent together by xdotool under Xvfb, none of ten clicks was seen and three of ten key presses were. A touchpad two finger tap would not open a context menu, which a tap on a real touchpad would confirm. Fix: chain GLFW's mouse button and scroll callbacks and feed ImGui from them. (reported: plausible, and since run in the container) -- [ ] Ctrl+A and Ctrl+C are still by US key position, arrows and paging do not repeat when held, and no letter shortcut matches on a non-Latin layout: `deview.cpp:929-944`, `:976-985`. Fix: `IsKeyPressedRepeat` for navigation, and resolve the chords through `GetKeyName`. (reported: read) -- [ ] No display scale handling: no `FLAG_WINDOW_HIGHDPI` and no `GetWindowScaleDPI()`, so on a HiDPI X11 display everything is about half size (`deview.cpp:2075-2091`, `:2137`). What would settle it: a display with `Xft.dpi` 192. (reported: plausible) -- [ ] A queue row's context menu is not kept inside the window: the clamp at `deview.cpp:1929-1944` is inside `if (paneMenu)`, so the last row's menu is cut off at the default size. (reported: read) -- [ ] Hover never ends when the pointer leaves the window: `io.AddMousePosEvent(mouse.x, mouse.y)` is unconditional (`deview.cpp:915-916`), so a row stays highlighted and its tooltip appears with the pointer elsewhere. (reported: plausible) -- [ ] A picture larger than `GL_MAX_TEXTURE_SIZE` draws as a black box rather than as nothing (`deview.cpp:613-621`, `:706-712`), and pictures shrunk more than two times are sampled bilinear with no mipmaps (`:464-469`, `:1457-1464`), so thin lines and small text drop out. (reported: plausible, and read) -- [ ] Labels containing `##` are cut short, since ImGui hides everything from there on: pane headers, queue rows, menu items and buttons (`deview.cpp:1687-1688`, `:1716`, `:1738`, `:1961`, `:2003`). (reported: read) +- [ ] A picture larger than `GL_MAX_TEXTURE_SIZE` now draws as nothing. It could be reduced on the decoder thread to fit a texture. `PixelTests.ImageTooLargeForATexture` holds only under llvmpipe, whose limit is 16384. (left by the fix) +- [ ] A held letter repeats its command at the keyboard's repeat rate, so holding `a` accepts repeatedly. Only navigation should repeat. The Windows head now drops those repeats. (left by the fix) +- [ ] Display scale is read once, as the window is made. A change of `Xft.dpi` while running, and a scale per monitor under Wayland or XWayland, are not followed, and a placement remembered at one scale is used in pixels at another. Checked only under Xvfb with an X resource at 144 and 192, never on a HiDPI desktop. (left by the fix) +- [ ] A frame of two pictures costs about a tenth more under llvmpipe (opaque at 4K, 27.0 ms to 30.1), probably the trilinear sampling of a picture drawn under half size. Mipmaps also add about a third to a picture's memory, drawn small or not. (left by the fix: measured, cause unconfirmed) +- [ ] `[`, `]`, `0`, `-` and `=` have no fallback by position on a layout that is not Latin; only the letters do. (left by the fix) +- [ ] All input now comes through GLFW's callbacks. A release that never arrives would leave a button held: GLFW's own release on losing focus is relied on, and hiding the window during a press was not tried. A two finger tap on a real touchpad is still unconfirmed; a press and release sent together now works. (left by the fix) ### Viewer, macOS head -- [ ] A picture landing during a repaint of the spinner alone is drawn clipped to the spinner's rectangle and never completed: `Renderer.swift:245`, `Runtime.swift:223-239`. A few in a thousand large images. The scaled copy of an enlarged picture lands the same way, and one that lands then leaves the picture drawn at `.low`. Fix: have `draw` report that something landed, and turn that into a full redraw. (reported: read) -- [ ] `[` and `]` never match on layouts where they need Option, since `charactersIgnoringModifiers` yields the digit (`ViewerView.swift:412-433`): German, French, Nordic, Spanish and Italian layouts cannot turn pages by key. Fix: match symbols on `event.characters` first. (reported: read) -- [ ] A pan drag in a pane that cannot move on an axis resets that axis for the other pane: the report is clamped with the dragged pane's own extents (`Renderer.swift:164-174`, `ViewerView.swift:180-187`, and `deview.cpp:1587-1591` on Linux). Fix: report the frame's own centre unchanged on an axis the pane cannot move on. (reported: read) -- [ ] `picturesChanged` never settles when a picture has no room, so a window shorter than about 176 pt redraws at sixty frames a second (`Renderer.swift:485-489`, `:846-848`). Fix: a `contentMinSize`, or record the stamp when nothing is drawn. (reported: read) -- [ ] A notched mouse wheel may do nothing until ten slow clicks add up (`ViewerView.swift:310-322`), and control-click never opens a context menu (`:124-170`). What would settle them: a Mac with a wheel mouse. (reported: plausible) -- [ ] The pump waits out its full 16.7 ms after input, and nothing slows the loop when the window is occluded or miniaturised (`Runtime.swift:209-249`, `:257-276`). (reported: read) -- [ ] `Runtime.open` cannot fail (`Runtime.swift:57-91` always returns true), so with no console session, as over SSH, AppKit aborts the process after the port was bound and the patch is lost. What would settle it: a failing inline snapshot over SSH with nobody logged in. (reported: plausible) +- [ ] `Runtime.open` cannot fail (it always returns true), so with no console session, as over SSH, AppKit aborts the process after the port was bound and the patch is lost. `CGSessionCopyCurrentDictionary()` is the documented question to ask, and was not used because it may also answer "no session" over SSH while the same user is logged in at the console, where the viewer opens today. What would settle it: print that call from an SSH shell with and without a console login, and run the viewer in each. (reported: plausible) +- [ ] None of the six macOS fixes in this round has been compiled or run by a person, and five are event handling or window state that no capture exercises. The check to make on a Mac is in each commit's message. (left by the fix) +- [ ] A drag still pulls the other pane's centre into the dragged pane's range on an axis both can move on, when the two pictures differ in shape. Only the axis the dragged pane cannot move on is left alone. The Linux head has the same rule. (left by the fix) +- [ ] A hidden, miniaturised or covered window comes forward up to a tenth of a second late for a patch arriving over the socket, since the managed side cannot interrupt the pump, and `OwnerLink` and `TrackedWatch` do not slow for a window that is only covered or miniaturised. Both need the ABI to carry it. During a scroll or a drag the loop now turns at the rate events arrive, which a 120 Hz device makes faster than sixty. (left by the fix) +- [ ] `+`, `-` and `=` are still matched on `charactersIgnoringModifiers`; only the brackets moved to `characters`. (left by the fix) +- [ ] A control-click over a footer button now does nothing, as a right click does, where it used to click the button. (left by the fix)