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

Ticket 266 #268

Merged
merged 5 commits into from
Jan 24, 2021
Merged

Ticket 266 #268

merged 5 commits into from
Jan 24, 2021

Conversation

cfraizer
Copy link
Contributor

  • Updated related documentations

  • Added the change to the Changelog

  • I wasn't sure how you wanted to document this, so I didn't.

  • The only significant change is in PHApp's focus method.

Of course, I'd be happy to make any changes you suggest.

@cfraizer
Temporary work-around for broken activateWithOptions.
ce1ac80
@cfraizer
For MacOS versions >= 11.0.0, call SetFrontProcessWithOptions.
f3fe504 
Apple broke NSRunningApplication's activateWithOptions method (so it
wrongly always brings all windows to the front).  This work-around
calls the older (deprecated) SetFrontProcessWithOptions to restore
the desired effect.

Once Apple fixes their bug, this code can be removed.
kasper
kasper reviewed Dec 30, 2020
View reviewed changes
Copy link
Owner

@kasper kasper left a comment

Choose a reason for hiding this comment

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

Nice, thank you! Added some things to address, could you also add the change to the CHANGELOG?

Phoenix.xcodeproj/project.pbxproj Outdated
@@ -748,7 +748,7 @@
COMBINE_HIDPI_IMAGES = YES;
CURRENT_PROJECT_VERSION = 92;
ENABLE_HARDENED_RUNTIME = YES;
GCC_TREAT_WARNINGS_AS_ERRORS = YES;
Copy link
Owner

Choose a reason for hiding this comment

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

Revert changes to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phoenix/Phoenix/PHApp.m:179:26: error: 'GetProcessForPID' is deprecated: first deprecated in macOS 10.9
      [-Werror,-Wdeprecated-declarations]
        OSStatus error = GetProcessForPID(self.app.processIdentifier, &process);

phoenix/Phoenix/PHApp.m:184:16: error: 'SetFrontProcessWithOptions' is deprecated: first deprecated in macOS 10.9
      [-Werror,-Wdeprecated-declarations]
        return SetFrontProcessWithOptions(&process, kSetFrontProcessFrontWindowOnly);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nvm. I used #pragma to suppress just those two warnings.

Phoenix/PHApp.m Outdated
@@ -166,7 +167,27 @@ - (BOOL) activate {

- (BOOL) focus {

return [self.app activateWithOptions:NSApplicationActivateIgnoringOtherApps];
if (![NSProcessInfo isOperatingSystemAtLeastBigSur]) {
Copy link
Owner

Choose a reason for hiding this comment

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

I would reverse this. So:

  1. check if version is Big Sur, call SetFrontProcessWithOptions
  2. do an early return
  3. always default to activateWithOptions: without the else as the last thing of this method

Phoenix/PHApp.m Outdated

ProcessSerialNumber process;
OSStatus error = GetProcessForPID(self.app.processIdentifier, &process);
if (error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Use noErr constant for the comparison, see

phoenix/Phoenix/PHOpenAtLoginHelper.m

Line 84 in 53c45c5

if (error != noErr) {
.

Phoenix/PHApp.m Outdated
if (!self.app || self.app.processIdentifier == -1) {
return false;
}

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove redundant newlines.

Phoenix/PHApp.m Outdated
}

error = SetFrontProcessWithOptions(&process, kSetFrontProcessFrontWindowOnly);
return (error == 0) ? true : false;
Copy link
Owner

Choose a reason for hiding this comment

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

You can just return the result of the conditional (i.e. no need for the explicit ternary).

Phoenix/PHApp.m Outdated
return [self.app activateWithOptions:NSApplicationActivateIgnoringOtherApps];
} else {

// Source https://stackoverflow.com/a/65464683/525411
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Source https://stackoverflow.com/a/65464683/525411
// FIX: Workaround for the buggy focus behaviour in Big Sur, see issue #266. For reference see https://stackoverflow.com/a/65464683/525411.

@cfraizer
Copy link
Contributor Author

I incorporated your changes, but I made one additional change.

I found that some windows (those from Emacs.app) would fail in PHAXUIElement's processIdentifier method.
So I changed line 174 in PHApp.m to call [self.app processIdentifier].

(I'm a total Mac programming n00b, so I apologize if I've made this more difficult than it should be! I really appreciate all your help with this.)

@cfraizer
Copy link
Contributor Author

Hmn. Something is still wrong. I don't really understand it.

Now PHWindow's focused method is broken. That is, Window.focused() returns null.
My guess is that changing the focus through SetFrontProcess doesn't do the right thing that PHWindow's focused method looks at.

@kasper
Copy link
Owner

kasper commented Dec 31, 2020

@cfraizer Hmm, could you also verify that it triggers the relevant events for focusing?

@cfraizer
Copy link
Contributor Author

I'm not sure what you mean. (Sorry that I'm a bozo!) It certainly moves the focus—that is, it raises the correct window and that window has keyboard input focus, but after moving it, then Window.focused() returns null.

@cfraizer
Copy link
Contributor Author

Ah, never mind on that other problem. I was returning the wrong value from PHApp's focus method and that uncovered a weird (ancient) bug in my own JavaScript that caused it to try to call the JS Window.focus() method on a null object.

I've tested as many variations I can think of and the problem no longer occurs.

kasper
kasper approved these changes Jan 24, 2021
View reviewed changes
Copy link
Owner

@kasper kasper left a comment

Choose a reason for hiding this comment

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

Seems reasonable, thanks @cfraizer! I’ll test around before releasing a new version.

@kasper kasper merged commit 2f08889 into kasper:master Jan 24, 2021
kasper added a commit that referenced this pull request Jan 24, 2021
@kasper
Cleanup for #268
dca384b
@kasper kasper added this to the 2.6.6 milestone Jan 24, 2021
@cfraizer cfraizer deleted the ticket-266 branch January 26, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasper kasper kasper approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@cfraizer @kasper