Skip to content

Commit 6385b43

Browse files
authored
Fix six things in the native heads (#846)
* Fix five things in the native heads Escape with the Linux context menu open reached the managed side as quit, which closes the menu and then runs the command, so dismissing the menu closed the viewer - and on Linux there is no tray to open it again from, so the queue went to staging. A click outside the menu reported nothing at all, leaving it floating until a row, a button or a key was hit, which is not what docs/viewer.md says of it. Both are dismissals now. WindowShouldClose waits on events while the window is minimised, and it is called inside deview_present, so minimising the Linux viewer stopped the managed loop being pumped: a snapshot arriving after that was accepted by the listener and never shown. FLAG_WINDOW_ALWAYS_RUN keeps the loop running, and a focus restores a minimised window rather than leaving it in the taskbar. The ImGui backend did not declare RendererHasVtxOffset and the renderer ignored the vertex offset, so a draw list past 65535 vertices - a maximised 4K window of dense long lines reaches that - wrapped its sixteen bit indices and drew scrambled panes, with IM_ASSERT compiled out of the release build to say nothing about it. A failed queue row on Linux was coloured and left unmarked, while the other three heads and the docs show " !" after the label. Trackpad scrolling: the Linux head truncated fractional wheel offsets to zero, and the Mac head rounded points to notches - so an ordinary flick, which is tens of points, arrived as tens of notches and the managed side multiplied it by three, while slow movement rounded away to nothing. Both accumulate now, and macOS converts points and lines separately. macOS tooltips: refreshToolTips removed and re-added every tracking rectangle on every frame, and AppKit times its tooltip delay from the moment the cursor enters one, so the delay was restarted before it could elapse and queue tooltips never appeared. The rectangles are rebuilt only when they change. The binaries these compile into are committed, so the build-native workflow's rebuild has to land on this branch before it is merged. * Carry the pending count across the ABI The macOS head derived "Pending (N)" from the queue it was handed, which is the visible slice: sized to the body, and with the members of folded groups left out. So thirty pending in a sixteen row body read "Pending (16)" beside "inline 1 of 30", and folding a group lowered it further. The ASCII and WinForms heads use Screen.PendingCount, which the shim had no field for. DeviewScreen carries it now, so DEVIEW_VERSION goes to 7 and the binaries have to be rebuilt with this. DeviewStructTests holds the two sides together and is the part of this a machine with no toolchain can still check. * Rebuild native renderer binaries --------- Co-authored-by: SimonCropp <122666+SimonCropp@users.noreply.github.com>
1 parent e6de342 commit 6385b43

12 files changed

Lines changed: 137 additions & 17 deletions

File tree

native/include/deview.h

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,13 @@ typedef struct DeviewScreen {
127127
const DeviewQueueItem* queue;
128128
int32_t queueCount;
129129

130+
/*
131+
* How many entries are pending in total, which is not queueCount: that is the visible slice,
132+
* sized to the body and with the members of folded groups left out. A head that counted the
133+
* slice reported "Pending (16)" beside "inline 1 of 30", and folding a group lowered it.
134+
*/
135+
int32_t pendingCount;
136+
130137
int32_t titleOffset;
131138
int32_t titleLength;
132139
int32_t subtitleOffset;
@@ -215,7 +222,7 @@ typedef struct DeviewInput {
215222
* described. A widened array element, so an older library reads every pane after the first at
216223
* the wrong offset — this is the bump that matters most to honour.
217224
*/
218-
#define DEVIEW_VERSION 6
225+
#define DEVIEW_VERSION 7
219226

220227
/*
221228
* The Swift implementation imports this header for the struct layouts, because Swift does not

native/src/deview.cpp

Lines changed: 76 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,15 @@ struct State
110110
ImGuiContext* context = nullptr;
111111
DeviewInput input{};
112112

113+
/* Whether the last screen carried a context menu, which is what makes Escape and a click
114+
* outside it a dismissal rather than what they would otherwise mean. */
115+
bool menuOpen = false;
116+
117+
/* What a wheel message left over. A notch is 1.0, and a touchpad sends fractions of one:
118+
* truncating each frame's value on its own threw all of them away, so a touchpad scrolled
119+
* nothing at all. */
120+
float scrollRemainder = 0.0f;
121+
113122
/*
114123
* The queue column, owned here rather than by the table.
115124
*
@@ -384,6 +393,7 @@ void UpdateTexture(ImTextureData* texture)
384393
void RenderTriangles(
385394
unsigned int count,
386395
unsigned int indexStart,
396+
unsigned int vertexOffset,
387397
const ImVector<ImDrawIdx>& indices,
388398
const ImVector<ImDrawVert>& vertices,
389399
ImTextureID textureId)
@@ -400,7 +410,12 @@ void RenderTriangles(
400410
{
401411
for (unsigned int corner = 0; corner < 3; corner++)
402412
{
403-
const ImDrawVert& vertex = vertices[indices[indexStart + index + corner]];
413+
/* Plus the command's own vertex offset. ImDrawIdx is sixteen bits, so a draw list
414+
* that runs past 65535 vertices - a maximised 4K window of dense long lines gets
415+
* there - is split by ImGui into commands whose indices restart from a base recorded
416+
* here. Without adding it the indices wrapped and the panes drew scrambled, and in a
417+
* release build, with IM_ASSERT compiled out, nothing said so. */
418+
const ImDrawVert& vertex = vertices[vertexOffset + indices[indexStart + index + corner]];
404419
const ImColor colour = ImColor(vertex.col);
405420
rlColor4f(colour.Value.x, colour.Value.y, colour.Value.z, colour.Value.w);
406421
rlTexCoord2f(vertex.uv.x, vertex.uv.y);
@@ -448,6 +463,7 @@ void RenderDrawData(ImDrawData* drawData)
448463
RenderTriangles(
449464
command.ElemCount,
450465
command.IdxOffset,
466+
command.VtxOffset,
451467
commands->IdxBuffer,
452468
commands->VtxBuffer,
453469
command.GetTexID());
@@ -695,6 +711,10 @@ void DrawPaneImage(const DeviewScreen* screen, const DeviewPane& pane, const Pan
695711

696712
void BuildFrame(const DeviewScreen* screen)
697713
{
714+
/* Read by the input pass, which has no screen of its own: Escape means dismiss while one of
715+
* these is up, and quit otherwise. */
716+
state.menuOpen = screen->menuCount > 0;
717+
698718
const ImGuiViewport* viewport = ImGui::GetMainViewport();
699719
ImGui::SetNextWindowPos(viewport->WorkPos);
700720
ImGui::SetNextWindowSize(viewport->WorkSize);
@@ -785,7 +805,7 @@ void BuildFrame(const DeviewScreen* screen)
785805
if (index < screen->queueCount)
786806
{
787807
const DeviewQueueItem& item = screen->queue[index];
788-
const std::string label = Copy(screen, item.labelOffset, item.labelLength);
808+
std::string label = Copy(screen, item.labelOffset, item.labelLength);
789809
if (item.flags & DEVIEW_QUEUE_HEADER)
790810
{
791811
/* A heading is dimmed like the subtitle, and never carries the selection.
@@ -805,9 +825,14 @@ void BuildFrame(const DeviewScreen* screen)
805825
else
806826
{
807827
const bool selected = (item.flags & DEVIEW_QUEUE_SELECTED) != 0;
808-
if (item.flags & DEVIEW_QUEUE_FAILED)
828+
const bool failed = (item.flags & DEVIEW_QUEUE_FAILED) != 0;
829+
if (failed)
809830
{
810831
ImGui::PushStyleColor(ImGuiCol_Text, RowColour(DEVIEW_ROW_REMOVED));
832+
/* The marker the other three heads and docs/viewer.md show. Colour
833+
* alone says nothing to a reader who cannot tell this red from the
834+
* one a removed line is drawn in, or from any other. */
835+
label += " !";
811836
}
812837

813838
ImGui::PushID(index);
@@ -817,7 +842,7 @@ void BuildFrame(const DeviewScreen* screen)
817842
}
818843

819844
ImGui::PopID();
820-
if (item.flags & DEVIEW_QUEUE_FAILED)
845+
if (failed)
821846
{
822847
ImGui::PopStyleColor();
823848
}
@@ -981,9 +1006,22 @@ void BuildFrame(const DeviewScreen* screen)
9811006
ImGui::PopID();
9821007
}
9831008

1009+
/* Asked before End, which is what makes it about this window. A click anywhere else is a
1010+
* dismissal: the menu used to float until a row, a button or a key was hit, contrary to
1011+
* what docs/viewer.md says of it. A right click elsewhere opens the next menu, and the
1012+
* managed side ignores a dismissal that arrives with one of those. */
1013+
const bool overMenu = ImGui::IsWindowHovered(ImGuiHoveredFlags_ChildWindows);
1014+
9841015
ImGui::End();
9851016
ImGui::PopStyleVar();
9861017
ImGui::PopStyleColor();
1018+
1019+
if (!overMenu &&
1020+
(ImGui::IsMouseClicked(ImGuiMouseButton_Left) ||
1021+
ImGui::IsMouseClicked(ImGuiMouseButton_Right)))
1022+
{
1023+
state.input.menuClosed = 1;
1024+
}
9871025
}
9881026

9891027
for (int index = 0; index < screen->buttonCount; index++)
@@ -1070,7 +1108,11 @@ int32_t deview_init(
10701108
/* No MSAA. ImGui draws axis aligned quads with pre-antialiased glyph textures, so multisampling
10711109
* buys nothing visually, and it is a real source of difference between a GPU and the software
10721110
* rasteriser the pixel snapshots are pinned to. */
1073-
unsigned int flags = FLAG_WINDOW_RESIZABLE;
1111+
/* ALWAYS_RUN because WindowShouldClose waits on events while the window is minimised, and
1112+
* that call is inside deview_present: without it the managed loop stops being pumped the
1113+
* moment the window is minimised, so a snapshot arriving after that is accepted by the
1114+
* listener and never shown. */
1115+
unsigned int flags = FLAG_WINDOW_RESIZABLE | FLAG_WINDOW_ALWAYS_RUN;
10741116
if (hidden != 0)
10751117
{
10761118
flags |= FLAG_WINDOW_HIDDEN;
@@ -1090,6 +1132,10 @@ int32_t deview_init(
10901132
ImGui::SetCurrentContext(state.context);
10911133
ImGuiIO& io = ImGui::GetIO();
10921134
io.BackendFlags |= ImGuiBackendFlags_RendererHasTextures;
1135+
/* Declared, so ImGui splits a long draw list into commands with a vertex offset rather than
1136+
* refusing to let one grow past what a sixteen bit index can address. RenderTriangles applies
1137+
* the offset. */
1138+
io.BackendFlags |= ImGuiBackendFlags_RendererHasVtxOffset;
10931139
io.IniFilename = nullptr;
10941140
io.LogFilename = nullptr;
10951141
ApplyStyle();
@@ -1164,8 +1210,25 @@ void deview_poll_input(DeviewInput* input)
11641210
if (state.initialised)
11651211
{
11661212
state.input.key = ReadKey();
1213+
1214+
/* Escape with a menu up dismisses the menu. It reached the managed side as quit, which
1215+
* closes the menu and then runs the command, so Esc-to-dismiss closed the viewer - and on
1216+
* Linux there is no tray to open it again from, so the queue went to staging. */
1217+
if (state.menuOpen &&
1218+
state.input.key == DEVIEW_KEY_QUIT &&
1219+
IsKeyPressed(KEY_ESCAPE))
1220+
{
1221+
state.input.key = DEVIEW_KEY_NONE;
1222+
state.input.menuClosed = 1;
1223+
}
1224+
1225+
/* Whole notches, keeping the fraction. A touchpad sends a fraction of one per frame and
1226+
* truncating each frame on its own threw every one of them away. */
11671227
const Vector2 wheel = GetMouseWheelMoveV();
1168-
state.input.scrollDelta = static_cast<int32_t>(wheel.y);
1228+
state.scrollRemainder += wheel.y;
1229+
const int32_t notches = static_cast<int32_t>(state.scrollRemainder);
1230+
state.input.scrollDelta = notches;
1231+
state.scrollRemainder -= static_cast<float>(notches);
11691232
MeasureGrid();
11701233
}
11711234

@@ -1252,6 +1315,13 @@ void deview_focus(void)
12521315
}
12531316

12541317
ClearWindowState(FLAG_WINDOW_HIDDEN);
1318+
/* A minimised window stays minimised through SetWindowFocused, so a focus for a new snapshot
1319+
* left it in the taskbar. */
1320+
if (IsWindowMinimized())
1321+
{
1322+
RestoreWindow();
1323+
}
1324+
12551325
SetWindowFocused();
12561326
}
12571327

native/swift/Sources/Deview/Frame.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ struct Frame {
1010
var subtitle = ""
1111
var status = ""
1212
var queue: [QueueItem] = []
13+
14+
/// Everything pending, which `queue` is not: that is the visible slice, sized to the body and
15+
/// with the members of folded groups left out.
16+
var pendingCount: Int32 = 0
17+
1318
var buttons: [Button] = []
1419
var left = Pane()
1520
var right = Pane()
@@ -63,6 +68,7 @@ struct Frame {
6368
frame.title = string(screen, screen.titleOffset, screen.titleLength)
6469
frame.subtitle = string(screen, screen.subtitleOffset, screen.subtitleLength)
6570
frame.status = string(screen, screen.statusOffset, screen.statusLength)
71+
frame.pendingCount = screen.pendingCount
6672

6773
if let items = screen.queue {
6874
for index in 0 ..< Int(screen.queueCount) {

native/swift/Sources/Deview/Renderer.swift

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,10 @@ final class Renderer {
172172

173173
let headerTop = firstRule + Renderer.gap
174174
if hasQueue {
175-
// Entries only: the rows include group headings, which are not pending anything.
176-
let pending = frame.queue.filter { !$0.header }.count
177-
text("Pending (\(pending))", in: rect(top: headerTop, left: Renderer.padding, width: queue, height: line, size), Palette.text, context)
175+
// The count the managed side carries, not one derived from `queue`: that is the
176+
// visible slice, so thirty pending in a sixteen row body read as "Pending (16)" beside
177+
// "inline 1 of 30", and folding a group lowered it further.
178+
text("Pending (\(frame.pendingCount))", in: rect(top: headerTop, left: Renderer.padding, width: queue, height: line, size), Palette.text, context)
178179
}
179180

180181
text(frame.left.header, in: rect(top: headerTop, left: panesLeft, width: half, height: line, size), Palette.text, context)

native/swift/Sources/Deview/ViewerView.swift

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,28 @@ final class ViewerView: NSView, NSViewToolTipOwner {
5252
///
5353
/// The text is answered on demand below rather than stored here, so a row whose label changed
5454
/// under a resting cursor still reads correctly.
55+
/// Rebuilt only when the regions themselves changed. AppKit times its tooltip delay from the
56+
/// moment the cursor enters a tracking rectangle, and this runs on every frame, so removing
57+
/// and re-adding the rectangle under a resting cursor restarted that delay before it could
58+
/// ever elapse - which is to say queue tooltips never appeared on macOS at all.
5559
func refreshToolTips() {
60+
let wanted = layout.queueItems.enumerated()
61+
.filter { $0.offset < model.queue.count && !model.queue[$0.offset].tooltip.isEmpty }
62+
.map(\.element)
63+
guard wanted != toolTipRects else {
64+
return
65+
}
66+
67+
toolTipRects = wanted
5668
removeAllToolTips()
57-
for (index, bounds) in layout.queueItems.enumerated()
58-
where index < model.queue.count && !model.queue[index].tooltip.isEmpty {
69+
for bounds in wanted {
5970
_ = addToolTip(bounds, owner: self, userData: nil)
6071
}
6172
}
6273

74+
/// What the tips are registered on, so an unchanged frame can leave them alone.
75+
private var toolTipRects: [NSRect] = []
76+
6377
/// Composed by the managed side, so this only finds the row under the cursor.
6478
func view(_ view: NSView, stringForToolTip tag: NSView.ToolTipTag, point: NSPoint, userData: UnsafeMutableRawPointer?) -> String {
6579
guard let index = layout.queueItems.firstIndex(where: { $0.contains(point) }),
@@ -132,12 +146,26 @@ final class ViewerView: NSView, NSViewToolTipOwner {
132146
super.rightMouseDown(with: event)
133147
}
134148

135-
/// Accumulated, because a trackpad delivers many small deltas between two polls and the
136-
/// managed side amplifies whatever it is given.
149+
/// A notch of a wheel, in the points a precise device reports one movement of it as.
150+
///
151+
/// AppKit reports a wheel in lines and a trackpad in points, and rounding both to an integer
152+
/// number of notches treated them as the same thing: an ordinary flick of a trackpad reads as
153+
/// tens of points, so it arrived as tens of notches and the managed side then multiplied it
154+
/// by three. Slow movement rounded to nothing at all.
155+
private static let pointsPerNotch = 16.0
156+
157+
/// What is left over between events, because a trackpad delivers many small deltas between
158+
/// two polls and dropping each one on its own is what made slow movement do nothing.
159+
private var scrollRemainder = 0.0
160+
137161
override func scrollWheel(with event: NSEvent) {
138-
let notches = Int32(event.scrollingDeltaY.rounded())
162+
scrollRemainder += event.hasPreciseScrollingDeltas
163+
? event.scrollingDeltaY / ViewerView.pointsPerNotch
164+
: event.scrollingDeltaY
165+
let notches = scrollRemainder.rounded(.towardZero)
166+
scrollRemainder -= notches
139167
if notches != 0 {
140-
Runtime.shared.input.scrollDelta += notches
168+
Runtime.shared.input.scrollDelta += Int32(notches)
141169
}
142170
}
143171

Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.

src/DiffEngineViewer/Native/Deview.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ static unsafe partial class Deview
1010
/// Must match DEVIEW_VERSION in native/include/deview.h. Bumped whenever the structs change,
1111
/// so a stale native library is reported rather than read as garbage.
1212
/// </summary>
13-
public const int ExpectedVersion = 6;
13+
public const int ExpectedVersion = 7;
1414

1515
[LibraryImport(library, EntryPoint = "deview_version")]
1616
public static partial int Version();

0 commit comments

Comments
 (0)