From: Sungbin Jo <goranmoomin@daum.net>
To: Po Lu <luangruo@yahoo.com>
Cc: 58271@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
Subject: bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully
Date: Tue, 4 Oct 2022 11:03:28 +0900 [thread overview]
Message-ID: <D75F5E89-B57D-4018-839A-222B6BDF000B@daum.net> (raw)
In-Reply-To: <87r0zo1l5e.fsf@yahoo.com>
[-- Attachment #1: Type: text/plain, Size: 3237 bytes --]
2022. 10. 4. 오전 9:34, Po Lu <luangruo@yahoo.com> 작성:
> Thanks; since you seem to be the original author of the xwidget code on
> Mac OS, could you please fix the crash there when an xwidget is deleted
> but remains on-screen?
I’ve continued on this (with some questions) on emacs-devel.
> Some minor formatting comments on the patch below:
Updated patch attached.
>> From: VirtualBuddy <virtualbuddy@monterey-vm.local>
>
> Is this a real email?
No, it was a mistake. Updated.
>> * etc/NEWS: Mention fix.
>
> Since this is a bug fix, I see no reason to mention it in NEWS. In
> addition, even if it warranted a mention, it ought to be placed under
> "Changes in Emacs 29.1 on Non-Free Operating Systems".
I did place it under the “Non-Free Operating Systems” part, but I removed them.
>> * src/nsxwidget.h: Remove now-unused NSView subclasses and functions.
>> * src/nsxwidget.m:
>> ([XwWebView mouseDown:], [XwWebView mouseUp:], [XwWebView keyDown:])
>> ([XwWebView userContentController:didReceiveScriptMessage:]): Rename field of
>> xwidget_view from emacswindow to emacsFrame to better match emacs terminology.
>> (nsxwidget_init, nsxwidget_resize_view, nsxwidget_move_widget_in_view):
>> Simplify logic by removing field xwWindow and using the xvWindow as the
>> container.
>> (nsxwidget_resize, XwWindow, XvWindow): Remove now-unused code.
>> (nsxwidget_init_view, nsxwidget_delete_view): Handle creating non-primary
>> xwidget views.
>> (nsxwidget_show_view, nsxwidget_hide_view): Remove poor hack to hide views.
>> * src/xwidget.c (xwidget_init_view): Update formatting.
>> (x_draw_xwidget_glyph_string): Handle displaying non-primary xwidget views and
>> remove previous message warning.
>> (Fxwidget_resize): Remove useless call to nsxwidget_resize, as the subsequent
>> redisplay handles them via nsxwidget_resize_view.
>> * src/xwidget.h (struct xwidget): Remove field xwWindow and update comments
>> to be more accurate.
>> (struct xwidget_view): Add field xvWidget and rename field emacswindow to
>> emacsFrame to better match emacs terminology.
>
> Please make sure that each line of the commit message is no longer than
> 64 characters in length.
Done.
>> {
>> - [self.xw->xv->emacswindow mouseDown:event];
>> + [self.xw->xv->emacsFrame mouseDown:event];
>> [super mouseDown:event];
>> }
>>
>> - (void)mouseUp:(NSEvent *)event
>> {
>> - [self.xw->xv->emacswindow mouseUp:event];
>> + [self.xw->xv->emacsFrame mouseUp:event];
>> [super mouseUp:event];
>> }
>
> The "emacswindow" field should actually be named "frame", IMHO.
I’m not sure I follow; that’s exactly the rename I’ve done. Or are you
suggesting that I should use ‘frame’ instead of ‘emacsFrame’? In any
case, I’m fine with either.
>> + {
>> + NSTextField *warningLabel = [NSTextField labelWithString:@"Cocoa Xwidgets do not support sharing widgets."];
>
> This line is too long. Please find a way to make it fit in 80 columns.
Done.
>> + if (xw->xv == xv)
>> + {
>> + xw->xv = NULL; /* Now model has no view. */
>> + }
>
> Please remove the unnecessary braces here.
Done.
[-- Attachment #2: 0001-Handle-sharing-Cocoa-xwidgets-more-gracefully.patch --]
[-- Type: application/octet-stream, Size: 11861 bytes --]
From 2f90ceb0d2e556cad2802cdca5ef8324a8dc5523 Mon Sep 17 00:00:00 2001
From: Sungbin Jo <goranmoomin@daum.net>
Date: Tue, 4 Oct 2022 08:51:33 +0900
Subject: [PATCH] Handle sharing Cocoa xwidgets more gracefully
* src/nsxwidget.h (nsxwidget_resize, XwWindow, XvWindow): Remove
unused subclasses and functions.
* src/nsxwidget.m:
([XwWebView mouseDown:], [XwWebView mouseUp:])
([XwWebView keyDown:])
([XwWebView userContentController:didReceiveScriptMessage:]):
Rename field of xwidget_view from emacswindow to emacsFrame to
better match emacs terminology.
(nsxwidget_init, nsxwidget_resize_view)
(nsxwidget_move_widget_in_view): Simplify logic by removing
field xwWindow and using the xvWindow as the container.
(nsxwidget_resize, XwWindow, XvWindow): Remove unused subclasses
and functions.
(nsxwidget_init_view, nsxwidget_delete_view): Handle creating
non-primary xwidget views.
(nsxwidget_show_view, nsxwidget_hide_view): Remove hack to hide
views.
* src/xwidget.c (xwidget_init_view): Update formatting.
(x_draw_xwidget_glyph_string): Handle displaying non-primary
xwidget views and remove previous message warning.
(Fxwidget_resize): Remove useless call to nsxwidget_resize, as
the subsequent redisplay handles them via nsxwidget_resize_view.
* src/xwidget.h (struct xwidget): Remove field xwWindow and
update comments to be more accurate.
(struct xwidget_view): Add field xvWidget and rename field
emacswindow to emacsFrame to better match emacs terminology.
---
src/nsxwidget.h | 14 --------
src/nsxwidget.m | 86 ++++++++++++++++++++-----------------------------
src/xwidget.c | 11 ++-----
src/xwidget.h | 15 ++++-----
4 files changed, 44 insertions(+), 82 deletions(-)
diff --git a/src/nsxwidget.h b/src/nsxwidget.h
index 666509744a..f7c744d405 100644
--- a/src/nsxwidget.h
+++ b/src/nsxwidget.h
@@ -42,26 +42,12 @@ #define NSXWIDGET_H_INCLUDED
/* Functions for xwidget model. */
-#ifdef __OBJC__
-@interface XwWindow : NSView
-@property struct xwidget *xw;
-@end
-#endif
-
void nsxwidget_init (struct xwidget *xw);
void nsxwidget_kill (struct xwidget *xw);
-void nsxwidget_resize (struct xwidget *xw);
Lisp_Object nsxwidget_get_size (struct xwidget *xw);
/* Functions for xwidget view. */
-#ifdef __OBJC__
-@interface XvWindow : NSView
-@property struct xwidget *xw;
-@property struct xwidget_view *xv;
-@end
-#endif
-
void nsxwidget_init_view (struct xwidget_view *xv,
struct xwidget *xww,
struct glyph_string *s,
diff --git a/src/nsxwidget.m b/src/nsxwidget.m
index be0eba0bcb..2a6d5cdb54 100644
--- a/src/nsxwidget.m
+++ b/src/nsxwidget.m
@@ -199,13 +199,13 @@ - (void)webView:(WKWebView *)webView
- (void)mouseDown:(NSEvent *)event
{
- [self.xw->xv->emacswindow mouseDown:event];
+ [self.xw->xv->emacsFrame mouseDown:event];
[super mouseDown:event];
}
- (void)mouseUp:(NSEvent *)event
{
- [self.xw->xv->emacswindow mouseUp:event];
+ [self.xw->xv->emacsFrame mouseUp:event];
[super mouseUp:event];
}
@@ -220,15 +220,15 @@ - (void)keyDown:(NSEvent *)event
Lisp_Object val = buffer_local_value (var, Fcurrent_buffer ());
if (!EQ (val, Qunbound) && !EQ (val, Qnil))
{
- [self.window makeFirstResponder:self.xw->xv->emacswindow];
- [self.xw->xv->emacswindow keyDown:event];
+ [self.window makeFirstResponder:self.xw->xv->emacsFrame];
+ [self.xw->xv->emacsFrame keyDown:event];
return;
}
/* Emacs handles keyboard events when javascript is blocked. */
if ([self.urlScriptBlocked[self.URL] boolValue])
{
- [self.xw->xv->emacswindow keyDown:event];
+ [self.xw->xv->emacsFrame keyDown:event];
return;
}
@@ -237,13 +237,13 @@ - (void)keyDown:(NSEvent *)event
if (error)
{
NSLog (@"xwHasFocus: %@", error);
- [self.xw->xv->emacswindow keyDown:event];
+ [self.xw->xv->emacsFrame keyDown:event];
}
else if (result)
{
NSNumber *hasFocus = result; /* __NSCFBoolean */
if (!hasFocus.boolValue)
- [self.xw->xv->emacswindow keyDown:event];
+ [self.xw->xv->emacsFrame keyDown:event];
else
[super keyDown:event];
}
@@ -291,7 +291,7 @@ - (void)userContentController:(WKUserContentController *)userContentController
{
/* Just give up focus, no relay "C-g" to emacs, another "C-g"
follows will be handled by emacs. */
- [self.window makeFirstResponder:self.xw->xv->emacswindow];
+ [self.window makeFirstResponder:self.xw->xv->emacsFrame];
}
}
@@ -437,16 +437,10 @@ - (void)userContentController:(WKUserContentController *)userContentController
}];
}
-/* Window containing an xwidget. */
-
-@implementation XwWindow
-- (BOOL)isFlipped { return YES; }
-@end
-
/* Xwidget model, macOS Cocoa part. */
void
-nsxwidget_init(struct xwidget *xw)
+nsxwidget_init (struct xwidget *xw)
{
block_input ();
NSRect rect = NSMakeRect (0, 0, xw->width, xw->height);
@@ -454,9 +448,6 @@ - (BOOL)isFlipped { return YES; }
initWithFrame:rect
configuration:[[WKWebViewConfiguration alloc] init]
xwidget:xw];
- xw->xwWindow = [[XwWindow alloc]
- initWithFrame:rect];
- [xw->xwWindow addSubview:xw->xwWidget];
xw->xv = NULL; /* for 1 to 1 relationship of webkit2. */
unblock_input ();
}
@@ -481,22 +472,10 @@ - (BOOL)isFlipped { return YES; }
[((XwWebView *) xw->xwWidget).urlScriptBlocked release];
[xw->xwWidget removeFromSuperviewWithoutNeedingDisplay];
[xw->xwWidget release];
- [xw->xwWindow removeFromSuperviewWithoutNeedingDisplay];
- [xw->xwWindow release];
xw->xwWidget = nil;
}
}
-void
-nsxwidget_resize (struct xwidget *xw)
-{
- if (xw->xwWidget)
- {
- [xw->xwWindow setFrameSize:NSMakeSize(xw->width, xw->height)];
- [xw->xwWidget setFrameSize:NSMakeSize(xw->width, xw->height)];
- }
-}
-
Lisp_Object
nsxwidget_get_size (struct xwidget *xw)
{
@@ -504,12 +483,6 @@ - (BOOL)isFlipped { return YES; }
xw->xwWidget.frame.size.height);
}
-/* Xwidget view, macOS Cocoa part. */
-
-@implementation XvWindow : NSView
-- (BOOL)isFlipped { return YES; }
-@end
-
void
nsxwidget_init_view (struct xwidget_view *xv,
struct xwidget *xw,
@@ -526,16 +499,28 @@ - (BOOL)isFlipped { return YES; }
xv->clip_top = 0;
xv->clip_bottom = xw->height;
- xv->xvWindow = [[XvWindow alloc]
+ xv->xvWindow = [[NSView alloc]
initWithFrame:NSMakeRect (x, y, xw->width, xw->height)];
- xv->xvWindow.xw = xw;
- xv->xvWindow.xv = xv;
- xw->xv = xv; /* For 1 to 1 relationship of webkit2. */
- [xv->xvWindow addSubview:xw->xwWindow];
+ if (!xw->xv)
+ {
+ xw->xv = xv; /* For 1 to 1 relationship of webkit2. */
+ /* This seems like it should be retained, but tests show otherwise. */
+ xv->xvWidget = xw->xwWidget;
+ }
+ else
+ {
+ NSString *warningString = @"Cocoa Xwidgets do not support sharing widgets.";
+ NSTextField *warningLabel = [NSTextField labelWithString:warningString];
+ [warningLabel setFrameSize:NSMakeSize (xw->width, xw->height)];
+ warningLabel.drawsBackground = YES;
+ warningLabel.backgroundColor = [NSColor textBackgroundColor];
+ xv->xvWidget = warningLabel;
+ }
- xv->emacswindow = FRAME_NS_VIEW (s->f);
- [xv->emacswindow addSubview:xv->xvWindow];
+ [xv->xvWindow addSubview:xv->xvWidget];
+ xv->emacsFrame = FRAME_NS_VIEW (s->f);
+ [xv->emacsFrame addSubview:xv->xvWindow];
}
void
@@ -544,8 +529,8 @@ - (BOOL)isFlipped { return YES; }
if (!EQ (xv->model, Qnil))
{
struct xwidget *xw = XXWIDGET (xv->model);
- [xw->xwWindow removeFromSuperviewWithoutNeedingDisplay];
- xw->xv = NULL; /* Now model has no view. */
+ if (xw->xv == xv)
+ xw->xv = NULL; /* Now model has no view. */
}
[xv->xvWindow removeFromSuperviewWithoutNeedingDisplay];
[xv->xvWindow release];
@@ -555,21 +540,21 @@ - (BOOL)isFlipped { return YES; }
nsxwidget_show_view (struct xwidget_view *xv)
{
xv->hidden = NO;
- [xv->xvWindow setFrameOrigin:NSMakePoint(xv->x + xv->clip_left,
- xv->y + xv->clip_top)];
+ [xv->xvWindow setHidden:NO];
}
void
nsxwidget_hide_view (struct xwidget_view *xv)
{
xv->hidden = YES;
- [xv->xvWindow setFrameOrigin:NSMakePoint(10000, 10000)];
+ [xv->xvWindow setHidden:YES];
}
void
nsxwidget_resize_view (struct xwidget_view *xv, int width, int height)
{
- [xv->xvWindow setFrameSize:NSMakeSize(width, height)];
+ [xv->xvWindow setFrameSize:NSMakeSize (width, height)];
+ [xv->xvWidget setFrameSize:NSMakeSize (width, height)];
}
void
@@ -582,8 +567,7 @@ - (BOOL)isFlipped { return YES; }
void
nsxwidget_move_widget_in_view (struct xwidget_view *xv, int x, int y)
{
- struct xwidget *xww = xv->xvWindow.xw;
- [xww->xwWindow setFrameOrigin:NSMakePoint (x, y)];
+ [xv->xvWidget setFrameOrigin:NSMakePoint (x, y)];
}
void
diff --git a/src/xwidget.c b/src/xwidget.c
index 8bdfab02fd..013b773a90 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -2762,7 +2762,7 @@ xwidget_init_view (struct xwidget *xww,
xv->just_resized = false;
#elif defined NS_IMPL_COCOA
nsxwidget_init_view (xv, xww, s, x, y);
- nsxwidget_resize_view(xv, xww->width, xww->height);
+ nsxwidget_resize_view (xv, xww->width, xww->height);
#endif
return xv;
@@ -2794,7 +2794,7 @@ x_draw_xwidget_glyph_string (struct glyph_string *s)
xv->just_resized = false;
#elif defined NS_IMPL_COCOA
- if (!xv)
+ if (!xv || xww->xv != xv)
{
/* Enforce 1 to 1, model and view for macOS Cocoa webkit2. */
if (xww->xv)
@@ -2805,11 +2805,6 @@ x_draw_xwidget_glyph_string (struct glyph_string *s)
XSETXWIDGET_VIEW (xvl, xww->xv);
Fdelete_xwidget_view (xvl);
}
- else
- {
- message ("You can't share an xwidget (webkit2) among windows.");
- return;
- }
}
xv = xwidget_init_view (xww, s, x, y);
}
@@ -3243,8 +3238,6 @@ DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
gtk_widget_queue_allocate (GTK_WIDGET (xw->widget_osr));
}
-#elif defined NS_IMPL_COCOA
- nsxwidget_resize (xw);
#endif
unblock_input ();
diff --git a/src/xwidget.h b/src/xwidget.h
index 502beb6765..3943148e3f 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -81,16 +81,13 @@ #define XWIDGET_H_INCLUDED
guint hit_result;
#elif defined (NS_IMPL_COCOA)
# ifdef __OBJC__
- /* For offscreen widgets, unused if not osr. */
+ /* The primary widget. */
NSView *xwWidget;
- XwWindow *xwWindow;
- /* Used only for xwidget types (such as webkit2) enforcing 1 to 1
- relationship between model and view. */
+ /* The current view that displays the primary widget, if exists. */
struct xwidget_view *xv;
# else
void *xwWidget;
- void *xwWindow;
struct xwidget_view *xv;
# endif
#endif
@@ -136,11 +133,13 @@ #define XWIDGET_H_INCLUDED
int just_resized;
#elif defined (NS_IMPL_COCOA)
# ifdef __OBJC__
- XvWindow *xvWindow;
- NSView *emacswindow;
+ NSView *xvWindow;
+ NSView *xvWidget;
+ NSView *emacsFrame;
# else
void *xvWindow;
- void *emacswindow;
+ void *xvWidget;
+ void *emacsFrame;
# endif
#endif
--
2.37.0 (Apple Git-136)
next prev parent reply other threads:[~2022-10-04 2:03 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-03 9:32 bug#58271: 29.0.50; [PATCH] Handle sharing Cocoa xwidgets more gracefully 조성빈
2022-10-03 18:00 ` Lars Ingebrigtsen
2022-10-03 23:01 ` goranmoomin
2022-10-03 23:35 ` Lars Ingebrigtsen
2022-10-04 0:01 ` Sungbin Jo
2022-10-04 4:42 ` Stefan Kangas
2022-10-04 6:48 ` Eli Zaretskii
2022-10-04 7:25 ` Sungbin Jo
2022-11-11 13:27 ` Stefan Kangas
2022-10-04 0:34 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-10-04 2:03 ` Sungbin Jo [this message]
2022-11-25 1:26 ` Stefan Kangas
2022-11-25 2:55 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-11-26 5:31 ` 조성빈
2022-11-26 5:54 ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D75F5E89-B57D-4018-839A-222B6BDF000B@daum.net \
--to=goranmoomin@daum.net \
--cc=58271@debbugs.gnu.org \
--cc=larsi@gnus.org \
--cc=luangruo@yahoo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).