macOS: Cursor leaving traces when scrolling

macOS: Cursor leaving traces when scrolling

[David Reitter]

On Thu, Oct 04, 2018 at 05:33:03PM -0400, David Reitter wrote:
> I back-ported the two macOS Mojave fixes to Emacs 25.
> Please check my handiwork before I push it [1]
> 
> One user reported issues with the cursor leaving traces while
> scrolling [2], which I reproduced on a pre-Mojave machine, but not
> on my Mojave system. I’m unclear whether that is also the case on
> the 26 branch. I can try tomorrow unless someone else can comment.

On Oct 4, 2018, at 5:58 PM, Alan Third <alan@idiocy.org> wrote:
> I believe it’s because we’re copying the contents of the frame on
> scroll without redrawing it without the cursor first. I think that
> means we need to stick a call to NSView displayIfNeeded into
> ns_copy_bits or ns_scroll_run.

It seems to be limited to scrolling, not all cursor movement, so yes.  
Any news on this?

I have other obligations this weekend and cannot reproduce outside of my work desktop, otherwise I’d work out a change.

Re: macOS: Cursor leaving traces when scrolling

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Sat, Oct 06, 2018 at 08:06:29AM -0400, David Reitter wrote:
> On Oct 4, 2018, at 5:58 PM, Alan Third <alan@idiocy.org> wrote:
> > I believe it’s because we’re copying the contents of the frame on
> > scroll without redrawing it without the cursor first. I think that
> > means we need to stick a call to NSView displayIfNeeded into
> > ns_copy_bits or ns_scroll_run.
> 
> It seems to be limited to scrolling, not all cursor movement, so yes.  
> Any news on this?

It seems this is more complicated that I thought. Adding a call to
displayIfNeeded causes the whole frame to flicker and doesn’t always
get rid of the cursor. I’ve no idea why.

The attached patch fixes it, but it marks the whole area dirty instead
of copying it, so it will be redrawn at the next expose event. I can’t
see any difference in simple testing, but it may slow down scrolling
on very complex buffers.
-- 
Alan Third

[-- Attachment #2: 0001-Fix-NS-redraw-errors.patch --]
[-- Type: text/plain, Size: 3593 bytes --]

From e828710d75d1271957375b8b132bef0c4e62a964 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Thu, 4 Oct 2018 22:47:23 +0100
Subject: [PATCH] Fix NS redraw errors

* src/nsterm.m (ns_clip_to_rect, ns_reset_clipping): Always pop the
graphics state.
(ns_copy_bits): Remove function.
(ns_scroll_run):
(ns_shift_glyphs_for_insert): Remove references to ns_copy_bits and
mark changed areas for redraw.
---
 src/nsterm.m | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index d92d6c3244..e4958543eb 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -277,7 +277,6 @@ - (NSColor *)colorUsingDefaultColorSpace
 
 /* display update */
 static int ns_window_num = 0;
-static BOOL gsaved = NO;
 static BOOL ns_fake_keydown = NO;
 #ifdef NS_IMPL_COCOA
 static BOOL ns_menu_bar_is_hidden = NO;
@@ -1180,7 +1179,6 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
             NSRectClipList (r, 2);
           else
             NSRectClip (*r);
-          gsaved = YES;
 
           return YES;
         }
@@ -1204,11 +1202,7 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
 {
   NSTRACE_WHEN (NSTRACE_GROUP_FOCUS, "ns_reset_clipping");
 
-  if (gsaved)
-    {
-      [[NSGraphicsContext currentContext] restoreGraphicsState];
-      gsaved = NO;
-    }
+  [[NSGraphicsContext currentContext] restoreGraphicsState];
 }
 
 
@@ -2707,23 +2701,6 @@ so some key presses (TAB) are swallowed by the system. */
     }
 }
 
-static void
-ns_copy_bits (struct frame *f, NSRect src, NSRect dest)
-{
-  NSTRACE ("ns_copy_bits");
-
-  if (FRAME_NS_VIEW (f))
-    {
-      hide_bell();              // Ensure the bell image isn't scrolled.
-
-      /* FIXME: scrollRect:by: is deprecated in macOS 10.14.  There is
-         no obvious replacement so we may have to come up with our own.  */
-      [FRAME_NS_VIEW (f) scrollRect: src
-                                 by: NSMakeSize (dest.origin.x - src.origin.x,
-                                                 dest.origin.y - src.origin.y)];
-    }
-}
-
 static void
 ns_scroll_run (struct window *w, struct run *run)
 /* --------------------------------------------------------------------------
@@ -2773,10 +2750,12 @@ so some key presses (TAB) are swallowed by the system. */
   x_clear_cursor (w);
 
   {
-    NSRect srcRect = NSMakeRect (x, from_y, width, height);
+    /* We should do this by copying the contents of the NSView,
+       however x_clear_cursor, above, seems to leave detritus since we
+       changed to the mark-dirty/expose method, and simply redrawing
+       the whole thing seems to have no performance issues.  */
     NSRect dstRect = NSMakeRect (x, to_y, width, height);
-
-    ns_copy_bits (f, srcRect , dstRect);
+    [FRAME_NS_VIEW (f) setNeedsDisplayInRect:dstRect];
   }
 
   unblock_input ();
@@ -2830,12 +2809,14 @@ so some key presses (TAB) are swallowed by the system. */
     External (RIF): copy an area horizontally, don't worry about clearing src
    -------------------------------------------------------------------------- */
 {
-  NSRect srcRect = NSMakeRect (x, y, width, height);
+  /* This should be done by copying the contents of the screen,
+     however we can get away with just marking the destination as
+     needing redrawn.  */
   NSRect dstRect = NSMakeRect (x+shift_by, y, width, height);
 
   NSTRACE ("ns_shift_glyphs_for_insert");
 
-  ns_copy_bits (f, srcRect, dstRect);
+  [FRAME_NS_VIEW (f) setNeedsDisplayInRect:dstRect];
 }
 
 
-- 
2.18.0

Re: macOS: Cursor leaving traces when scrolling

[David Reitter]

On Oct 7, 2018, at 7:14 AM, Alan Third <alan@idiocy.org> wrote:

> The attached patch fixes it, but it marks the whole area dirty instead
> of copying it, so it will be redrawn at the next expose event. I can’t
> see any difference in simple testing, but it may slow down scrolling
> on very complex buffers.

Thank you.
I can’t see any obvious difference in speed either on a modern laptop.

NSView scrollRect has been deprecated, so replacing it is a good thing all other things being equal. Implementing true scrolling rather than redrawing would still be worth a try, as display doesn’t seem particularly fast.

I’ll apply it to my own branch and report back if users complain.

Re: macOS: Cursor leaving traces when scrolling

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On Sun, 7 Oct 2018, 19:36 David Reitter, <david.reitter@gmail.com> wrote:

> On Oct 7, 2018, at 7:14 AM, Alan Third <alan@idiocy.org> wrote:
>
> > The attached patch fixes it, but it marks the whole area dirty instead
> > of copying it, so it will be redrawn at the next expose event. I can’t
> > see any difference in simple testing, but it may slow down scrolling
> > on very complex buffers.
>
> Thank you.
> I can’t see any obvious difference in speed either on a modern laptop.
>
> NSView scrollRect has been deprecated, so replacing it is a good thing all
> other things being equal. Implementing true scrolling rather than redrawing
> would still be worth a try, as display doesn’t seem particularly fast.
>
> I’ll apply it to my own branch and report back if users complain.
>

I did eventually find this was a fair bit slower if I made the frame quite
large and zoomed the text out quite a lot, so I suppose it might affect
people using sublimetext style "maps".

I have come up with a possible work around continuing to use scrollRect,
but I've not had time to try implementing it yet.

As you say it would be better to use proper scrolling without scrollRect,
but I don't see any obvious way to do it. I suspect I just don't have
enough knowledge of the cocoa APIs.

[-- Attachment #2: Type: text/html, Size: 1859 bytes --]

Re: macOS: Cursor leaving traces when scrolling

[David Reitter]

On Oct 7, 2018, at 3:31 PM, Alan Third <alan@idiocy.org> wrote:

> As you say it would be better to use proper scrolling without scrollRect, but I don't see any obvious way to do it. I suspect I just don't have enough knowledge of the cocoa APIs.

Remove the EmacsScroller machinery (for the Cocoa API), create a NSScrollView that wraps around the EmacsView (NSView):

[[scrollView contentView]addSubview: self];

Re: macOS: Cursor leaving traces when scrolling

[Alan Third]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Sun, 7 Oct 2018, 22:30 David Reitter, <david.reitter@gmail.com> wrote:

> On Oct 7, 2018, at 3:31 PM, Alan Third <alan@idiocy.org> wrote:
>
> > As you say it would be better to use proper scrolling without
> scrollRect, but I don't see any obvious way to do it. I suspect I just
> don't have enough knowledge of the cocoa APIs.
>
> Remove the EmacsScroller machinery (for the Cocoa API), create a
> NSScrollView that wraps around the EmacsView (NSView):
>
> [[scrollView contentView]addSubview: self];
>

Wouldn't we need one scrollView for each Emacs window? We'd need to hook
into Emacs window resizing and so on. I don't think that's directly exposed
to the NS code.

>

[-- Attachment #2: Type: text/html, Size: 1267 bytes --]

Re: macOS: Cursor leaving traces when scrolling

[David Reitter]

On Oct 7, 2018, at 5:35 PM, Alan Third <alan@idiocy.org> wrote:
> [[scrollView contentView]addSubview: self];
> 
> Wouldn't we need one scrollView for each Emacs window? We'd need to hook into Emacs window resizing and so on. I don't think that's directly exposed to the NS code.


Right.  So right now we have an EmacsView associated with the frame (FRAME_NS_VIEW).  One would want an subview of that for each window, each with its NSScrollView object.  

There could a lot implications to this.

Does the non-Cocoa API include NSScrollView?