diff --git a/claude.md b/claude.md index 9c0b1233..0e383075 100644 --- a/claude.md +++ b/claude.md @@ -273,8 +273,11 @@ the comment there about not caching "nothing staged" asks for. 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. A repeat of a held key is + A drag moves the centre the frame asked for and clamps it to the wider of the two panes' + ranges (`PictureSpace.dragged(by:other:)`), with the same arithmetic in the shim and in + WinForms (`PicturePlacement.Dragged`): clamped to the dragged pane's own range, a drag pulled + the other pane's picture into it. `grid(for:)` reports no more rows than the last window draw + left the body under a tall footer. A repeat of a held key is dropped for accept, accept all and discard. A picture both panes name keeps a scaled copy for each pane (`Picture.spare`), since the panes can be a point apart in width and one copy was made again for each in turn without end. @@ -320,7 +323,12 @@ the comment there about not caching "nothing staged" asks for. three quarters of a picture's size up, and mipmaps made only for a picture drawn smaller than that, on the decoder thread when first needed. A picture past `GL_MAX_TEXTURE_SIZE` is brought down to one that fits as it is read. `PixelTests` has two tests that show the window and count - draws through a GL query, `AWindowLeftAloneIsNotDrawn` and `AHiddenWindowIsNotDrawn`. + draws through a GL query, `AWindowLeftAloneIsNotDrawn` and `AHiddenWindowIsNotDrawn`, and a + third that shows it to ask for its rows, `ATallFooterTakesRowsFromTheBody`. A minimised window + is left alone as a hidden one is. `MeasureGrid` reports no more rows than the body had room for + in the last window frame plus the model's eight chrome lines (`chromeRows`, kept in step with + `ScreenBuilder.Chrome` here and in `Renderer.swift`), so a footer taller than its allowance + takes rows from the body and not from under it. - 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 @@ -595,7 +603,11 @@ the comment there about not caching "nothing staged" asks for. refuses a library whose version is not an exact match, so a bump and a binaries rebuild land together: change `native/`, run `build-native`, merge the PR it opens. Between the two, the `native` CI job — the one that loads the committed binaries — reports the mismatch, which is the - check working. + check working. Version 12 is two things: `DeviewInput.rows` capped by the body's room, and + `DeviewInput.unseen`, a state like the grid and not an event, which is a head saying nobody can + see its window (hidden or minimised on Linux; ordered out, miniaturised or occluded on macOS; + minimised on Windows, through `ViewerInput.Unseen`). `ViewerProgram.Loop` keeps its own `hidden` + and the head's `unseen` apart and slows `OwnerLink`, `TrackedWatch` and `DocumentWatch` on either. - Built binaries are **committed** to `src/DiffEngineViewer.{Linux,Mac}/runtimes/{rid}/native/`, so a plain `dotnet build` produces a shippable package and contributors never need CMake. Regenerate them with the `build-native` GitHub workflow, which opens a PR. @@ -703,12 +715,21 @@ the comment there about not caching "nothing staged" asks for. - A move that writes a file marks the delete pending on it (`TrackedDelete.Written`), however the move was accepted, and no accept-all carries a marked delete out until a run raises it again; accepting it on its own still does. `Tracker.HeldReason` is what the menu and the debug - view show. "Discard (n)" runs wholly on a worker. A scan that fails three times running is one + view show, and it rides a full listing as a `held: key|reason` line of its own + (`ViewerResponseDelete.Held`), since the `delete` line is parsed by field count and an older + reader skips a line it does not know. An attached viewer shows it as the entry's status and + leaves held deletes out of "Accept all in" a group (`OwnerLink.AcceptGroup`), and the wire's + accept-all answer ends with `Tracker.DeletesKept`. An owning viewer goes by the same rules for + its own files: a move arriving withdraws the delete on its target (`EnqueueTracked`), a move + carried out marks it (`QueueEntry.Written`), a batch keeps a marked or awaited delete + (`ViewerSession.HeldReason`), a single accept still deletes, and `TrackedEntry.DeleteAgain` + carries the mark across the watch's re-read. "Discard (n)" runs wholly on a worker. A scan that fails three times running is one balloon for the run. Every tray keeps the `SessionEndWindow` now, and `Program.SessionEnding` stages the queue where it is held here and then removes the version marker. - `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 + listing carries. `TrackedDelete.Written` is the exception, and its changes are counted into the + version as the restores are. 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()`. diff --git a/native/include/deview.h b/native/include/deview.h index 7cd46d94..a9c6a0b7 100644 --- a/native/include/deview.h +++ b/native/include/deview.h @@ -140,7 +140,8 @@ typedef struct DeviewPane { * * The managed side does not know how many pixels a pane has, so it may ask for a centre that * would leave part of the space empty. A renderer moves the centre in as far as it takes to - * keep the space full, and that clamped centre is the one a drag starts from. + * keep the space full. That is for drawing only. A drag starts from the centre asked for, + * moved in only as far as the pane that can go further would move it: see DeviewInput.panX. */ float imageZoom; float imageCenterX; @@ -294,6 +295,15 @@ typedef struct DeviewInput { * The window size in character cells, not pixels. Measured here from the font that was * actually loaded, because this side is the only one that knows it. Reporting pixels and * having the managed side divide by a constant is what left the viewer with no DPI handling. + * + * rows is the window's height in rows, and no more than the body has room for with the eight + * lines the managed side keeps for everything that is not a row (ScreenBuilder.Chrome) added + * back. Those eight lines are more than a title, the headers and a footer of one row take, and + * a footer that fits in what is over costs nothing. One that does not - buttons that wrap onto + * a third and fourth row in a narrow window - is laid out over the bottom of the body, so the + * rows it covers are taken off here, and the managed side slices a body that ends above it. + * Counted from the last frame laid out for the window, the only place a footer's height is + * known. */ int32_t columns; int32_t rows; @@ -327,8 +337,11 @@ typedef struct DeviewInput { int32_t zoomDelta; /* - * 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. + * Where a drag has left an enlarged picture: the centre both panes are to draw about, as + * fractions of the picture's width and height. It is the centre the frame asked for that the + * drag moves, and it is kept inside what the pane showing less of its picture can show: the + * two pictures need not be the same shape, and kept to the dragged pane's own range a drag + * brought the other pane's picture in from wherever beyond it that one had been taken. * 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. @@ -346,6 +359,19 @@ typedef struct DeviewInput { * says which pane it is for in DeviewScreen.menuPane. */ int32_t rightClickedPane; + + /* + * 1 while nobody can see the window, and 0 otherwise: minimised, hidden by deview_set_hidden, + * or with nothing of it showing where the window system can say so, which AppKit can and X11 + * cannot. A state, as the grid is, and not an event: reported by every poll for as long as + * it is so. + * + * The managed side keeps things going beside the window for whoever is reading it - it reads + * the files behind the rows again, lists the queue's owner, draws a document's pages - and + * knew to slow those only for a window it had hidden itself. A window in the taskbar or the + * Dock, or behind another, was kept up as one being read. + */ + int32_t unseen; } DeviewInput; /* @@ -402,8 +428,14 @@ typedef struct DeviewPlacement { * widened array element again, in the same bump because they shipped together. * And DeviewInput reports a right-click over a pane, which DeviewScreen answers with a menu * that names the pane rather than a queue row. + * 12: DeviewInput.rows is fewer than the window's height in rows where the footer is taller than + * the managed side allows for, by the rows of the body that footer covers. No struct moved: + * what a field means did, and a library from before reports rows that a tall footer hides. + * DeviewInput also reports a window nobody can see, at its end, in the same bump because + * they shipped together. And panX and panY are kept inside what either pane can show, where + * they were kept inside what the dragged one can. */ -#define DEVIEW_VERSION 11 +#define DEVIEW_VERSION 12 /* * The Swift implementation imports this header for the struct layouts, because Swift does not diff --git a/native/src/deview.cpp b/native/src/deview.cpp index 2011c588..f43a70d6 100644 --- a/native/src/deview.cpp +++ b/native/src/deview.cpp @@ -148,6 +148,12 @@ constexpr float minPaneCells = 12.0f; */ constexpr float grabWidth = 4.0f; +/* + * The lines the managed side keeps for everything that is not a row of the body, which it takes + * off the rows it is told the window has. Keep in sync with ScreenBuilder.Chrome. + */ +constexpr int chromeRows = 8; + /* * deview_init's fontSize is an em size, which is what Core Text and GDI+ take and therefore what * the other two heads render at. ImGui's stb_truetype loader scales by pixel height instead @@ -355,6 +361,10 @@ struct State float cellWidth = 0.0f; float lineHeight = 0.0f; + /* How many rows the body had room for in the last frame built for the window, above whatever + * footer that frame laid out, or -1 before there has been one: see MeasureGrid. */ + int32_t bodyRows = -1; + /* 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; @@ -517,6 +527,11 @@ struct State float across = 1.0f; float down = 1.0f; + /* The centre the frame asked for, before it was moved in to keep this space full: one + * point for both panes, so it can be somewhere this pane cannot show and the other can. */ + float askedX = 0.5f; + float askedY = 0.5f; + /* 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; @@ -528,12 +543,15 @@ struct State /* * An enlarged picture being dragged: where the button went down, and how the picture was placed * then, which the whole drag is measured from. Measured from the last frame instead, a drag - * would drift by whatever each frame's clamp took off it. + * would drift by whatever each frame's clamp took off it. And how the other pane's was placed + * then, since how far that one can go is part of how far the drag can take the centre the two + * share. */ bool panning = false; int32_t panSide = 0; ImVec2 panStart{}; PictureSpace panFrom{}; + PictureSpace panOther{}; /* * Where the right-click that asked for a pane's menu landed, which is where the menu hangs: the @@ -2606,6 +2624,14 @@ int ReadKey(bool& escape) * * A row is one text line plus the spacing between rows, which is what the table the panes are * drawn in lays out on. + * + * And no more rows than the body has room for, with the lines the managed side takes off for + * everything else added back. That side keeps eight lines where this head's title, headers and one + * line of footer take under five, so a footer of two or three lines fits in what is over and the + * window's height in rows is the answer, as it always was. A taller one does not: a paged + * document's buttons come to four rows in a window under 450 pixels wide, and the body is given + * what the footer leaves, so the last rows the managed side sliced were under it. They are taken + * off here instead, counted from where the last frame put the body's first row and its bottom. */ void MeasureGrid() { @@ -2622,6 +2648,10 @@ void MeasureGrid() state.input.rows = height > 0.0f ? static_cast(static_cast(GetScreenHeight()) / height) : 0; + if (state.bodyRows >= 0) + { + state.input.rows = std::min(state.input.rows, state.bodyRows + chromeRows); + } } /* ---- the frame ---- */ @@ -3153,6 +3183,8 @@ void DrawPaneImage(const DeviewScreen* screen, const DeviewPane& pane, const Pan space.centreY = centreY; space.across = across; space.down = down; + space.askedX = pane.imageCenterX; + space.askedY = pane.imageCenterY; space.movesAcross = std::floor(whole.x) > size.x; space.movesDown = std::floor(whole.y) > size.y; } @@ -3201,6 +3233,12 @@ bool OverPicture(float x, float y) return false; } +/* A centre moved in as far as it takes for a space showing so much of its picture to stay full. */ +float KeptCentre(float centre, float shown) +{ + return std::min(std::max(centre, shown * 0.5f), 1.0f - shown * 0.5f); +} + /* * An enlarged picture taken hold of and moved, reduced to the centre the managed side takes. * @@ -3240,6 +3278,7 @@ bool UpdatePan(const DeviewScreen* screen) state.panSide = side; state.panStart = mouse; state.panFrom = space; + state.panOther = state.pictureSpaces[1 - side]; break; } } @@ -3257,8 +3296,7 @@ bool UpdatePan(const DeviewScreen* screen) } /* - * The picture follows the pointer, so the point at the middle moves the other way, as far as - * this pane's picture can go. + * The picture follows the pointer, so the point at the middle moves the other way. * * 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 @@ -3266,16 +3304,23 @@ bool UpdatePan(const DeviewScreen* screen) * 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. + * + * And on an axis both can move on, as far as the one that can go further. What is moved is + * the centre the frame asked for, not the one this pane drew about, and it is kept inside what + * the pane that shows less of its picture can show. Moved from this pane's own and kept to + * this pane's range, the first move of a drag brought the other pane's picture in from + * wherever beyond that range it had been dragged to. */ const State::PictureSpace& from = state.panFrom; + const State::PictureSpace& other = state.panOther; 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; + const float across = other.enlarged && other.movesAcross ? std::min(from.across, other.across) : from.across; + const float down = other.enlarged && other.movesDown ? std::min(from.down, other.down) : from.down; state.input.panX = from.movesAcross - ? std::min(std::max(x, from.across * 0.5f), 1.0f - from.across * 0.5f) + ? KeptCentre(KeptCentre(from.askedX, across) - (mouse.x - state.panStart.x) / from.wholeWidth, across) : pane.imageCenterX; state.input.panY = from.movesDown - ? std::min(std::max(y, from.down * 0.5f), 1.0f - from.down * 0.5f) + ? KeptCentre(KeptCentre(from.askedY, down) - (mouse.y - state.panStart.y) / from.wholeHeight, down) : pane.imageCenterY; if (!ImGui::IsMouseDown(ImGuiMouseButton_Left)) @@ -3530,13 +3575,12 @@ void BuildFrame(const DeviewScreen* screen) ImGui::Separator(); /* - * Its height comes off the body, and the managed side is not asked for fewer rows to make up - * for it. That side keeps eight lines for everything that is not a row, where this head's - * title, headers and one line of footer take under five, so there are 62 pixels and more under - * the last row it slices. A second row of buttons takes 23 of them and a line for the status - * 17, and a third row of buttons on top of both is a pixel over at most. It is only past - * that - four rows, which a paged document's buttons come to in a window under 450 pixels - * wide - that the last rows of the body are cut off, behind a footer that can at least be read. + * Its height comes off the body. The managed side keeps eight lines for everything that is not + * a row, where this head's title, headers and one line of footer take under five, so there are + * 62 pixels and more under the last row it slices. A second row of buttons takes 23 of them + * and a line for the status 17, and a third row of buttons on top of both is a pixel over at + * most. Past that - four rows, which a paged document's buttons come to in a window under 450 + * pixels wide - the managed side is asked for fewer rows: see MeasureGrid. */ const Footer footer = LayOutFooter(screen, ImGui::GetContentRegionAvail().x); @@ -3738,6 +3782,17 @@ void BuildFrame(const DeviewScreen* screen) ImGui::EndTable(); } + if (!state.capturing) + { + /* What MeasureGrid holds the rows it reports to: how many of them there is room for + * between the body's first and its bottom, which is where the footer begins. Unknown for + * a frame with no rows, which says nothing about where one would be. */ + const float pitch = leftHit.pitch > 0.0f ? leftHit.pitch : ImGui::GetTextLineHeightWithSpacing(); + state.bodyRows = leftHit.first >= 0.0f && pitch > 0.0f + ? std::max(0, static_cast((bodyMin.y + bodyAvail.y - leftHit.first) / pitch)) + : -1; + } + if (screen->paneCount >= 2) { const float bottom = bodyMin.y + bodyAvail.y; @@ -4489,10 +4544,14 @@ int32_t deview_present(const DeviewScreen* screen) * arrival, and marks the window stale, so the first present after it builds the screen it is * handed and draws it: see deview_set_hidden and Arrived. * + * A minimised window the same, which was still built and drawn whenever its screen changed. + * Nothing of it is on the screen either, and coming back from the taskbar is an arrival that + * marks it stale as being shown is. + * * Not before a frame has been built for the window at all. The grid the managed side slices * its rows by is measured from one, and ImGui has no font to measure with until its first. */ - if (state.hidden && + if ((state.hidden || state.minimised) && state.cellWidth > 0.0f) { if (changed) @@ -4637,6 +4696,11 @@ void deview_poll_input(DeviewInput* input) } MeasureGrid(); + + /* As it is now, like the grid. Nothing of a window that is hidden or minimised is on the + * screen. One behind another window is not known to be: X11 says nothing of it that GLFW + * passes on. */ + state.input.unseen = IsWindowState(FLAG_WINDOW_HIDDEN) || IsWindowMinimized() ? 1 : 0; } *input = state.input; diff --git a/native/swift/Sources/Deview/Renderer.swift b/native/swift/Sources/Deview/Renderer.swift index d6e49e26..53be6a0d 100644 --- a/native/swift/Sources/Deview/Renderer.swift +++ b/native/swift/Sources/Deview/Renderer.swift @@ -43,6 +43,14 @@ final class Renderer { /// line lands in the same column in both. private static let gutterCells: CGFloat = 8 + /// The lines the managed side keeps for everything that is not a row of the body, which it + /// takes off the rows it is told the window has. Keep in sync with ScreenBuilder.Chrome. + private static let chromeRows = 8 + + /// How many rows the body had room for in the window's last draw, above whatever footer that + /// draw laid out, and the size the window was then. Nil before there has been one: see `grid`. + private var room: (size: CGSize, rows: Int)? + private let font: CTFont private let ascent: CGFloat private let descent: CGFloat @@ -277,16 +285,41 @@ final class Renderer { /// 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 { + /// + /// And a way both can move, it goes as far as the one that can go further. What is moved + /// is the centre the frame asked for, not the one this space drew about, and it is kept + /// inside what the pane showing less of its picture can show: `other` is that pane's + /// space, or nil when it has no picture. Moved from this space's own and kept to this + /// space's range, the first move of a drag brought the other pane's picture in from + /// wherever beyond that range it had been dragged to. + func dragged(by: CGSize, other: PictureSpace?) -> CGPoint { guard enlarged, whole.width > 0, whole.height > 0 else { return centre } - let x = centre.x - by.width / whole.width - let y = centre.y + by.height / whole.height + var acrossShown = across + var downShown = down + if let other, other.enlarged { + if other.movesAcross { + acrossShown = min(acrossShown, other.across) + } + + if other.movesDown { + downShown = min(downShown, other.down) + } + } + + let x = PictureSpace.kept(PictureSpace.kept(asked.x, acrossShown) - by.width / whole.width, acrossShown) + let y = PictureSpace.kept(PictureSpace.kept(asked.y, downShown) + by.height / whole.height, downShown) return CGPoint( - x: movesAcross ? min(max(x, across / 2), 1 - across / 2) : asked.x, - y: movesDown ? min(max(y, down / 2), 1 - down / 2) : asked.y) + x: movesAcross ? x : asked.x, + y: movesDown ? y : asked.y) + } + + /// A centre moved in as far as it takes for a space showing so much of its picture to + /// stay full. + private static func kept(_ centre: CGFloat, _ shown: CGFloat) -> CGFloat { + min(max(centre, shown / 2), 1 - shown / 2) } } @@ -370,8 +403,21 @@ final class Renderer { /// The window size in character cells, which is what version 2 of the ABI reports. Net of the /// scroller, because a column the scroller is sitting on is not a column the diff can use. + /// + /// And no more rows than the body has room for, with the lines the managed side takes off for + /// everything else added back. That side keeps eight lines, which is 64 points more than this + /// head's title, headers and a footer of one row take, so a footer of three rows of buttons, + /// or two and a status line, fits in what is over and the window's height in rows is the + /// answer, as it always was. A taller one does not, and the body ends where the footer + /// begins, so the last one or two rows the managed side sliced were not drawn. They are taken + /// off here instead, counted by the window's last draw, when that was at this size. func grid(for size: CGSize) -> (columns: Int32, rows: Int32) { - (Int32(max(0, size.width - rightInset) / cell.width), Int32(size.height / cell.height)) + var rows = Int(size.height / cell.height) + if let room, room.size == size { + rows = min(rows, room.rows + Renderer.chromeRows) + } + + return (Int32(max(0, size.width - rightInset) / cell.width), Int32(rows)) } /// `capturing` decodes and scales pictures here and now, and stands a spinner still: a capture @@ -450,7 +496,14 @@ final class Renderer { // Before the body, which ends where the footer begins. The footer is as tall as its // buttons take, and that is more than one row of them once they are wider than the window. let placed = place(frame, width: size.width, line: line) - let capacity = max(1, Int((size.height - bodyTop - placed.height - Renderer.padding) / line)) + let fits = Int(max(0, size.height - bodyTop - placed.height - Renderer.padding) / line) + let capacity = max(1, fits) + // What `grid` holds the rows it reports to. Not a capture's, which is drawn at a size of + // its own and is no window. + if !capturing { + room = (size: size, rows: fits) + } + let rows = min(capacity, max(frame.queue.count, max(frame.left.rows.count, frame.right.rows.count))) for index in 0 ..< rows { diff --git a/native/swift/Sources/Deview/Runtime.swift b/native/swift/Sources/Deview/Runtime.swift index 26e1bd8e..38f03994 100644 --- a/native/swift/Sources/Deview/Runtime.swift +++ b/native/swift/Sources/Deview/Runtime.swift @@ -506,6 +506,9 @@ final class Runtime { func takeInput() -> DeviewInput { var taken = input resetInput() + // As it is now, like the grid: what slows the pump is said to the managed side as well, + // which slows what it keeps going beside a window for whoever is reading it. + taken.unseen = unseen ? 1 : 0 guard !discrete.isEmpty else { return taken } diff --git a/native/swift/Sources/Deview/ViewerView.swift b/native/swift/Sources/Deview/ViewerView.swift index cb000241..dcedbffe 100644 --- a/native/swift/Sources/Deview/ViewerView.swift +++ b/native/swift/Sources/Deview/ViewerView.swift @@ -22,6 +22,10 @@ final class ViewerView: NSView, NSViewToolTipOwner { private var panStart = NSPoint.zero private var panFrom = Renderer.PictureSpace() + /// How the other pane's picture was placed then, or nil when that pane has none: how far it + /// can go is part of how far the drag can take the centre the two share. + private var panOther: Renderer.PictureSpace? + /// Where the last frame put things. Read by `Runtime` to anchor the context menu, which is a /// real `NSMenu` and so is popped from outside the drawing code. private(set) var layout = Renderer.Layout() @@ -158,10 +162,12 @@ final class ViewerView: NSView, NSViewToolTipOwner { // A picture enlarged past its space is taken hold of and moved. Before the text below it, // so the same press is not also the start of a selection in the rows above. One that fits // has nowhere to go, and a press on it is whatever it always was. - if let picture = layout.pictures.first(where: { $0.enlarged && $0.bounds.contains(point) }) { + if let index = layout.pictures.firstIndex(where: { $0.enlarged && $0.bounds.contains(point) }) { panning = true panStart = point - panFrom = picture + panFrom = layout.pictures[index] + // There are two at the most, one a pane, so the other is whichever this is not + panOther = layout.pictures.indices.first(where: { $0 != index }).map { layout.pictures[$0] } return } @@ -189,11 +195,12 @@ final class ViewerView: NSView, NSViewToolTipOwner { } if panning { - // Where it is rather than how far it moved, and already inside what the space can show: + // Where it is rather than how far it moved, and already inside what the panes can show: // 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)) + by: CGSize(width: point.x - panStart.x, height: point.y - panStart.y), + other: panOther) Runtime.shared.input.panX = Float(centre.x) Runtime.shared.input.panY = Float(centre.y) return diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 60dbf8cf..4a40c9b5 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -354,6 +354,65 @@ public async Task ADeleteLineRoundTrips() await Assert.That(delete.File).IsEqualTo(@"c:\code\extra.verified.txt"); } + /// + /// Why a delete is held is a line of its own beside the delete's, which keeps its four fields: + /// a reader that predates the line skips it, where a fifth field would have had it refuse the + /// whole listing. + /// + [Test] + public async Task AHeldDeleteSaysWhyOnALineOfItsOwn() + { + var listing = ViewerResponse.Listing( + [], + deletes: + [ + new(@"delete:c:\code\held.verified.txt", "held.verified.txt", null, @"c:\code\held.verified.txt") + { + Held = "Kept: a move wrote this file | and more" + }, + new(@"delete:c:\code\extra.verified.txt", "extra.verified.txt", null, @"c:\code\extra.verified.txt") + ]); + + var text = listing.Build(); + await Assert.That(Fields(text, "delete: ").Select(_ => _.Length)).IsEquivalentTo([4, 4]); + await Assert.That(Fields(text, "held: ").Single().Length).IsEqualTo(2); + await Assert.That(ViewerResponse.TryParse(text, out var parsed)).IsTrue(); + await Assert.That(parsed!.Deletes.Select(_ => _.Held)).IsEquivalentTo(["Kept: a move wrote this file | and more", null]); + } + + /// + /// Both directions of an older peer. An owner that predates the line sends none, which reads + /// as nothing held. And a hold is matched to its delete by key once every line is in, so one + /// naming a delete the listing does not carry is dropped rather than refused. + /// + [Test] + public async Task AHoldIsOptionalAndMatchedByKey() + { + var held = new ViewerResponseDelete("delete:a", "a", null, "a") + { + Held = "why" + }; + var text = ViewerResponse.Listing([], deletes: [held]).Build(); + var holdLine = text.Split('\n').Single(_ => _.StartsWith("held: ", StringComparison.Ordinal)); + + await Assert.That(ViewerResponse.TryParse(text.Replace($"{holdLine}\n", ""), out var older)).IsTrue(); + await Assert.That(older!.Deletes.Single().Held).IsNull(); + + // Ahead of its delete + var deleteLine = text.Split('\n').Single(_ => _.StartsWith("delete: ", StringComparison.Ordinal)); + var reordered = text + .Replace($"{holdLine}\n", "") + .Replace($"{deleteLine}\n", $"{holdLine}\n{deleteLine}\n"); + await Assert.That(ViewerResponse.TryParse(reordered, out var early)).IsTrue(); + await Assert.That(early!.Deletes.Single().Held).IsEqualTo("why"); + + var orphan = ViewerResponse.Listing([]).Build() + holdLine + "\n"; + await Assert.That(ViewerResponse.TryParse(orphan, out var none)).IsTrue(); + await Assert.That(none!.Deletes).IsEmpty(); + + await Assert.That(ViewerResponse.TryParse(text.Replace(holdLine, "held: only-one-field"), out _)).IsFalse(); + } + [Test] public async Task AListingWithoutTrackedItemsParsesEmpty() { diff --git a/src/DiffEngine/Protocol/ViewerResponse.cs b/src/DiffEngine/Protocol/ViewerResponse.cs index 286d68f5..e26edbfd 100644 --- a/src/DiffEngine/Protocol/ViewerResponse.cs +++ b/src/DiffEngine/Protocol/ViewerResponse.cs @@ -34,7 +34,22 @@ record ViewerResponseMove(string Key, string Name, string? Group, string Temp, s /// /// A tracked pending delete riding a full listing. /// -record ViewerResponseDelete(string Key, string Name, string? Group, string File); +record ViewerResponseDelete(string Key, string Name, string? Group, string File) +{ + /// + /// Why the owner's accept-all would leave this delete pending, in words for whoever is looking + /// at it, or null when it would carry it out: a move was accepted onto the file since the + /// delete was raised, or one still pending is going to be. A reader shows it beside the delete + /// and leaves the delete out of its own bulk accepts, since an accept by key is carried out + /// as asked. + /// + /// On a line of its own (held) rather than a fifth field of delete, which a + /// reader that predates it would refuse the whole listing over. That reader skips the line, + /// and an owner that predates it sends none, which reads as nothing held. + /// + /// + public string? Held { get; init; } +} /// /// The reply the queue owner writes before closing the connection. @@ -180,6 +195,10 @@ public string Build() { var group = delete.Group is null ? "" : ViewerPayload.Encode(delete.Group); builder.Append($"delete: {ViewerPayload.Encode(delete.Key)}|{ViewerPayload.Encode(delete.Name)}|{group}|{ViewerPayload.Encode(delete.File)}\n"); + if (delete.Held is not null) + { + builder.Append($"held: {ViewerPayload.Encode(delete.Key)}|{ViewerPayload.Encode(delete.Held)}\n"); + } } return builder.ToString(); @@ -206,6 +225,7 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? var moves = new List(); var deletes = new List(); Dictionary>? variants = null; + Dictionary? holds = null; foreach (var (name, value) in lines) { switch (name) @@ -301,6 +321,15 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? deletes.Add(delete); continue; + case "held": + if (!TryParseHeld(value, out var heldKey, out var reason)) + { + return false; + } + + holds ??= new(StringComparer.Ordinal); + holds[heldKey] = reason; + continue; default: continue; } @@ -324,6 +353,19 @@ public static bool TryParse(string text, [NotNullWhen(true)] out ViewerResponse? } } + // As the variants are: by key, so a hold does not have to follow its delete, and one for + // a delete that is not listed is dropped. + if (holds is not null) + { + for (var index = 0; index < deletes.Count; index++) + { + if (holds.TryGetValue(deletes[index].Key, out var reason)) + { + deletes[index] = deletes[index] with { Held = reason }; + } + } + } + response = new(ok.Value, message, items, window, windowKey) { Moves = moves, @@ -444,4 +486,21 @@ static bool TryParseDelete(string value, [NotNullWhen(true)] out ViewerResponseD delete = new(key, name, group.Length == 0 ? null : group, file); return true; } + + static bool TryParseHeld(string value, out string key, out string reason) + { + key = ""; + reason = ""; + var parts = value.Split('|'); + if (parts.Length != 2 || + !ViewerPayload.TryDecode(parts[0], out var decodedKey) || + !ViewerPayload.TryDecode(parts[1], out var decodedReason)) + { + return false; + } + + key = decodedKey; + reason = decodedReason; + return true; + } } diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index 3c74511a..51f4aff5 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -210,6 +210,54 @@ public async Task ViewerAcceptAllKeepsTheFileATrackedMoveJustWrote() await Assert.That(viewer.Keys()).IsEquivalentTo([TrackedKeys.ForDelete(move.Target)]); } + /// + /// The same pair as the window shows it. The tray marks a delete it would hold in its menu, + /// and the window attached to it showed that delete like any other, with "1 kept" for an + /// answer when it asked for an accept-all. The hold rides the listing, and is the entry's + /// status: why while the move is pending, why once it has been accepted, and nothing once a + /// run raises the delete again - a change to a tracked object that is still the same object, + /// which the listing's tag has to move for all the same. + /// + [Test] + public async Task AnAttachedViewerSaysWhyTheTrayHoldsADelete() + { + await using var pair = new TrayOwned(); + var move = pair.AddMove(); + await File.WriteAllTextAsync(move.Target, "verified"); + var delete = pair.Tracker.AddDelete(move.Target); + var key = TrackedKeys.ForDelete(move.Target); + + await Assert.That(pair.Pump().Queue.Single(_ => _.Key == key).Status).IsEqualTo(Tracker.AwaitsItsFile); + + pair.Link.Post(ViewerSideVerb.AcceptAll, null); + + var viewer = pair.Pump(); + await Assert.That(viewer.Message).IsEqualTo($"Accepted 0, plus 1 files (1 kept). {Tracker.DeletesKept([delete])}"); + await Assert.That(viewer.Queue.Single().Status).IsEqualTo(Tracker.WroteItsFile); + + pair.Tracker.AddDelete(move.Target); + + await Assert.That(pair.Pump().Queue.Single().Status).IsNull(); + } + + /// + /// A move accepted from the tray's own menu, with a window attached: the delete it leaves held + /// says so in the window on the next listing. + /// + [Test] + public async Task AMoveAcceptedInTheTrayMarksItsDeleteInTheAttachedViewer() + { + await using var pair = new TrayOwned(); + var move = pair.AddMove(); + await File.WriteAllTextAsync(move.Target, "verified"); + pair.Tracker.AddDelete(move.Target); + pair.Pump(); + + pair.Tracker.Accept(pair.Tracker.Moves.Single()); + + await Assert.That(pair.Pump().Queue.Single().Status).IsEqualTo(Tracker.WroteItsFile); + } + /// /// A snapshot moving inline lands while an accept-all is applying: its patch, and the delete of /// the verified file that patch replaces. The batch takes its snapshots as it begins, so the @@ -363,6 +411,66 @@ public async Task AViewerGroupAcceptCarriesOutItsDeletesOnceTheTrayWroteTheSnaps await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); } + /// + /// "Accept all in" a group, from a window attached to the tray, over a move and the delete + /// pending on the file it writes. The window sent an accept per key, and the tray carries out + /// an accept by key as asked, so the received file was moved into place and then deleted. A + /// group accept is a bulk accept: the delete the tray holds is left, as the tray's own + /// accept-all leaves it, the one beside it goes, and the held one still goes when it is + /// accepted on its own. + /// + [Test] + public async Task AViewerGroupAcceptLeavesADeleteTheTrayHolds() + { + await using var pair = new TrayOwned(); + var move = pair.AddMove(); + await File.WriteAllTextAsync(move.Target, "verified"); + pair.Tracker.AddDelete(move.Target); + var held = TrackedKeys.ForDelete(move.Target); + var free = pair.AddDelete(); + pair.Pump(); + + pair.Link.PostAcceptGroup([move.Key], [], [held, free.Key]); + + var viewer = pair.Pump(); + await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); + await Assert.That(File.Exists(free.File)).IsFalse(); + await Assert.That(viewer.Keys()).IsEquivalentTo([held]); + await Assert.That(viewer.Message).IsEqualTo(OwnerLink.DeletesKept); + await Assert.That(viewer.Queue.Single().Status).IsEqualTo(Tracker.WroteItsFile); + + pair.Link.Post(ViewerSideVerb.Accept, held); + + await Assert.That(pair.Pump().Queue).IsEmpty(); + await Assert.That(File.Exists(move.Target)).IsFalse(); + } + + /// + /// The same from a window attached to a viewer that owns the queue, which holds the delete by + /// the same rule and says so on the same line of its listing. + /// + [Test] + public async Task AViewerGroupAcceptLeavesADeleteAnOwningViewerHolds() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var move = pair.StageMove(); + await File.WriteAllTextAsync(move.Target, "verified"); + PendingFiles.AddMove(move.Temp, move.Target, null, null, false, null); + await DiffRunner.AddDeleteAsync(move.Target); + var held = TrackedKeys.ForDelete(move.Target); + var attached = new SessionHost(SessionState.Start(ViewerMode.Inline)); + var link = new OwnerLink(attached, pair.Port); + await Assert.That(link.Pump()).IsTrue(); + + link.PostAcceptGroup([move.Key], [], [held]); + + await Assert.That(link.Pump()).IsTrue(); + await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); + await Assert.That(attached.State.Keys()).IsEquivalentTo([held]); + await Assert.That(attached.State.Message).IsEqualTo(OwnerLink.DeletesKept); + } + /// /// The tray answers an unchanged listing without it, but its tracked files change on their own /// scan rather than through the queue, and a focus it has stashed only reaches a window on a @@ -1047,6 +1155,30 @@ public async Task AFullListingCarriesTheViewersOwnPendingFiles() await Assert.That(plain.Deletes).IsEmpty(); } + /// + /// The other arrangement's half of the same rule: a viewer that owns the queue holds a delete + /// whose file a move wrote, as a tray does, and says why on its listing and in the answer to + /// an accept-all, so whoever is attached to it is told what a tray would have told it. + /// + [Test] + public async Task AnOwningViewerHoldsADeleteItsMoveWroteAndSaysWhy() + { + await using var pair = new ViewerOwned(); + using var noTray = new NoTray(); + var move = pair.StageMove(); + await File.WriteAllTextAsync(move.Target, "verified"); + PendingFiles.AddMove(move.Temp, move.Target, null, null, false, null); + await DiffRunner.AddDeleteAsync(move.Target); + + await Assert.That(pair.Send(new(ViewerVerb.ListFull)).Deletes.Single().Held).IsEqualTo(ViewerSession.AwaitsItsFile); + + var response = pair.Send(new(ViewerVerb.AcceptAll)); + + await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); + await Assert.That(response.Message).IsEqualTo($"Accepted 0, plus 1 files (1 kept). {ViewerSession.DeletesKept}"); + await Assert.That(pair.Send(new(ViewerVerb.ListFull)).Deletes.Single().Held).IsEqualTo(ViewerSession.WroteItsFile); + } + /// /// A tray that owns the queue answers these too, and routes them into the same tracked files /// the piper port fills. That is not theoretical: a test process that started before the tray @@ -1347,6 +1479,7 @@ public TrackedMoveFiles StageMove() } public SessionHost Window { get; } + public int Port => server.Port; public RecordingTracker Tracker { get; } public List Applied { get; } = []; public List Failures { get; } = []; diff --git a/src/DiffEngineTray/OwnedInlineHost.cs b/src/DiffEngineTray/OwnedInlineHost.cs index 16c15c60..f6f914dc 100644 --- a/src/DiffEngineTray/OwnedInlineHost.cs +++ b/src/DiffEngineTray/OwnedInlineHost.cs @@ -449,6 +449,7 @@ string IQueueOwner.AcceptAll() (int accepted, int kept)? tracked; string message; bool held; + List keptForAMove = []; lock (accepting) { var moves = TrackedFiles?.Moves().Count ?? 0; @@ -459,6 +460,19 @@ string IQueueOwner.AcceptAll() message = AcceptEvery(moves + deletes.Count, out var refused); tracked = TrackedFiles?.AcceptAll(deletes, refused, Advance); held = refused && deletes.Count > 0; + if (!held && + deletes.Count > 0) + { + // The deletes of this batch that are still pending and held for a move: the + // sweep left those because of the move, and counted them as kept beside the + // ones that failed + var listed = deletes.ToHashSet(); + keptForAMove = TrackedFiles! + .Deletes() + .Where(_ => _.Held is not null && listed.Contains(_.Key)) + .Select(_ => _.Name) + .ToList(); + } } finally { @@ -481,6 +495,13 @@ string IQueueOwner.AcceptAll() return $"{message}, plus {clause}. {Tracker.DeletesHeld}"; } + // "1 kept" alone reads as a failure, and a viewer that asked for this has the answer and + // nothing else to go on: the tray's own accept-all says the same in its balloon + if (keptForAMove.Count > 0) + { + return $"{message}, plus {clause}. {Tracker.DeletesKept(keptForAMove)}"; + } + return $"{message}, plus {clause}"; } diff --git a/src/DiffEngineTray/TrackedDelete.cs b/src/DiffEngineTray/TrackedDelete.cs index eafd0322..68311114 100644 --- a/src/DiffEngineTray/TrackedDelete.cs +++ b/src/DiffEngineTray/TrackedDelete.cs @@ -15,6 +15,11 @@ public TrackedDelete(string file, string? group) /// Set once a move has written this file while the delete was pending, and from then on no /// accept-all carries the delete out: see . Cleared when a /// test run raises the delete again, which is a statement made after the write. + /// + /// The one thing here that changes, and a listing carries what follows from it, so whoever + /// changes it has to say so to : see the tracker's count of + /// restores. + /// /// public bool Written { get; set; } } diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index 595b451c..682075d7 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -662,7 +662,12 @@ public TrackedDelete AddDelete(string file) => Log.Information("DeleteUpdated. File:{file}", file); // Raised again, so by a run that looked at the file as it is now. Whatever a move // wrote there since the delete was first raised, this is the later statement - existing.Written = false; + if (existing.Written) + { + existing.Written = false; + Interlocked.Increment(ref restores); + } + return existing; }); @@ -671,14 +676,19 @@ public TrackedDelete AddDelete(string file) => /// out. For the menu and the debug view, which say it beside the delete, and for the sweeps, /// which act on it. /// - public string? HeldReason(TrackedDelete delete) + public string? HeldReason(TrackedDelete delete) => + HeldReason( + delete, + moves.Values.Any(_ => string.Equals(_.Target, delete.File, StringComparison.OrdinalIgnoreCase))); + + static string? HeldReason(TrackedDelete delete, bool awaited) { if (delete.Written) { return WroteItsFile; } - if (moves.Values.Any(_ => string.Equals(_.Target, delete.File, StringComparison.OrdinalIgnoreCase))) + if (awaited) { return AwaitsItsFile; } @@ -711,9 +721,12 @@ public TrackedDelete AddDelete(string file) => void MarkWritten(string target) { - if (deletes.TryGetValue(target, out var delete)) + if (deletes.TryGetValue(target, out var delete) && + !delete.Written) { delete.Written = true; + // A listing says why a delete is held, so this is a change to what one carries + Interlocked.Increment(ref restores); } } @@ -816,9 +829,11 @@ void Restore(TrackedMove removed) } /// - /// How many times something taken out to be accepted has been put back, which is the one - /// change to what is tracked that cannot see by looking: - /// the same object in the same place as before it left. + /// How many times something taken out to be accepted has been put back, which is a change to + /// what is tracked that cannot see by looking: the same + /// object in the same place as before it left. And how many times a delete's + /// has changed, which is the other: the one thing a + /// listing carries of a tracked object that is not fixed when the object is made. /// long restores; @@ -1226,10 +1241,16 @@ List AcceptDeletes(List pending, HashSet w /// What a user is told about deletes an accept-all kept because of a move onto their file. /// The menu says the same beside each of them, for whoever missed the balloon. /// - internal static string DeletesKept(IReadOnlyList kept) + internal static string DeletesKept(IReadOnlyList kept) => + DeletesKept(kept.Select(_ => _.Name).ToList()); + + /// + /// By name, for the wire's accept-all, which knows the deletes it kept as a listing has them. + /// + internal static string DeletesKept(IReadOnlyList kept) { var which = kept.Count == 1 - ? $"The pending delete of '{kept[0].Name}' was kept" + ? $"The pending delete of '{kept[0]}' was kept" : $"{kept.Count} pending deletes were kept"; return $"{which}, since a move was accepted onto the same file, or is still to be, and deleting it would remove what the move put there. Accept a delete on its own to delete its file anyway."; } @@ -1271,14 +1292,28 @@ IReadOnlyList ITrackedFiles.Moves() => _.Target)) .ToList(); - IReadOnlyList ITrackedFiles.Deletes() => - deletes.Values + IReadOnlyList ITrackedFiles.Deletes() + { + // The files the pending moves are onto, gathered once: HeldReason walks the moves for + // the one delete it is asked about, which here would be every move for every delete + var awaited = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var move in moves) + { + awaited.Add(move.Value.Target); + } + + return deletes.Values .Select(_ => new ViewerResponseDelete( TrackedKeys.ForDelete(_.File), _.Name, _.Group, - _.File)) + _.File) + { + // What the menu says beside it, for a viewer showing this queue to say too + Held = HeldReason(_, awaited.Contains(_.File)) + }) .ToList(); + } readonly Lock versionGate = new(); readonly List versioned = []; @@ -1309,6 +1344,11 @@ IReadOnlyList ITrackedFiles.Deletes() => /// two places, and the delete that could not be deleted, and they are /// counted. /// + /// + /// Why a delete is held rides a listing too (). Half of that is which + /// moves are tracked, which is seen here already, and the other half is a flag set on a + /// delete that stays the same object, so its two changes are counted with the restores. + /// /// long ITrackedFiles.Version() { 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 59afe83e..ed2b5718 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 7f232f08..bc05f05e 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 9378285c..57d2f6d7 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 9378285c..57d2f6d7 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/AttachedViewerTests.cs b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs index 1c52460d..05cb3c6f 100644 --- a/src/DiffEngineViewer.Tests/AttachedViewerTests.cs +++ b/src/DiffEngineViewer.Tests/AttachedViewerTests.cs @@ -416,6 +416,64 @@ public async Task UnchangedFilesAreNotReReadAndAChangeRefreshes() } } + /// + /// A delete the owner's accept-all would leave is marked in the owner's own menu, and looked + /// like any other here. What the owner says of it is the entry's status, so the row carries + /// the mark a failure does and its tip says why. The same entry from pump to pump while the + /// owner says the same, and unmarked again once the owner lets go of it. + /// + [Test] + public async Task ADeleteTheOwnerHoldsSaysWhy() + { + var file = Path.Combine(Path.GetTempPath(), $"AttachedViewerTests_{Guid.NewGuid():N}.verified.txt"); + await File.WriteAllTextAsync(file, "first"); + try + { + var held = "Kept by 'Accept all': a move was accepted onto this file."; + // ReSharper disable once AccessToModifiedClosure + var (server, cancel) = Listing(() => ViewerResponse.Listing( + [], + deletes: + [ + new(TrackedKeys.ForDelete(file), "extra.verified.txt", null, file) + { + Held = held + } + ])); + using (server) + using (cancel) + { + var host = new SessionHost(SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows)); + var link = new OwnerLink(host, server.Port); + + link.Pump(); + var first = host.State.Queue.Single(); + await Assert.That(first.Status).IsEqualTo(held); + var row = QueueProjection.Rows(host.State).Single(_ => _.Kind == QueueRowKind.Entry); + await Assert.That(row.Status).IsEqualTo(held); + await Assert.That(row.Tooltip!).Contains(held); + + link.Pump(); + await Assert.That(host.State.Queue.Single()).IsSameReferenceAs(first); + + // Still said of the entry read again from a file that changed + await File.WriteAllTextAsync(file, "second, longer", cancel.Token); + link.Pump(); + await Assert.That(host.State.Queue.Single().RightText).IsEqualTo("second, longer"); + await Assert.That(host.State.Queue.Single().Status).IsEqualTo(held); + + held = null; + link.Pump(); + await Assert.That(host.State.Queue.Single().Status).IsNull(); + await cancel.CancelAsync(); + } + } + finally + { + File.Delete(file); + } + } + /// /// Accepting a conflicted entry names the variant on screen, and the owner applies exactly /// that one. @@ -508,7 +566,82 @@ await Assert.That(string.Join(" | ", owner.Actions)) } } - static readonly InlinePatch solutionA = Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 10); + /// + /// "Accept all in" a group sends an accept per key, and the owner carries out an accept by key + /// as asked, held or not. So the delete an owner says it holds is not sent, and the one beside + /// it is. An owner that predates the hold says nothing of it and is sent both, as it always + /// was. + /// + [Test] + [Arguments(true)] + [Arguments(false)] + public async Task AGroupAcceptLeavesOutADeleteTheOwnerHolds(bool ownerSaysHeld) + { + var held = Path.Combine(Path.GetTempPath(), $"AttachedViewerTests_{Guid.NewGuid():N}.verified.txt"); + var free = Path.Combine(Path.GetTempPath(), $"AttachedViewerTests_{Guid.NewGuid():N}.verified.txt"); + await File.WriteAllTextAsync(held, "what a move put here"); + await File.WriteAllTextAsync(free, "stale"); + try + { + if (!ViewerServer.TryBind(0, out var server)) + { + throw new("Could not bind an ephemeral port."); + } + + var accepted = new ConcurrentQueue(); + using (server) + using (var cancel = new CancelSource()) + { + _ = server.Listen( + message => + { + if (message.Verb == ViewerVerb.Accept) + { + accepted.Enqueue(message.Key!); + return ViewerResponse.Success("Deleted"); + } + + return ViewerResponse.Listing( + [], + deletes: + [ + new(TrackedKeys.ForDelete(held), "held.verified.txt", null, held) + { + Held = ownerSaysHeld ? "Kept by 'Accept all'" : null + }, + new(TrackedKeys.ForDelete(free), "free.verified.txt", null, free) + ]); + }, + cancel.Token); + var host = new SessionHost(SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows)); + var link = new OwnerLink(host, server.Port); + link.Pump(); + + link.PostAcceptGroup([], [], [TrackedKeys.ForDelete(held), TrackedKeys.ForDelete(free)]); + link.Pump(); + + if (ownerSaysHeld) + { + await Assert.That(accepted).IsEquivalentTo([TrackedKeys.ForDelete(free)]); + await Assert.That(host.State.Message).IsEqualTo(OwnerLink.DeletesKept); + } + else + { + await Assert.That(accepted).IsEquivalentTo([TrackedKeys.ForDelete(held), TrackedKeys.ForDelete(free)]); + await Assert.That(host.State.Message).IsEqualTo("Deleted"); + } + + await cancel.CancelAsync(); + } + } + finally + { + File.Delete(held); + File.Delete(free); + } + } + + static readonly InlinePatch solutionA =Fixtures.Patch(Fixtures.SolutionFile("SolutionA", "Tests", "ATests.cs"), 10); static readonly InlinePatch solutionB = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 10); /// diff --git a/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs b/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs new file mode 100644 index 00000000..35216785 --- /dev/null +++ b/src/DiffEngineViewer.Tests/MoveOntoDeleteTests.cs @@ -0,0 +1,250 @@ +/// +/// A pending move and a pending delete that name the same verified file, in a queue this viewer +/// owns. The delete can be the last copy of a snapshot leaving and the move is the snapshot +/// arriving, so a bulk accept that carries out both, the move first, leaves neither file. The +/// tray's tracker has the same pair and the same rules for it (TrackerMoveOntoDeleteTest). +/// +public class MoveOntoDeleteTests +{ + const string temp = "temp/sample.received.txt"; + const string target = "code/sample.verified.txt"; + + /// + /// The order a batch took them in when the delete was raised second: the received file was + /// moved into place and then deleted. + /// + [Test] + public async Task An_accept_all_keeps_the_file_its_move_just_wrote() + { + var disk = new Disk(); + var state = Queued(Fixtures.Move(), Delete()); + + var swept = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + await Assert.That(disk.Files[target]).IsEqualTo("received"); + var delete = swept.Queue.Single(); + await Assert.That(delete.Kind).IsEqualTo(QueueEntryKind.Delete); + await Assert.That(delete.Status).IsEqualTo(ViewerSession.WroteItsFile); + await Assert.That(swept.Message).IsEqualTo($"Accepted 0, plus 1 files (1 kept). {ViewerSession.DeletesKept}"); + } + + /// + /// A move onto a file is a run that verified against it, so the delete an earlier run raised + /// for that file no longer describes a stale one, and goes as the move arrives. + /// + [Test] + public async Task A_move_withdraws_the_delete_pending_on_its_target() + { + var disk = new Disk(); + var state = Queued(Delete(), Fixtures.Move()); + + await Assert.That(state.Queue.Select(_ => _.Kind)).IsEquivalentTo([QueueEntryKind.Move]); + await Assert.That(state.Current!.Kind).IsEqualTo(QueueEntryKind.Move); + + var swept = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + await Assert.That(disk.Files[target]).IsEqualTo("received"); + await Assert.That(swept.Queue).IsEmpty(); + } + + /// + /// The delete of some other file is nothing to do with the move, and goes with it. + /// + [Test] + public async Task A_delete_of_another_file_is_carried_out() + { + var disk = new Disk(); + disk.Files["code/extra.verified.txt"] = "stale"; + var state = Queued(Fixtures.Delete(), Fixtures.Move(), Fixtures.Delete("other.verified.txt")); + + var swept = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + await Assert.That(swept.Queue).IsEmpty(); + await Assert.That(disk.Files.Keys).IsEquivalentTo([target]); + await Assert.That(swept.Message).IsEqualTo("Accepted 0, plus 3 files"); + } + + /// + /// The hold is the delete's, not the batch's: a second accept-all, or one after the move was + /// accepted on its own, finds a delete that looks like any other unless it remembers. + /// + [Test] + public async Task A_delete_whose_file_a_single_accept_wrote_is_held_by_every_accept_all_after() + { + var disk = new Disk(); + var state = Queued(Fixtures.Move(), Delete()); + state = ViewerSession.SelectKey(state, Fixtures.Move().Key); + + state = ViewerSession.Apply(state, CommandKind.Accept, disk.Actions); + await Assert.That(state.Queue.Single().Status).IsEqualTo(ViewerSession.WroteItsFile); + + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + await Assert.That(disk.Files[target]).IsEqualTo("received"); + await Assert.That(state.Queue.Single().Kind).IsEqualTo(QueueEntryKind.Delete); + } + + /// + /// Accepted on its own it is carried out, held or not: that is the reviewer saying the file + /// is redundant. + /// + [Test] + public async Task A_held_delete_accepted_on_its_own_is_carried_out() + { + var disk = new Disk(); + var state = Queued(Fixtures.Move(), Delete()); + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + state = ViewerSession.Apply(state, CommandKind.Accept, disk.Actions); + + await Assert.That(state.Queue).IsEmpty(); + await Assert.That(disk.Files).IsEmpty(); + } + + /// + /// Raised again, so by a run that looked at the file as it is now: the later statement, and + /// the hold is let go. Both ways an arrival is taken, since the file a move wrote may or may + /// not hold what the delete's entry was showing. + /// + [Test] + [Arguments(Fixtures.Expected)] + [Arguments("something else")] + public async Task A_delete_raised_again_is_no_longer_held(string contentNow) + { + var disk = new Disk(); + var state = Queued(Fixtures.Move(), Delete()); + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + state = ViewerSession.EnqueueTracked(state, Delete(contentNow)); + await Assert.That(state.Queue.Single().Status).IsNull(); + await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsNull(); + + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + + await Assert.That(state.Queue).IsEmpty(); + await Assert.That(disk.Files).IsEmpty(); + } + + /// + /// A move that could not be carried out is still going to write the file, so its delete waits + /// for it, and stops waiting if the move is discarded. + /// + [Test] + public async Task A_delete_waits_for_a_move_that_is_still_pending() + { + var disk = new Disk(); + var locked = disk.Actions with + { + MoveFile = static (_, _) => throw new("The file is locked.") + }; + var state = Queued(Fixtures.Move(), Delete()); + + state = ViewerSession.Apply(state, CommandKind.AcceptAll, locked); + + await Assert.That(state.Queue.Select(_ => _.Kind)).IsEquivalentTo([QueueEntryKind.Move, QueueEntryKind.Delete]); + var delete = state.Queue.Single(_ => _.Kind == QueueEntryKind.Delete); + await Assert.That(delete.Status).IsEqualTo(ViewerSession.AwaitsItsFile); + await Assert.That(disk.Files[target]).IsEqualTo("verified"); + + state = ViewerSession.SelectKey(state, Fixtures.Move().Key); + state = ViewerSession.Apply(state, CommandKind.Discard, disk.Actions); + await Assert.That(ViewerSession.HeldReason(state.Queue, state.Queue.Single())).IsNull(); + + state = ViewerSession.Apply(state, CommandKind.AcceptAll, disk.Actions); + await Assert.That(state.Queue).IsEmpty(); + await Assert.That(disk.Files).IsEmpty(); + } + + /// + /// "Accept all in" a header is the same batch, so it holds what an accept-all holds. + /// + [Test] + public async Task A_group_accept_keeps_the_file_its_move_just_wrote() + { + var disk = new Disk(); + var other = Fixtures.Patch(Fixtures.SolutionFile("SolutionB", "Tests", "BTests.cs"), 10); + var state = ViewerSession.EnqueueTracked(Fixtures.Inline(other), Fixtures.Move(solution: "SolutionA")); + state = ViewerSession.EnqueueTracked(state, Delete(solution: "SolutionA")); + var visible = QueueProjection.Visible(state, ScreenBuilder.BodyRows(state), out _).ToList(); + state = ViewerSession.OpenMenu(state, visible.FindIndex(_ => _.GroupName == "SolutionA")); + + var swept = ViewerSession.Apply(state, CommandKind.AcceptGroup, disk.Actions); + + await Assert.That(disk.Files[target]).IsEqualTo("received"); + await Assert.That(swept.Queue.Select(_ => _.Kind)).IsEquivalentTo([QueueEntryKind.Inline, QueueEntryKind.Delete]); + } + + /// + /// The watch over owned files reads a delete's file again once the move has written it, and + /// the entry it builds from what is there now is still the delete that was held. + /// + [Test] + public async Task A_held_delete_read_again_is_still_held() + { + var directory = Path.Combine(Path.GetTempPath(), $"MoveOntoDeleteTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + var file = Path.Combine(directory, "sample.verified.txt"); + await File.WriteAllTextAsync(file, "verified"); + var queued = TrackedEntry.ForDelete(file) with + { + Written = true, + Status = ViewerSession.WroteItsFile + }; + + await File.WriteAllTextAsync(file, "received"); + var again = TrackedEntry.DeleteAgain(queued, file); + + await Assert.That(again.RightText).IsEqualTo("received"); + await Assert.That(again.Written).IsTrue(); + await Assert.That(again.Status).IsEqualTo(ViewerSession.WroteItsFile); + } + finally + { + Directory.Delete(directory, true); + } + } + + static SessionState Queued(params QueueEntry[] entries) + { + var state = SessionState.Start(ViewerMode.Inline, Fixtures.Columns, Fixtures.Rows); + foreach (var entry in entries) + { + state = ViewerSession.EnqueueTracked(state, entry); + } + + return state; + } + + /// + /// A pending delete of the file is a move onto. + /// + static QueueEntry Delete(string content = Fixtures.Expected, string? solution = null) => + Fixtures.Delete("sample.verified.txt", solution, content); + + /// + /// The files a test's actions act on, so what is asserted is what is left rather than which + /// calls were made. + /// + sealed class Disk + { + public Dictionary Files { get; } = new() + { + [temp] = "received", + [target] = "verified" + }; + + public ViewerActions Actions => + Fixtures.Applied with + { + MoveFile = (from, to) => + { + Files[to] = Files[from]; + Files.Remove(from); + }, + DeleteFile = _ => Files.Remove(_) + }; + } +} diff --git a/src/DiffEngineViewer.Tests/PixelTests.ATallFooterTakesRowsFromTheBody.Linux.verified.png b/src/DiffEngineViewer.Tests/PixelTests.ATallFooterTakesRowsFromTheBody.Linux.verified.png new file mode 100644 index 00000000..0cfb702a Binary files /dev/null and b/src/DiffEngineViewer.Tests/PixelTests.ATallFooterTakesRowsFromTheBody.Linux.verified.png differ diff --git a/src/DiffEngineViewer.Tests/PixelTests.cs b/src/DiffEngineViewer.Tests/PixelTests.cs index 4ed645ab..8141fc91 100644 --- a/src/DiffEngineViewer.Tests/PixelTests.cs +++ b/src/DiffEngineViewer.Tests/PixelTests.cs @@ -647,6 +647,77 @@ public async Task AHiddenWindowIsNotDrawn() await Assert.That(shown).IsEqualTo(1); } + /// + /// A footer taller than the lines the model keeps for it takes its rows from the body: the + /// Linux head reports fewer rows for a window whose footer is that tall, and the screen built + /// for what it reports ends above the footer. It reported the window's height in rows whatever + /// the footer came to, so the last rows of the body were sliced, handed over and laid out + /// under the buttons. + /// + /// The rows are asked of the window, shown for as long as that takes, since a capture measures + /// nothing: first with the one row of buttons a pair of files has, which fits in what the + /// model allows, and then with those buttons eight times over, which is five rows. That is + /// more than any real screen has at this width, and is what a paged document has in a window + /// under 450 pixels wide, a size the one window these tests share is never given. The picture + /// is the second screen, built for the rows the window reported for it: forty lines a side, + /// of which the last one sliced is the one above the footer. + /// + /// + [Test] + [PixelTest] + [NotInParallel(nameof(PixelTests), Order = 23)] + [SkipOnMac("A capture host never creates the macOS window, and it is the window whose rows are measured.")] + public async Task ATallFooterTakesRowsFromTheBody() + { + const int measuredRows = height / 17; + var state = ViewerSession.Resize(Fixtures.File(Fixtures.Long(true), Fixtures.Long(false)), columns, measuredRows); + var (fitting, tall) = await OnShimThread( + () => + { + window!.SetHidden(false); + try + { + return (Measured(ScreenBuilder.Build(state)), Measured(TallFooter(state))); + } + finally + { + window.SetHidden(true); + } + }); + + await Assert.That(fitting).IsEqualTo(measuredRows); + await Assert.That(tall).IsLessThan(measuredRows); + await Capture(TallFooter(ViewerSession.Resize(state, columns, tall))); + } + + /// + /// The rows the window reports once it has laid a screen out. Three presents, since a table + /// can take a second frame to settle on where its rows are. + /// + static int Measured(Screen screen) + { + for (var frame = 0; frame < 3; frame++) + { + window!.Present(screen); + } + + return window!.Poll().Rows; + } + + static Screen TallFooter(SessionState state) + { + var screen = ScreenBuilder.Build(state); + return screen with + { + Buttons = + [ + .. Enumerable + .Range(1, 8) + .SelectMany(_ => screen.Buttons.Select(button => button with {Label = $"{button.Label} {_}"})) + ] + }; + } + static uint drawnQuery; /// @@ -702,9 +773,11 @@ static class Gl public static extern void GetQueryObject(uint id, uint name, out uint value); } - static async Task Capture(SessionState state, int gridRows = rows) + static Task Capture(SessionState state, int gridRows = rows) => + Capture(ScreenBuilder.Build(ViewerSession.Resize(state, columns, gridRows))); + + static async Task Capture(Screen screen) { - 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/ViewerProgramTests.cs b/src/DiffEngineViewer.Tests/ViewerProgramTests.cs index 5d572b20..02bf6f31 100644 --- a/src/DiffEngineViewer.Tests/ViewerProgramTests.cs +++ b/src/DiffEngineViewer.Tests/ViewerProgramTests.cs @@ -124,6 +124,92 @@ static IViewerWindow Open(string title, int width, int height, bool hidden, Wind await Assert.That(preferences.Window).IsEqualTo(new WindowPlacement(10, 20, 900, 600, true)); } + /// + /// A window in the taskbar or the Dock, or wholly behind another, is one nobody is reading, + /// and the loop only knew that of a window it had hidden itself: the owner went on being + /// listed, the pending files watched and the documents drawn as for one on screen. A head + /// says so with each poll now, and what is kept going beside the window is slowed for as long + /// as it does. + /// + [Test] + public async Task AWindowNobodyCanSeeSlowsWhatIsKeptGoingBesideIt() + { + var host = new SessionHost(Fixtures.File()); + var link = new OwnerLink(host, port: 1); + var watch = new TrackedWatch(host); + // None of the three is run, so the reader is never asked for a document: each is only + // told whether to slow + var reader = new DocumentWatch(host, null!); + List slowed = []; + var window = new ScriptedWindow( + [true, true, false], + () => slowed.Add($"{link.Hidden} {watch.Hidden} {reader.Hidden}")); + + ViewerProgram.Loop(host, window, link, reader, watch, new(), null, new(), new()); + + // As each frame was presented: before anything was said, after each of the two polls + // that said nobody could see the window, and after the one that said somebody could + await Assert.That(string.Join(", ", slowed)) + .IsEqualTo("False False False, True True True, True True True, False False False"); + } + + /// + /// The two reasons come and go apart. A window hidden from here stays slowed whatever its head + /// goes on to say, since a head need not count a window it was told to hide. + /// + [Test] + public async Task AWindowHiddenFromHereStaysSlowedWhateverItsHeadSays() + { + var host = new SessionHost(Fixtures.File()); + var watch = new TrackedWatch(host); + List slowed = []; + var window = new ScriptedWindow([true, false], () => slowed.Add(watch.Hidden)); + ConcurrentQueue commands = new(); + commands.Enqueue(WindowCommand.Hide); + + ViewerProgram.Loop(host, window, null, null, watch, commands, null, new(), new()); + + await Assert.That(string.Join(", ", slowed)).IsEqualTo("True, True, True"); + } + + /// + /// A window that says, poll by poll, whether anybody can see it, and nothing else. It closes + /// on the present after its last poll, and tells the test as each present begins. + /// + sealed class ScriptedWindow(bool[] unseen, Action presenting) : IViewerWindow + { + int polls; + + public bool Present(Screen screen) + { + presenting(); + return polls < unseen.Length; + } + + public ViewerInput Poll() => + // Not default: that zeroes every index, and zero is the first button and the first row + new(CommandKind.None, -1, -1, 0, false, Fixtures.Columns, Fixtures.Rows, Unseen: unseen[polls++]); + + public void SetHidden(bool hidden) + { + } + + public void Focus() + { + } + + public void SetClipboard(string text) + { + } + + public bool Capture(Screen screen, int width, int height, string pngPath) => + false; + + public void Dispose() + { + } + } + /// /// Closed on its first frame, which is all a test of what happens around the loop needs. /// diff --git a/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs b/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs index 734e03a8..c0eb2bb9 100644 --- a/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/FrameWaitTests.cs @@ -43,4 +43,32 @@ public async Task AMinimisedWindowIsStillVisible() await Assert.That(visible).IsTrue(); await Assert.That(FormsViewerWindow.FrameWait(visible, state)).IsEqualTo(100); } + + /// + /// And the loop is told, since the wait above slows only this head's own frames: what the loop + /// keeps going beside the window went on as for one being read. Shown as the window above is, + /// and for its reasons. + /// + [Test] + public async Task AMinimisedWindowSaysNobodyCanSeeIt() + { + var (shown, minimised) = UnseenDesktop.Run(() => + { + using var form = new ViewerForm("title", 800, 600) + { + Opacity = 0, + ShowInTaskbar = false, + StartPosition = FormStartPosition.Manual, + Location = new(-4000, -2000), + Parked = true + }; + form.Show(); + var shown = form.Drain().Unseen; + form.WindowState = FormWindowState.Minimized; + return (shown, form.Drain().Unseen); + }); + + await Assert.That(shown).IsFalse(); + await Assert.That(minimised).IsTrue(); + } } diff --git a/src/DiffEngineViewer.Windows.Tests/PicturePlacementTests.cs b/src/DiffEngineViewer.Windows.Tests/PicturePlacementTests.cs index cb3e9db3..2f35d873 100644 --- a/src/DiffEngineViewer.Windows.Tests/PicturePlacementTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/PicturePlacementTests.cs @@ -105,6 +105,53 @@ public async Task ADragStopsAtTheEdgeOfThePicture() await Assert.That(dragged).IsEqualTo(new(0.125, 0.8125)); } + /// + /// The centre is one point for both panes, and here it is somewhere the pane beside can show + /// and this one cannot: a wide picture beside a tall one, eight times the size that fits, with + /// the tall one dragged nearly to its bottom. Dragging the wide one sideways leaves that where + /// it is. It came back as far as the wide one's own picture can go, so the tall one jumped. + /// + [Test] + public async Task ADragAcrossLeavesThePaneBesideWhereItIsDown() + { + var wide = PicturePlacement.Of(space, Picture(200, 100, zoom: 8, y: 0.875)); + var tall = PicturePlacement.Of(space, Picture(100, 200, zoom: 8, y: 0.875)); + + // What the wide one draws about, which is as far down as it goes + await Assert.That(wide.Centre.Y).IsEqualTo(0.8125); + await Assert.That(tall.Centre.Y).IsEqualTo(0.875); + await Assert.That(wide.Dragged(new(160, 0), tall)).IsEqualTo(new(0.4, 0.875)); + } + + /// + /// And a drag stops where the pane that can go further does, each way: the wide picture's + /// range across and the tall one's down, whichever of the two is under the pointer. + /// + [Test] + public async Task ADragStopsWhereThePaneThatGoesFurtherDoes() + { + var wide = PicturePlacement.Of(space, Picture(200, 100, zoom: 8)); + var tall = PicturePlacement.Of(space, Picture(100, 200, zoom: 8)); + + await Assert.That(wide.Dragged(new(100_000, -100_000), tall)).IsEqualTo(new(0.125, 0.90625)); + await Assert.That(tall.Dragged(new(100_000, -100_000), wide)).IsEqualTo(new(0.125, 0.90625)); + } + + /// + /// A way the picture cannot move, because all of it shows, is left as the model had it. Kept + /// inside what this pane shows it was the middle, on every move of a drag along the other + /// way, which took the pane beside back to its middle from wherever it had been dragged to. + /// + [Test] + public async Task AWayThePictureCannotMoveIsLeftAsTheModelHadIt() + { + var placement = PicturePlacement.Of(space, Picture(200, 50, zoom: 4, y: 0.2)); + + await Assert.That(placement.MovesAcross).IsTrue(); + await Assert.That(placement.MovesDown).IsFalse(); + await Assert.That(placement.Dragged(new(80, 50))).IsEqualTo(new(0.4, 0.2)); + } + static ImagePane Picture(int width, int height, double zoom = 1, double x = 0.5, double y = 0.5) => new("picture.png", width, height, "HASH", zoom, x, y); } diff --git a/src/DiffEngineViewer.Windows.Tests/PictureZoomTests.cs b/src/DiffEngineViewer.Windows.Tests/PictureZoomTests.cs index 62b1199f..7286d2ae 100644 --- a/src/DiffEngineViewer.Windows.Tests/PictureZoomTests.cs +++ b/src/DiffEngineViewer.Windows.Tests/PictureZoomTests.cs @@ -117,6 +117,55 @@ public async Task DraggingAnEnlargedPictureReportsWhereItWent() await Assert.That(host.Canvas.TakePan()).IsNull(); } + /// + /// The pane beside is part of a drag: a wide picture beside a tall one, enlarged, with the + /// centre the two share as far down as the tall one can show, which is further than the wide + /// one can. Dragging the wide one sideways reports that centre as far down as it was, where + /// it reported it as far down as its own picture goes, and the tall one jumped up to there. + /// + [Test] + public async Task DraggingOnePictureLeavesTheOtherWhereItWasOnTheOtherAxis() + { + var directory = Path.Combine(Path.GetTempPath(), $"deview-pan-{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + var wide = Path.Combine(directory, "wide.received.png"); + var tall = Path.Combine(directory, "tall.verified.png"); + File.WriteAllBytes(wide, SamplePng.Build(160, 120, 198, 64, 64)); + File.WriteAllBytes(tall, SamplePng.Build(120, 160, 64, 150, 198)); + using var host = new Host(); + var state = Enlarged( + ViewerSession.EnqueueFile( + SessionState.Start(ViewerMode.File, Fixtures.Columns, Fixtures.Rows), + QueueEntry.ForFiles(wide, tall, FileSide.Read(wide), FileSide.Read(tall)))); + // As far down as there is, which each pane brings in to as far as its own picture goes + state = ViewerSession.PanTo(state, 0.5, 1); + host.Draw(state); + var screen = ScreenBuilder.Build(ViewerSession.Resize(state, columns, rows)); + var left = PicturePlacement.Of(host.Canvas.PictureAreas[0], screen.Left.Image!); + var right = PicturePlacement.Of(host.Canvas.PictureAreas[1], screen.Right.Image!); + var press = Middle(host.Canvas.PictureAreas[0]); + + host.Press(press); + host.Move(new(press.X + 40, press.Y)); + var dragged = host.Canvas.TakePan(); + host.Release(new(press.X + 40, press.Y)); + + // Or the scene is not the one this is about: both move down, the right one further + await Assert.That(left.MovesDown).IsTrue(); + await Assert.That(left.Centre.Y).IsLessThan(right.Centre.Y); + await Assert.That(dragged).IsEqualTo(left.Dragged(new(40, 0), right)); + // To what a float keeps of how much shows, which is what a drag is kept inside + await Assert.That(Math.Abs(dragged!.Value.Y - right.Centre.Y)).IsLessThan(0.000001); + await Assert.That(dragged.Value.X).IsLessThan(0.5); + } + finally + { + Directory.Delete(directory, true); + } + } + /// /// A picture that fits has nowhere to go, so pressing on one and moving is nothing. /// diff --git a/src/DiffEngineViewer.Windows/PicturePlacement.cs b/src/DiffEngineViewer.Windows/PicturePlacement.cs index 6e7834a6..ac98c488 100644 --- a/src/DiffEngineViewer.Windows/PicturePlacement.cs +++ b/src/DiffEngineViewer.Windows/PicturePlacement.cs @@ -23,8 +23,25 @@ /// The point at the middle of what shows, which is the one asked for moved as far as it has to /// be for the picture to fill the space. /// -readonly record struct PicturePlacement(Rectangle Bounds, RectangleF Source, SizeF Size, PanPoint Centre) +/// +/// The point the model asked for, before it was moved in. One point for both panes, so it can be +/// somewhere this pane cannot show and the other can. +/// +readonly record struct PicturePlacement(Rectangle Bounds, RectangleF Source, SizeF Size, PanPoint Centre, PanPoint Asked) { + /// + /// Whether there is more of the picture across than the space shows, by a whole pixel or more, + /// which is whether a drag can move it that way. + /// + public bool MovesAcross => + (int) Size.Width > Bounds.Width; + + /// + /// As , down. + /// + public bool MovesDown => + (int) Size.Height > Bounds.Height; + public static PicturePlacement Of(Rectangle available, ImagePane image) { var fit = Math.Min( @@ -37,7 +54,7 @@ public static PicturePlacement Of(Rectangle available, ImagePane image) Math.Max(1, (int) (image.Height * fit))); if (image.Zoom <= 1) { - return new(Centred(available, fitted), new(0, 0, 1, 1), fitted, PanPoint.Centre); + return new(Centred(available, fitted), new(0, 0, 1, 1), fitted, PanPoint.Centre, new(image.CenterX, image.CenterY)); } var width = fitted.Width * image.Zoom; @@ -54,23 +71,60 @@ public static PicturePlacement Of(Rectangle available, ImagePane image) Centred(available, shown), new((float) (centre.X - across / 2), (float) (centre.Y - down / 2), (float) across, (float) down), new((float) width, (float) height), - centre); + centre, + new(image.CenterX, image.CenterY)); } /// /// The centre after the pointer has dragged the picture pixels: the /// picture follows the pointer, so the point at the middle moves the other way. Kept inside - /// what the space can show, which is why it is asked here rather than worked out by the model. + /// what the panes can show, which is why it is asked here rather than worked out by the model. + /// + /// The centre is one point for both panes, and the two pictures need not be the same shape, + /// so what is moved is the point the model asked for and not the one this pane drew about, + /// and it is kept inside what the pane that shows less of its picture can show. Moved from + /// this pane's own and kept to this pane's range, a drag put the other pane's picture where + /// this one's could go: back to its middle row on the first move of a drag along the other + /// axis, when all of this one shows from top to bottom, and in from its edge when both can + /// move and the other can move further. + /// + /// + /// A way this picture cannot move is left as the model had it, whatever the pointer does: the + /// picture under the pointer is the one being dragged. + /// /// - public PanPoint Dragged(Size by) + /// How far the pointer has gone since the button went down. + /// Where the other pane's picture is, or null when it has none. + public PanPoint Dragged(Size by, PicturePlacement? other = null) { - var across = Source.Width; - var down = Source.Height; + // How much of its picture shows each way in whichever pane shows less of it + double across = Source.Width; + double down = Source.Height; + if (other is { } beside) + { + if (beside.MovesAcross) + { + across = Math.Min(across, beside.Source.Width); + } + + if (beside.MovesDown) + { + down = Math.Min(down, beside.Source.Height); + } + } + return new( - Math.Clamp(Centre.X - by.Width / (double) Size.Width, across / 2.0, 1 - across / 2.0), - Math.Clamp(Centre.Y - by.Height / (double) Size.Height, down / 2.0, 1 - down / 2.0)); + MovesAcross ? Kept(Kept(Asked.X, across) - by.Width / (double) Size.Width, across) : Asked.X, + MovesDown ? Kept(Kept(Asked.Y, down) - by.Height / (double) Size.Height, down) : Asked.Y); } + /// + /// A centre moved in as far as it takes for a pane showing of its + /// picture to stay full. + /// + static double Kept(double value, double shown) => + Math.Clamp(value, shown / 2, 1 - shown / 2); + static Rectangle Centred(Rectangle available, Size size) => new( available.X + (available.Width - size.Width) / 2, diff --git a/src/DiffEngineViewer.Windows/ViewerCanvas.cs b/src/DiffEngineViewer.Windows/ViewerCanvas.cs index c0ab63cb..1cda9903 100644 --- a/src/DiffEngineViewer.Windows/ViewerCanvas.cs +++ b/src/DiffEngineViewer.Windows/ViewerCanvas.cs @@ -848,6 +848,12 @@ void FillChecker(Graphics graphics, Rectangle bounds) => PicturePlacement panFrom; + /// + /// The placement the other pane's picture had then, or null when it has none: how far that + /// one can go is part of how far the drag can take the centre the two share. + /// + PicturePlacement? panBeside; + /// /// Where a drag has left the middle of the picture, until reports it. /// @@ -1180,6 +1186,15 @@ protected override void OnMouseDown(MouseEventArgs e) Capture = true; panStart = e.Location; panFrom = PicturePlacement.Of(picture.Available, picture.Image); + panBeside = null; + foreach (var other in pictures) + { + if (other != picture) + { + panBeside = PicturePlacement.Of(other.Available, other.Image); + } + } + return; } @@ -1237,7 +1252,7 @@ protected override void OnMouseMove(MouseEventArgs e) { // From where the button went down rather than from the last move, so the picture is // where the pointer has taken it however the moves in between were reported - pan = panFrom.Dragged(new(e.X - panStart.X, e.Y - panStart.Y)); + pan = panFrom.Dragged(new(e.X - panStart.X, e.Y - panStart.Y), panBeside); return; } diff --git a/src/DiffEngineViewer.Windows/ViewerForm.cs b/src/DiffEngineViewer.Windows/ViewerForm.cs index f2ec6fb8..2f46b763 100644 --- a/src/DiffEngineViewer.Windows/ViewerForm.cs +++ b/src/DiffEngineViewer.Windows/ViewerForm.cs @@ -846,7 +846,10 @@ public ViewerInput Drain() ZoomDelta: zoomDelta, PanX: pan?.X ?? -1, PanY: pan?.Y ?? -1, - RightClickedPane: next.RightClickedPane); + RightClickedPane: next.RightClickedPane, + // In the taskbar, where FormsViewerWindow.FrameWait already slows this head's own + // frames. A window behind another is not asked about: Windows has no one answer for it. + Unseen: WindowState == FormWindowState.Minimized); scrollTo = -1; scrollDelta = 0; diff --git a/src/DiffEngineViewer/AcceptBatch.cs b/src/DiffEngineViewer/AcceptBatch.cs index 86ce3a29..f5df62d3 100644 --- a/src/DiffEngineViewer/AcceptBatch.cs +++ b/src/DiffEngineViewer/AcceptBatch.cs @@ -43,6 +43,13 @@ Only is null || /// public int Kept { get; init; } + /// + /// Whether any of is a delete left because of a move onto its file + /// (), for the batch to say so when it is done: nothing + /// failed, and a count of what was kept reads as though something had. + /// + public bool KeptForAMove { get; init; } + /// /// The entry claimed and being applied outside the session's lock, as it was when claimed, /// which is what recording the outcome checks the queue against. Null between entries. diff --git a/src/DiffEngineViewer/Documents/DocumentWatch.cs b/src/DiffEngineViewer/Documents/DocumentWatch.cs index fdaea6a0..5b9bf0de 100644 --- a/src/DiffEngineViewer/Documents/DocumentWatch.cs +++ b/src/DiffEngineViewer/Documents/DocumentWatch.cs @@ -53,8 +53,9 @@ sealed class DocumentWatch(SessionHost host, DocumentPlugin documents) public TimeSpan Timeout { get; init; } = TimeSpan.FromMinutes(2); /// - /// Whether the window is hidden, set by the render loop as it hides and shows it. Nothing is read - /// or drawn for a window nobody can see, which a tray-hidden viewer is for whole test runs. + /// Whether the window is hidden, set by the render loop as it hides and shows it, and as its + /// head says it is minimised or covered. Nothing is read or drawn for a window nobody can see, + /// which a tray-hidden viewer is for whole test runs. /// public bool Hidden { get; set; } diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index ad06545e..d96de528 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -127,7 +127,12 @@ ViewerResponse IQueueOwner.Listing(bool withPatches) .ToList(), deletes: queue .Where(_ => _.Kind == QueueEntryKind.Delete) - .Select(_ => new ViewerResponseDelete(_.Key, _.Name, _.Solution, _.LeftFile!)) + .Select(_ => new ViewerResponseDelete(_.Key, _.Name, _.Solution, _.LeftFile!) + { + // As a tray owner says it, so whoever shows this queue leaves the delete out + // of a bulk accept for the reason this process's own batch would + Held = ViewerSession.HeldReason(queue, _) + }) .ToList(), progress: state.ListedProgress); } diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs index 9c935772..7cf176e8 100644 --- a/src/DiffEngineViewer/Ipc/OwnerLink.cs +++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs @@ -98,12 +98,34 @@ public void Post(ViewerVerb verb, string? key, string? body = null) => /// none at all - holds the deletes too, because a delete is the one thing not safe to guess /// about. The deletes held are still queued, to be accepted on their own. /// + /// + /// And never a delete the owner holds because a move wrote its file, or is still to + /// (). The owner carries out an accept by key as asked, + /// held or not, since that is how a reviewer says the file is redundant; a group accept is a + /// bulk accept and says no such thing, so the key is not sent, which is what the owner's own + /// accept-all does with it. Left out here rather than asked of the owner through a verb for a + /// group, because the listing already says which they are, and an owner that predates the + /// hold would not know the verb either: it reports no holds and is sent every key, as before. + /// /// public void PostAcceptGroup(IReadOnlyList moves, IReadOnlyList snapshots, IReadOnlyList deletes) => Enqueue(() => AcceptGroup(moves, snapshots, deletes)); public const string DeletesHeld = "Deletes kept: a snapshot in this group was not written, and a file being deleted may be the only copy of it left. Accept them on their own to delete them anyway."; + /// + /// What a group accept says when it left out a delete the owner holds: see + /// . + /// + public const string DeletesKept = "Deletes kept: a move was accepted onto the same file, or is still to be, and deleting it would remove what the move put there. Accept them on their own to delete them anyway."; + + /// + /// The deletes of the last listing, for a group accept to ask which of them the owner holds. + /// Written by the loop and read by a send, which run on different threads, so it is only ever + /// replaced whole. + /// + volatile IReadOnlyList listedDeletes = []; + void Enqueue(Func send) { outbound.Enqueue(send); @@ -135,12 +157,26 @@ string AcceptGroup(IReadOnlyList moves, IReadOnlyList snapshots, return DeletesHeld; } + // Asked now rather than when the group was posted: the listing held is the latest there + // is, and a delete waiting on one of this group's moves is held before that move is + // accepted and after it, for one reason and then the other + var held = listedDeletes + .Where(_ => _.Held is not null) + .Select(_ => _.Key) + .ToHashSet(); + var kept = false; foreach (var key in deletes) { + if (held.Contains(key)) + { + kept = true; + continue; + } + message = Send(new(ViewerVerb.Accept, key, null)); } - return message; + return kept ? DeletesKept : message; } public bool Pump() => @@ -218,6 +254,7 @@ held is not null && : null; } + listedDeletes = response.Deletes; var changes = ReadChanges(response); host.Mutate(_ => ViewerSession.Sync(_, pending, changes, message, response.Progress)); @@ -378,14 +415,24 @@ List ReadChanges(ViewerResponse response) held = null; } - changes.Add(Read( + var entry = Read( held, () => QueueEntry.ForDelete( delete.Key, delete.Name, delete.Group, delete.File, - FileSide.Read(delete.File, documents)))); + FileSide.Read(delete.File, documents))); + // Why the owner's accept-all would leave it, where it would, said on the entry as a + // failure is. A delete's status is nothing else here: this process applies nothing, + // so nothing of its own is ever recorded against an entry. The same entry when the + // owner says what it said before, for the reason Read gives + if (entry.Status != delete.Held) + { + entry = entry with { Status = delete.Held }; + } + + changes.Add(entry); } return changes; diff --git a/src/DiffEngineViewer/Native/Deview.cs b/src/DiffEngineViewer/Native/Deview.cs index 292399f7..877b3982 100644 --- a/src/DiffEngineViewer/Native/Deview.cs +++ b/src/DiffEngineViewer/Native/Deview.cs @@ -11,7 +11,7 @@ static unsafe partial class Deview /// Must match DEVIEW_VERSION in native/include/deview.h. Bumped whenever the structs change, /// so a stale native library is reported rather than read as garbage. /// - public const int ExpectedVersion = 11; + public const int ExpectedVersion = 12; [LibraryImport(library, EntryPoint = "deview_version")] public static partial int Version(); diff --git a/src/DiffEngineViewer/Native/DeviewStructs.cs b/src/DiffEngineViewer/Native/DeviewStructs.cs index 36b032d0..9b13ff6f 100644 --- a/src/DiffEngineViewer/Native/DeviewStructs.cs +++ b/src/DiffEngineViewer/Native/DeviewStructs.cs @@ -188,6 +188,12 @@ struct DeviewInput /// A right-click over a pane: 0 left, 1 right, -1 when there is none. /// public int RightClickedPane; + + /// + /// 1 while nobody can see the window: minimised, hidden, or wholly covered where the window + /// system says so. A state, reported by every poll, rather than an event. + /// + public int Unseen; } /// diff --git a/src/DiffEngineViewer/Native/NativeViewerWindow.cs b/src/DiffEngineViewer/Native/NativeViewerWindow.cs index c3f31ad9..d61108b5 100644 --- a/src/DiffEngineViewer/Native/NativeViewerWindow.cs +++ b/src/DiffEngineViewer/Native/NativeViewerWindow.cs @@ -140,7 +140,8 @@ public unsafe ViewerInput Poll() ZoomDelta: input.ZoomDelta, PanX: input.PanX, PanY: input.PanY, - RightClickedPane: input.RightClickedPane); + RightClickedPane: input.RightClickedPane, + Unseen: input.Unseen != 0); } public void SetHidden(bool hidden) => diff --git a/src/DiffEngineViewer/QueueEntry.cs b/src/DiffEngineViewer/QueueEntry.cs index 21f53f5d..b72d8ecf 100644 --- a/src/DiffEngineViewer/QueueEntry.cs +++ b/src/DiffEngineViewer/QueueEntry.cs @@ -174,6 +174,14 @@ public bool ShowsProperties(DrawingView drawing) => public bool Conflicted => Variants.Count > 1; + /// + /// On a pending delete this process owns: a move has written its file since the delete was + /// raised, and from then on no bulk accept carries the delete out (see + /// ). Gone when a run raises the delete again, which + /// is a statement made after the write. What TrackedDelete.Written is to the tray. + /// + public bool Written { get; init; } + /// /// Whether one side of an entry holds what a side that arrived, or was read again, holds. /// diff --git a/src/DiffEngineViewer/TrackedEntry.cs b/src/DiffEngineViewer/TrackedEntry.cs index b4286178..1e51159f 100644 --- a/src/DiffEngineViewer/TrackedEntry.cs +++ b/src/DiffEngineViewer/TrackedEntry.cs @@ -75,7 +75,20 @@ public static QueueEntry DeleteAgain(QueueEntry queued, string file, DocumentPlu return queued with { LeftStamp = current.Stamp }; } - return QueueEntry.ForDelete(queued.Key, queued.Name, queued.Solution, file, current); + var fresh = QueueEntry.ForDelete(queued.Key, queued.Name, queued.Solution, file, current); + if (!queued.Written) + { + return fresh; + } + + // Held because a move wrote this file, which is the very thing that has it read again + // with something else in it: what it holds is another thing and why it is held is not. + // A run raising the delete again is what lets go of it (ViewerSession.EnqueueTracked) + return fresh with + { + Written = true, + Status = queued.Status + }; } static bool Shows(string text, ImageFile? image, DocumentFile? document, FileSide side) => diff --git a/src/DiffEngineViewer/TrackedWatch.cs b/src/DiffEngineViewer/TrackedWatch.cs index 50a9b08d..f7e7026e 100644 --- a/src/DiffEngineViewer/TrackedWatch.cs +++ b/src/DiffEngineViewer/TrackedWatch.cs @@ -30,7 +30,8 @@ sealed class TrackedWatch(SessionHost host, DocumentPlugin? documents = null) public static TimeSpan Interval { get; set; } = TimeSpan.FromMilliseconds(200); /// - /// Whether the window is hidden, set by the render loop as it hides and shows it. Nobody is + /// Whether the window is hidden, set by the render loop as it hides and shows it, and as its + /// head says it is minimised or covered, which nobody can see either. Nobody is /// reading a hidden window's rows, so its files are looked at as often as an attached viewer /// lists its owner while hidden. Not stopped, because an entry whose received file has gone /// is still pending for as long as it is in the queue, and the queue is what gets staged. diff --git a/src/DiffEngineViewer/ViewerInput.cs b/src/DiffEngineViewer/ViewerInput.cs index 251f7d92..82dee0de 100644 --- a/src/DiffEngineViewer/ViewerInput.cs +++ b/src/DiffEngineViewer/ViewerInput.cs @@ -41,4 +41,8 @@ readonly record struct ViewerInput( double PanY = -1, // A right-click on a pane's text: 0 for the left, 1 for the right, or -1. Opens the menu that // copies from it, which the head hangs where the click landed. - int RightClickedPane = -1); + int RightClickedPane = -1, + // Nobody can see the window: it is minimised, or wholly covered where a head can tell. A + // state rather than something that happened, so it makes no frame one with input in it. The + // loop slows what it keeps going beside the window, as it does for one it hid itself. + bool Unseen = false); diff --git a/src/DiffEngineViewer/ViewerProgram.cs b/src/DiffEngineViewer/ViewerProgram.cs index 7992ff04..58be1bf2 100644 --- a/src/DiffEngineViewer/ViewerProgram.cs +++ b/src/DiffEngineViewer/ViewerProgram.cs @@ -415,7 +415,10 @@ static void Remember(IViewerWindow window, ViewerPreferences preferences) } } - static void Loop( + /// + /// Internal so ViewerProgramTests can hand it the watchers it slows and see them slowed. + /// + internal static void Loop( SessionHost host, IViewerWindow window, OwnerLink? link, @@ -426,6 +429,22 @@ static void Loop( ViewerPreferences preferences, ScreenCache screens) { + // Two reasons nobody is looking at the window, which come and go apart: it was hidden + // from here, by a command or by closing it beside a tray, or its head says nothing of it + // is on screen - minimised, or covered where a head can tell. Either slows what is kept + // going beside it. A minimised window used to be listed, watched and read as one on + // screen, since only hiding it was known here. + var hidden = false; + var unseen = false; + + void Slow() + { + var slow = hidden || unseen; + link?.Hidden = slow; + reader?.Hidden = slow; + watch?.Hidden = slow; + } + while (true) { // Every renderer here is single threaded, so socket driven window changes are applied @@ -439,9 +458,8 @@ static void Loop( var hide = command == WindowCommand.Hide; // A focus shows the window as well as raising it - link?.Hidden = hide; - reader?.Hidden = hide; - watch?.Hidden = hide; + hidden = hide; + Slow(); if (command == WindowCommand.Focus) { @@ -476,6 +494,14 @@ static void Loop( } var input = window.Poll(); + // Said by every poll, and acted on when it changes. Apart from the state, which it is + // no part of: a window going to the taskbar is not a frame with input in it. + if (input.Unseen != unseen) + { + unseen = input.Unseen; + Slow(); + } + // Not on a frame with nothing in it, which is almost all of them. The listener thread // takes the same lock to accept a snapshot, which can wait ten seconds on // InlineApplier's mutex, and taking it every frame put the render loop behind that @@ -527,9 +553,8 @@ static void Loop( { Remember(window, preferences); window.SetHidden(true); - link?.Hidden = true; - reader?.Hidden = true; - watch?.Hidden = true; + hidden = true; + Slow(); continue; } diff --git a/src/DiffEngineViewer/ViewerSession.cs b/src/DiffEngineViewer/ViewerSession.cs index d0933df2..2b2e7593 100644 --- a/src/DiffEngineViewer/ViewerSession.cs +++ b/src/DiffEngineViewer/ViewerSession.cs @@ -161,6 +161,23 @@ public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) return state; } + if (entry.Kind == QueueEntryKind.Move) + { + state = Withdraw(state, entry.TargetFile!); + } + else if (entry.Written) + { + // A delete raised again, built from the entry that was queued for it + // (TrackedEntry.DeleteAgain). Raised by a run that looked at the file as it is now, so + // whatever a move wrote there since the delete was first raised, this is the later + // statement + entry = entry with + { + Written = false, + Status = null + }; + } + var existing = IndexOf(state.Queue, entry.Key); if (existing >= 0 && SameContent(state.Queue[existing], entry)) @@ -191,6 +208,30 @@ public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) return Clamp(next); } + /// + /// Drops the delete pending on a file a move has just arrived for. + /// + /// A move onto a file is a run that verified against it, so a delete an earlier run raised for + /// that file no longer describes a stale one. DiffRunner.SettleDelete says the same thing, but + /// not from a library that predates it, and not while this port was remembered as unowned. + /// The delete stayed, and an accept-all moved the received file into place and then deleted + /// it. The tray's tracker withdraws one for the same reason (Tracker.AddMove). + /// + /// + static SessionState Withdraw(SessionState state, string target) + { + var kept = state.Queue + .Where(_ => _.Kind != QueueEntryKind.Delete || !InlineKey.SamePath(_.LeftFile!, target)) + .ToList(); + if (kept.Count == state.Queue.Count) + { + return state; + } + + // The message is carried, as Refresh carries it: this is not something the reader did + return Remove(state, kept, state.Message); + } + /// /// The entry already queued for a pair that arrived again saying the same thing, with the /// files' new stamps and nothing else about the window changed. @@ -207,10 +248,15 @@ public static SessionState EnqueueTracked(SessionState state, QueueEntry entry) static SessionState Restaged(SessionState state, int index, QueueEntry entry) { var queue = new List(state.Queue); - queue[index] = queue[index] with + var queued = queue[index]; + queue[index] = queued with { LeftStamp = entry.LeftStamp, - RightStamp = entry.RightStamp + RightStamp = entry.RightStamp, + // A delete raised again is no longer held for what a move wrote before it was: see + // EnqueueTracked. Nothing but a delete is ever marked + Written = false, + Status = queued.Written ? null : queued.Status }; return Clamp(state with { @@ -1196,7 +1242,8 @@ static SessionState BeginAccept(SessionState state, IReadOnlyCollection? /// Entries with nothing left to apply are passed over rather than claimed: one that has gone /// since the batch began - settled, discarded, its file taken away - and one a second /// framework has since made a conflict of. A delete is held rather than claimed once a - /// snapshot in the batch was not written. + /// snapshot in the batch was not written, and when a move has written its file or is still + /// to (). /// /// /// A snapshot moving inline arrives as two unrelated entries: the patch that writes the literal @@ -1224,6 +1271,7 @@ public static SessionState ClaimNext(SessionState state) var queue = state.Queue; var kept = batch.Kept; + var keptForAMove = batch.KeptForAMove; for (var position = 0; position < batch.Remaining.Count; position++) { var index = IndexOf(queue, batch.Remaining[position]); @@ -1246,6 +1294,18 @@ public static SessionState ClaimNext(SessionState state) continue; } + // A delete of a file a move wrote, in this batch or before it, or one a move still + // pending is going to write. Carried out, the received file was moved into place and + // then deleted, and neither was left. Asked as its turn comes, so a move the batch + // took first has been recorded by now, and one it has yet to take is still pending + if (HeldReason(queue, entry) is { } held) + { + kept++; + keptForAMove = true; + queue = Replace(queue, index, entry with { Status = held }); + continue; + } + var remaining = batch.Remaining.Skip(position + 1).ToList(); return state with { @@ -1253,6 +1313,7 @@ public static SessionState ClaimNext(SessionState state) Batch = batch with { Kept = kept, + KeptForAMove = keptForAMove, Current = entry, Together = TakeSameFile(queue, entry, remaining), Remaining = remaining @@ -1265,7 +1326,8 @@ public static SessionState ClaimNext(SessionState state) batch with { Remaining = [], - Kept = kept + Kept = kept, + KeptForAMove = keptForAMove }); } @@ -1548,6 +1610,13 @@ static SessionState RecordTracked(SessionState state, QueueEntry entry, string? var claimed = index >= 0 && ReferenceEquals(state.Queue[index], entry); if (failure is null) { + var queue = claimed ? Without(state.Queue, index) : state.Queue; + // Whether or not its entry is still there: the file was written either way + if (!batch.Discarding) + { + queue = MarkWritten(queue, entry); + } + return Remove( state with { @@ -1557,7 +1626,7 @@ state with Current = null } }, - claimed ? Without(state.Queue, index) : state.Queue, + queue, state.Message); } @@ -1602,10 +1671,47 @@ static SessionState Finish(SessionState state, AcceptBatch batch) } } + var message = WithFiles(batch.Tally.Message(conflicted), batch.Swept, batch.Kept); + if (batch.KeptForAMove) + { + message = $"{message}. {DeletesKept}"; + } + return Remove( state with { Batch = null }, state.Queue, - WithFiles(batch.Tally.Message(conflicted), batch.Swept, batch.Kept)); + message); + } + + /// + /// The list once a move has been carried out: a delete pending on the file it wrote is marked + /// as written and says why it is held from here on (). The same list + /// when there is none, or when what was carried out was not a move. + /// + static IReadOnlyList MarkWritten(IReadOnlyList queue, QueueEntry accepted) + { + if (accepted is not { Kind: QueueEntryKind.Move, TargetFile: { } target }) + { + return queue; + } + + List? marked = null; + for (var index = 0; index < queue.Count; index++) + { + var entry = queue[index]; + if (entry is { Kind: QueueEntryKind.Delete, Written: false } && + InlineKey.SamePath(entry.LeftFile!, target)) + { + marked ??= [..queue]; + marked[index] = entry with + { + Written = true, + Status = WroteItsFile + }; + } + } + + return marked ?? queue; } /// @@ -1661,6 +1767,64 @@ static string WithFiles(string message, int swept, int kept) const string deleteHeld = "Held: a snapshot in this batch could not be written inline, and this file may be the only copy of it left. Accept it on its own to delete it anyway."; + /// + /// Why a bulk accept leaves a pending delete where it is, when it would: null when it would + /// carry it out. The tray's Tracker.HeldReason, for a queue this process owns, and what + /// a full listing of that queue says beside the delete. + /// + public static string? HeldReason(IReadOnlyList queue, QueueEntry delete) + { + if (delete.Kind != QueueEntryKind.Delete) + { + return null; + } + + if (delete.Written) + { + return WroteItsFile; + } + + foreach (var entry in queue) + { + if (entry.Kind == QueueEntryKind.Move && + InlineKey.SamePath(entry.TargetFile!, delete.LeftFile!)) + { + return AwaitsItsFile; + } + } + + return null; + } + + /// + /// A move was accepted onto the file while its delete was pending. + /// + /// A move that arrives withdraws the delete it finds waiting for its target + /// (), so the two are only pending together when the delete arrived + /// second, and which of them is the stale one is then not knowable from here. The move is the + /// snapshot arriving and the delete is the last copy leaving, so the move goes ahead and the + /// delete waits to be accepted on its own. + /// + /// + /// Remembered on the delete () rather than for the batch that + /// wrote the file, so that a second accept-all, or one after the move was accepted on its own, + /// does not delete what was just accepted. + /// + /// + public const string WroteItsFile = "Held: a move was accepted onto this file after the delete was raised, so deleting it would remove what was just accepted. Accept it on its own to delete it anyway, or run the tests again."; + + /// + /// A move still pending is going to write the file. Not remembered: it is true for as long as + /// the move is there. + /// + public const string AwaitsItsFile = "Held: a pending move is still to be accepted onto this file. Accept it on its own to delete it anyway."; + + /// + /// What a bulk accept says after its counts when it kept a delete for either reason, since + /// "1 kept" reads as a failure and this is not one. + /// + public const string DeletesKept = "Pending deletes were kept, since a move was accepted onto the same file, or is still to be, and deleting it would remove what the move put there. Accept a delete on its own to delete its file anyway."; + /// /// Accepting a tracked entry is the file operation it describes; discarding one is throwing /// the received file away, or, for a delete, leaving the file alone and only untracking it — @@ -1728,7 +1892,13 @@ static SessionState ApplyTracked( }); } - return Remove(state, state.Queue.Where(_ => _.Key != entry.Key).ToList(), done); + IReadOnlyList left = state.Queue.Where(_ => _.Key != entry.Key).ToList(); + if (!discarding) + { + left = MarkWritten(left, entry); + } + + return Remove(state, left, done); } static SessionState DiscardInline(SessionState state) diff --git a/todo.md b/todo.md index dfb54bee..760bc2f6 100644 --- a/todo.md +++ b/todo.md @@ -42,9 +42,11 @@ An item with no tag was said by whoever made the fix it follows from. A tag says ## Tray -- [ ] "Accept all in" a group from an attached viewer sends one `Accept` per key (`OwnerLink.AcceptGroup`), which carries out a held delete, as accepting it on its own does. -- [ ] A delete held because a move wrote its file is marked in the tray's menu and debug view only. A viewer showing the tray's queue shows it like any other, and the wire's accept-all counts it as kept without saying why: `ViewerResponseDelete` has no field for it. The hold is let go when the delete is raised again, which another framework's process of the same run, having decided before the move was accepted, can do. -- [ ] An owning viewer's own batch looks to have the shape the tray's had: `ViewerSession.EnqueueTracked` replaces by key only, and `BeginAcceptAll` takes moves and deletes in queue order, so a delete can follow the move that wrote its file. (read, not run) +- [ ] The hold on a delete is let go when the delete is raised again, which another framework's process of the same run, having decided before the move was accepted, can do. True of the tray's tracker and of an owning viewer's queue (`ViewerSession.EnqueueTracked`) alike. +- [ ] A tray's listing taken while a move is out of `moves` being accepted, before `MarkWritten`, says the delete on its target is not held. A group accept sent from an attached viewer inside that one poll interval still sends the delete's key, after the move has written the file. `Tracker.HeldReason` does not know of a move in flight. (read, not run) +- [ ] An owning viewer's delete kept by a batch because a move was still pending keeps that status text if the move is later discarded. `ViewerSession.HeldReason`, the listing and the next batch are right; only the row's tooltip is stale. +- [ ] A held delete's row carries the same ` !` mark as a failed entry in every renderer, on an owning and an attached viewer. Only the tooltip tells them apart. +- [ ] A move arriving in an owning viewer withdraws the delete pending on its target, which can be the entry on screen, and closes an open menu as any removal does. The withdrawal and the mark compare paths as the file system does (`InlineKey.SamePath`), case sensitive on Linux, where the tray's rule ignores case throughout. Not run off Windows. - [ ] A batch that begins between Verify raising a delete and queueing its patch can still carry out the delete without the patch. Closing that needs the two tied together on the wire. - [ ] The session ending, which stages the queue and removes the version marker, was exercised by sending `WM_QUERYENDSESSION` and `WM_ENDSESSION` to the window, not by logging off, and its wiring in `Program.Inner` is read, not run. - [ ] A process is believed to be a move's tool only when its command line names the received file. A custom tool that rewrites the path it is given is not tracked as a process: not closed on accept, not counted as open. The command line is read by a call Windows has from 8.1, and was read from PowerShell children only, not a real diff tool. @@ -66,6 +68,9 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - [ ] Past its budget a diff goes by the lines that occur once on each side. Texts made mostly of repeated lines have few, and are still split wherever the search stopped. - [ ] A bulk discard under way in an owning viewer is not on its listings, so an attached window or the tray sees the queue shrink with no progress and refuses nothing meanwhile. A discard-all from the tray during an accept-all now waits for it to end. - [ ] `ScreenBuilder.Build` for an entry of 100,000 lines is 1.5 to 2.5 ms a screen. (noticed, cause not found) +- [ ] The rows a native head reports now depend on its footer, and the status line depends on the rows ("lines 1-N"). At a width where one character of status decides whether it gets a line of its own, the two could alternate frame to frame. Not seen; the WinForms head has always had the same loop. +- [ ] A drag in the pane that can go less far, from a centre beyond its own range, moves only the other pane's picture until the centre is back inside its range. All three heads. +- [ ] `DocumentWatch` does nothing for a window a head reports as unseen, as for a hidden one, so pages are not drawn until it is seen again. On macOS that now includes a wholly covered window: if `occlusionState` is ever wrong, pages stall. - [ ] Both sides of a document are drawn at once, and four things about that could be better: a drawing is not stopped when the reader leaves its entry, though between two pages of a PDF it 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`. - [ ] Which of two PDFs stopped inside PDFium is inferred from whose pages stopped first, not known. A thread descheduled between landing a page and asking for the lock, at the moment the other side hangs, would have the innocent side given up on and the culprit put back. @@ -80,14 +85,14 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - [ ] `ImageCacheTests` and `FormsHeadTests.EveryPictureEverDrawnStaysDecoded` write to fixed folders under the temp folder (`deview-image-cache`, `deview-review-cache`), so two runs of the suite at once on one machine fail each other. (seen) - [ ] In a window about 560 wide the left pane's header runs into the right pane's with no gap. It shows in `FooterThatWraps`. (noticed, not looked into) - [ ] The test that an exception comes out of a message pump has only been seen to pass, since failing it is what shows the dialog. -- [ ] A minimised window slows only its own frame wait. `OwnerLink`, `TrackedWatch` and the document reader go by `Hidden`, which only a Hide command sets. The head would have to tell the loop. +- [ ] A window wholly behind another is not reported as unseen; only a minimised one is. ## Viewer, Linux head -- [ ] The body is not told about a taller footer: a paged document in a window under about 450 px wide needs four footer rows, and the last body rows are then hidden. `rows` in `deview.h` would become the rows the body has room for and the model's eight chrome lines, with `MeasureGrid` here and `Renderer.grid(for:)` on macOS taking off what the footer exceeds its allowance by. No managed change, a `DEVIEW_VERSION` bump, and both binaries built together. +- [ ] The rows reported are capped by what a tall footer leaves the body (ABI 12). The scene that holds it, `ATallFooterTakesRowsFromTheBody`, repeats a file pair's buttons at 1100 px, since the shared test window cannot be made 450 px wide: a real paged document in a narrow window was not run. - [ ] The machine's fonts are drawn, not shaped: Arabic is unjoined and right to left text is in stored order. Colour emoji fonts and CFF2 variable fonts cannot be read by stb_truetype and are passed over, so a machine whose only CJK font is the variable Noto still shows replacement glyphs. At most fifteen fonts are merged. -- [ ] A minimised window is still built and drawn when its screen changes; only a hidden one is left alone. +- [ ] A window behind another is not known to be unseen, since GLFW passes nothing on from X11, so its watchers run as for one on screen. That a minimised window is left alone, and the drag, were checked by hand under Xvfb with openbox and xdotool, not in the suite: CI has neither. - [ ] An idle window still turns sixty times a second: 3 to 9 ms of processor a second. Waiting on the window system instead would take the managed loop being told when to wake. - [ ] Leaving a window alone was run only under Xvfb with Mesa's software rasteriser, with no window manager and under openbox. Not on a GPU, under a compositor, on Wayland or over forwarded X. - [ ] Control held for the wheel to zoom is read from the key's physical state, so a latched Control (sticky keys) scrolls instead. GLFW's scroll callback carries no modifiers. (plausible) @@ -112,11 +117,13 @@ An item with no tag was said by whoever made the fix it follows from. A tag says - Text outside Latin: `--diff` two files of Chinese and hold Down. The same symbol for each frame: the new row alone. - An enlarged picture: `--diff` two 2880 by 1800 screenshots, `+` once, and drag. Time under `Renderer.enlarged` for each frame: a blit, and one 1560 by 975 bitmap a pane about a tenth of a second after the step. - [ ] To confirm on a Mac, from the two rounds of smaller fixes: ten changes, eight of them event handling or window state that no capture exercises. The check for each is in its commit's message. +- [ ] To confirm on a Mac, from the ABI round. CI's job only captures, so a green build exercises none of the three: + - A PDF pair in a window narrow enough for four rows of buttons has its last body row drawn above the footer, and "lines 1-N" in the status drops as the footer grows. + - Miniaturise or wholly cover an owning viewer and rewrite a pending received file: the pane follows about a second later rather than within 200 ms, and at once after the window is uncovered. + - With a wide and a tall image enlarged, drag the tall one to its bottom edge, then drag the wide one sideways: the tall one does not jump. - [ ] A live resize still draws the rows sliced for the old size until the mouse comes up. It 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. -- [ ] 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. The Linux head's item on `rows` is the fix for both. - [ ] A title's width is counted in cells, so a title of characters a fallback font draws wider than a cell can still reach the subtitle. One that fits exactly touches the subtitle with no gap, where Linux keeps a character. - [ ] The zoom keys behind Option were not tried on any layout, and zoom reset (`0`) is matched on `charactersIgnoringModifiers` only. - [ ] An enlarged pair that is the same picture in both panes is still drawn from the picture rather than from a copy, though with a copy kept for each pane it no longer has to be. - [ ] Since macOS 11 a view with an automatic backing store is handed its whole bounds whatever was invalidated, clip included, so the clip test probably leaves nothing out on any supported macOS. A view of the spinner's own would make it pay only if AppKit gives that view a layer of its own, which cannot be checked from here. (read, in Apple's developer forums) -- [ ] 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. Fixing it means moving the centre the frame asked for and clamping to the wider of the two panes' ranges, in both native heads together. -- [ ] 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. +- [ ] 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. That needs a way from the listener thread into AppKit's event loop.