Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

devdraw: Handle windowDidResize to support non-live window resize. #212

Closed
wants to merge 1 commit into from
Closed

devdraw: Handle windowDidResize to support non-live window resize. #212

wants to merge 1 commit into from

Conversation

pocket7878
Copy link
Contributor

@pocket7878 pocket7878 commented Nov 17, 2018 •
edited
Loading

Some window management tools (e.g Moom which I'm using on my mac) using Accessibility API to support resizing window from keyboard.
And current Metal cocoa screen (Implemented in #194) only redraw after live resize event to improve performance.
But window resizing through accessibility API does not sent live-resize event.
So I'm add a handler for windowDidResize event, and check inLiveResize flag to skip inefficient redraw.

mattwidmann
mattwidmann reviewed Nov 17, 2018
View reviewed changes
src/cmd/devdraw/cocoa-screen-metal.m Outdated
- (void)windowDidResize:(NSNotification *)notification
{
if([self inLiveResize]) {
LOG(@"Skip resizeimg in live resize");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this useful? Seems like it would overwhelm the log whenever a user-driven resize is active.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwidmann Thank you for your review! I'll remove this debug Log.

src/cmd/devdraw/cocoa-screen-metal.m Outdated
@@ -197,6 +197,7 @@ + (void)makewin:(NSValue *)v
[win setContentView:myContent];
[myContent setWantsLayer:YES];
[myContent setLayerContentsRedrawPolicy:NSViewLayerContentsRedrawOnSetNeedsDisplay];
[[NSNotificationCenter defaultCenter] addObserver:myContent selector:@selector(windowDidResize:) name:NSWindowDidResizeNotification object:nil];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this pass win as the object? I wonder why myContent would work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to register this event for the view. Just using the NSApplicationDelegate method should be enough.

Copy link
Contributor Author

@pocket7878 pocket7878 Nov 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review!
@jxy If I use NSWindowDelegate method on AppDelgate,
How can I check inLiveResize flag of DevDrawView?
I'll check inLiveReisize through myContent value thanks!

src/cmd/devdraw/cocoa-screen-metal.m Outdated
@@ -373,6 +374,12 @@ - (BOOL)applicationShouldTerminateAfterLastWindowClosed:(NSApplication *)theAppl
return YES;
}

- (void)windowWillClose:(NSNotification *)notification
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need a notification registered for it, too? Or is this enabled by default when asking for another notification on the window?

@jxy
Copy link
Contributor

jxy commented Nov 18, 2018 •
edited
Loading

This may overlap with other resizing event. Perhaps we need to remove other events that calls resizeimg?

@pocket7878
Copy link
Contributor Author

pocket7878 commented Nov 18, 2018 •
edited
Loading

@jxy Thank you for your review
I've added some LOG line to other events viewDidEndLiveResize, viewDidChangeBackingProperties.

When I resize by mouse, resizeimg runs only once from viewDidEndLiveResize, cause I checked inLiveResize flag check in inside windowDidResize callback.
When I resize form keyboard resizeimg runs only once from windowDidResize, cause that event is not live resize event.
So I think there might be no overlap.

jxy
jxy reviewed Nov 20, 2018
View reviewed changes
src/cmd/devdraw/cocoa-screen-metal.m Outdated
@@ -373,6 +373,13 @@ - (BOOL)applicationShouldTerminateAfterLastWindowClosed:(NSApplication *)theAppl
return YES;
}

- (void)windowDidResize:(NSNotification *)notification
{
if(![myContent inLiveResize]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that we probably need to check if img is nil before calling resizeimg. It may get resized before attachscreen.

@jxy
Copy link
Contributor

jxy commented Nov 20, 2018

I think setContentSize would trigger windowDidResize. Please verify. Probably we can just remove the resizeimg call after the call to setContentSize in function resizewindow.

@jxy
Copy link
Contributor

jxy commented Nov 20, 2018

It's probably better to squash your commits.

@jxy jxy mentioned this pull request Dec 17, 2018
Closed
@pocket7878
Copy link
Contributor Author

@jxy Thank you for your review! I just update codes & squashed

@mkmik
Copy link

mkmik commented Jun 7, 2019

Compiling this on macos 10.14.5 I get:

9c -DOSX_VERSION=101405 -fobjc-arc -o cocoa-screen-metal-objc.o cocoa-screen-metal.m
cocoa-screen-metal.m:393:9: error: use of undeclared identifier 'windowDidBecomeKey'
cocoa-screen-metal.m:1274:2: error: expected '}'
cocoa-screen-metal.m:388:1: note: to match this '{'
cocoa-screen-metal.m:1274:2: error: missing '@end'
cocoa-screen-metal.m:139:1: note: implementation started here
mk: 9c -DOSX_VERSION=101405 -fobjc-arc ...  : exit status=exit(1)

@jxy
Copy link
Contributor

jxy commented Jun 7, 2019

Yes. There is an extra { after the if statement.

@pocket7878
Copy link
Contributor Author

@mkmik Thank you for your reporting! Fixed :)

rsc pushed a commit that referenced this pull request Jun 11, 2019
@pocket7878 @rsc
devdraw: handle windowDidResize on macOS (#212)
d4e16c8 
This supports non-live window resize.
@rsc
Copy link
Contributor

rsc commented Jun 11, 2019

Committed, thanks.

@rsc rsc closed this Jun 11, 2019
@pocket7878 pocket7878 deleted the feature/redraw-on-window-did-resize branch June 12, 2019 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattwidmann mattwidmann mattwidmann left review comments

@jxy jxy jxy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pocket7878 @jxy @mkmik @rsc @mattwidmann