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.

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

Reorganize how window validity exceptions are checked and add ColorSlurp exception #1871

Closed
wants to merge 3 commits into from

Conversation

zacharee
Copy link
Contributor

This PR has 2 changes:

  • I noticed that checking for exceptions for when to show windows is getting a little complex and hard to read. This PR extracts checks into separate WindowCheck sub-classes and iterates over those instances, instead of creating and ORing a new function in for every exception. I also added a provision for excepting smaller-than-100x100 windows in the future.
  • ColorSlurp's picked-color dialog is meant to behave as a full window, but has an abnormal role. This adds an exception check to allow AltTab to show the ColorSlurp window.

Comment on lines +95 to +97
windowChecks.contains(where: { element in
element.check(opts)
}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have a similar short-circuiting effect compared to the old ORing method. If the checking order matters, it can be changed by rearranging the items in the windowChecks array.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you lost some of the nuances of the checks in the process of uniform-ing the checks. Apps like Books and Keynote are at a different level of the boolean chain than the apps later on. Same thing for Jetbrains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, you're right. I added in an isValidLevel() function to WindowCheck in the latest push.

Comment on lines +10 to +27
let windowChecks: [WindowCheck] = [
JetBrainsWindowCheck(),
IINAWindowCheck(),
KeyNoteWindowCheck(),
OpenBoardWindowCheck(),
AdobeAuditionWindowCheck(),
BooksWindowCheck(),
WorldOfWarcraftWindowCheck(),
BattleNetBootstrapperWindowCheck(),
DrBetotteWindowCheck(),
DVDFabWindowCheck(),
SanGuoShaAirWDWindowCheck(),
SteamWindowCheck(),
FirefoxFullscreenVideoWindowCheck(),
VLCFullscreenVideoWindowCheck(),
AndroidEmulatorWindowCheck(),
ColorSlurpWindowCheck()
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too familiar with Swift so there may be a better way collect all the subclasses.

Comment on lines +56 to +58
func isValidSize(_ size: CGSize?) -> Bool {
return size != nil && size!.width > AXUIElement.minWindowSize && size!.height > AXUIElement.minWindowSize
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be useful for #1466

@zacharee
Copy link
Contributor Author

I also have the Ventura workaround commit here, but it should be possible to cherry-pick the other two commits.

@zacharee zacharee marked this pull request as ready for review August 14, 2022 22:55
@zacharee zacharee force-pushed the reorg-and-colorslurp-fix branch from 873537b to d5c7542 Compare August 14, 2022 23:18
@lwouis
Copy link
Owner

lwouis commented Aug 14, 2022

@zacharee thank you for sharing this work.

I would like to see what you see in this refactor, but I'm afraid I don't. The business logic is moved from simple functions into something more complex involving a class hierarchy with subclass overriding methods, instantiating classes, etc.

I think if you find the current AXUIElement.swift#isActualWindow method noisy, you could create this WindowCheck file you created here, and simply move all the various functions in there to contain the "zoo". I'm not bothered by the methods and the current volume of lines in AXUIElement to be honest, but I would understand someone wanting to cut isActualWindow out of this file. However, I don't see the value in moving from straight-forward functions that are unique and customized to each business rule, into some generic system using a class hierarchy. I find for instance that each overriden method having the whole "opts" object passed reduces clarity, versus the current implem where each business function gets passed only what it needs to discriminate the window.

@zacharee
Copy link
Contributor Author

@zacharee thank you for sharing this work.

I would like to see what you see in this refactor, but I'm afraid I don't. The business logic is moved from simple functions into something more complex involving a class hierarchy with subclass overriding methods, instantiating classes, etc.

I think if you find the current AXUIElement.swift#isActualWindow method noisy, you could create this WindowCheck file you created here, and simply move all the various functions in there to contain the "zoo". I'm not bothered by the methods and the current volume of lines in AXUIElement to be honest, but I would understand someone wanting to cut isActualWindow out of this file. However, I don't see the value in moving from straight-forward functions that are unique and customized to each business rule, into some generic system using a class hierarchy. I find for instance that each overriden method having the whole "opts" object passed reduces clarity, versus the current implem where each business function gets passed only what it needs to discriminate the window.

The issue I was having with the original setup was parsing all of the nested groups of disjunctions and conjunctions and where a new exception should be added (for example, the JetBrains check was inverted vs. the rest).

There is more code, but imo now it's clearer how and when each step is checked, and adding a new exception just requires a new class and instantiating that class in the list, without needing to step through the parentheses to make sure it's placed correctly.

My inspiration was the Strategy pattern, which does have the drawback of needing more generic arguments, but the advantage of encapsulating similar functions into a predictable structure, i.e., prioritizing knowing where and when each one is used vs. knowing what each one does specifically.

Definitely up for more discussion on this.

@lwouis lwouis force-pushed the master branch 2 times, most recently from 796a3c3 to a67d123 Compare June 23, 2023 21:31
@lwouis
Copy link
Owner

lwouis commented Jun 23, 2023

Closing this PR as the ColorSlurp got merged from the other PR 👍

@lwouis lwouis closed this Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants