From 4c3fbffa4beeb480948e4963de9672ba18bbb408 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 20 Jun 2024 10:31:51 -0700 Subject: [PATCH] macos: return valid selection range --- include/ghostty.h | 1 + .../Sources/Ghostty/SurfaceView_AppKit.swift | 24 +++++++++----- src/Surface.zig | 32 +++++++++++++++++++ src/apprt/embedded.zig | 20 ++++++++++++ 4 files changed, 69 insertions(+), 8 deletions(-) diff --git a/include/ghostty.h b/include/ghostty.h index 8aef893c9..a0160af43 100644 --- a/include/ghostty.h +++ b/include/ghostty.h @@ -557,6 +557,7 @@ uintptr_t ghostty_surface_selection(ghostty_surface_t, char*, uintptr_t); #ifdef __APPLE__ void ghostty_surface_set_display_id(ghostty_surface_t, uint32_t); void* ghostty_surface_quicklook_font(ghostty_surface_t); +void ghostty_surface_selection_range(ghostty_surface_t, uint32_t*, uint32_t*); #endif ghostty_inspector_t ghostty_surface_inspector(ghostty_surface_t); diff --git a/macos/Sources/Ghostty/SurfaceView_AppKit.swift b/macos/Sources/Ghostty/SurfaceView_AppKit.swift index 2abf20d9c..91bc37f35 100644 --- a/macos/Sources/Ghostty/SurfaceView_AppKit.swift +++ b/macos/Sources/Ghostty/SurfaceView_AppKit.swift @@ -931,12 +931,14 @@ extension Ghostty.SurfaceView: NSTextInputClient { func selectedRange() -> NSRange { guard let surface = self.surface else { return NSRange() } - guard ghostty_surface_has_selection(surface) else { return NSRange() } - // If we have a selection, we just return a non-empty range. The actual - // values are meaningless but the non-emptiness of it tells AppKit we - // have a selection. - return NSRange(location: 0, length: 1) + // Get our range from the Ghostty API. There is a race condition between getting the + // range and actually using it since our selection may change but there isn't a good + // way I can think of to solve this for AppKit. + var start: UInt32 = 0; + var len: UInt32 = 0; + ghostty_surface_selection_range(surface, &start, &len); + return NSRange(location: Int(start), length: Int(len)) } func setMarkedText(_ string: Any, selectedRange: NSRange, replacementRange: NSRange) { @@ -961,12 +963,18 @@ extension Ghostty.SurfaceView: NSTextInputClient { } func attributedSubstring(forProposedRange range: NSRange, actualRange: NSRangePointer?) -> NSAttributedString? { - // We ignore the proposed range and always return the selection from - // this (if we have one). This enables features like QuickLook. I don't - // know if this breaks anything else... + Ghostty.logger.warning("pressure substring range=\(range) selectedRange=\(self.selectedRange())") guard let surface = self.surface else { return nil } guard ghostty_surface_has_selection(surface) else { return nil } + // If the range is empty then we don't need to return anything + guard range.length > 0 else { return nil } + + // I used to do a bunch of testing here that the range requested matches the + // selection range or contains it but a lot of macOS system behaviors request + // bogus ranges I truly don't understand so we just always return the + // attributed string containing our selection which is... weird but works? + // Get our selection. We cap it at 1MB for the purpose of this. This is // arbitrary. If this is a good reason to increase it I'm happy to. let v = String(unsafeUninitializedCapacity: 1000000) { diff --git a/src/Surface.zig b/src/Surface.zig index 625e1ddb0..e7ae87fe4 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -849,6 +849,38 @@ pub fn selectionString(self: *Surface, alloc: Allocator) !?[]const u8 { }); } +/// This returns the selection range offset from the beginning of the +/// viewport. If the selection is not entirely within the viewport then +/// this will return null. +/// +/// This is a oddly specific function that is used with macOS to enable +/// NSTextInputClient to work properly for features such as the IME Emoji +/// keyboard and QuickLook amongst other things. +pub fn selectionRange(self: *Surface) ?struct { + start: u32, + len: u32, +} { + self.renderer_state.mutex.lock(); + defer self.renderer_state.mutex.unlock(); + + // Get the TL/BR pins for the selection + const sel = self.io.terminal.screen.selection orelse return null; + const tl = sel.topLeft(&self.io.terminal.screen); + const br = sel.bottomRight(&self.io.terminal.screen); + + // Convert the pins to coordinates (x,y) + const tl_pt = self.io.terminal.screen.pages.pointFromPin(.viewport, tl) orelse return null; + const br_pt = self.io.terminal.screen.pages.pointFromPin(.viewport, br) orelse return null; + const tl_coord = tl_pt.coord(); + const br_coord = br_pt.coord(); + + // Utilize viewport sizing to convert to offsets + const start = tl_coord.y * self.io.terminal.screen.pages.cols + tl_coord.x; + const end = br_coord.y * self.io.terminal.screen.pages.cols + br_coord.x; + + return .{ .start = start, .len = end - start }; +} + /// Returns the pwd of the terminal, if any. This is always copied because /// the pwd can change at any point from termio. If we are calling from the IO /// thread you should just check the terminal directly. diff --git a/src/apprt/embedded.zig b/src/apprt/embedded.zig index 392c3a90b..5726bc1f9 100644 --- a/src/apprt/embedded.zig +++ b/src/apprt/embedded.zig @@ -1803,6 +1803,26 @@ pub const CAPI = struct { return copy; } + /// This returns the start and length of the current selection range + /// in viewport coordinates. If the selection is not visible in the + /// viewport completely then this will return 0,0. This rather odd + /// detail is due to the current usage of this in the macOS app where + /// selections are only meaningful if they're in the viewport. We can + /// change this behavior if we have something useful to do with the + /// selection range outside of the viewport in the future. + export fn ghostty_surface_selection_range( + ptr: *Surface, + start: *u32, + len: *u32, + ) void { + start.* = 0; + len.* = 0; + + const range = ptr.core_surface.selectionRange() orelse return; + start.* = range.start; + len.* = range.len; + } + export fn ghostty_inspector_metal_init(ptr: *Inspector, device: objc.c.id) bool { return ptr.initMetal(objc.Object.fromId(device)); }