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

Significant delay when loading hs.window.filter #3712

Open
avegetablechicken opened this issue Nov 9, 2024 · 14 comments
Open

Significant delay when loading hs.window.filter #3712

avegetablechicken opened this issue Nov 9, 2024 · 14 comments

Comments

@avegetablechicken
Copy link

I experienced significant delay when loading hammerspoon scripts. The delay happens when hammerspoon loads hs.window.filter module for the first time. The delay lasts for almost the same time as hs.window._timed_allWindows hangs. Considering that process "com.apple.WebKit.WebContent" accounts for major part of delay when executing hs.window._timed_allWindows, I add a line of code in function "startAppWatcher" in file "window_filter.lua". It seems that the unexpected delay disappears.

local function startAppWatcher(app,appname,retry,nologging,force)
  if not app or not appname then log.e('called startAppWatcher with no app') return end
  if apps[appname] then return not nologging and log.df('app %s already registered',appname) end
 + if app:bundleID() == "com.apple.WebKit.WebContent" then return end
  if app:kind()<0 or not windowfilter.isGuiApp(appname) then log.df('app %s has no GUI',appname) return end
  if not fnutils.contains(axuielement.applicationElement(app):attributeNames() or {}, "AXFocusedWindow") then
      log.df('app %s has no AXFocusedWindow element',appname)
      return
  end
  retry=(retry or 0)+1

  if app:focusedWindow() or force then
    pendingApps[appname]=nil --done
    local watcher = app:newWatcher(appWindowEvent,appname)
    watcher:start({uiwatcher.windowCreated,uiwatcher.focusedWindowChanged})
    App.new(app,appname,watcher)
  else
    -- apps that start with an open window will often fail to be detected by the watcher if we
    -- start it too early, so we try `app:focusedWindow()` MAX_RETRIES times before giving up
    pendingApps[appname] = timer.doAfter(retry*RETRY_DELAY,function()
        startAppWatcher(app,appname,retry,nologging, retry > MAX_RETRIES)
    end)
  end
end

I don't know whether this is a bug, but it does occur to both of my MBP. Have anyone else come into it?

@franzbu
Copy link

franzbu commented Nov 11, 2024

Thanks for that; it speeds up starting EnhancedSpaces (https://github.com/franzbu/EnhancedSpaces.spoon) and I haven't encountered any side effects.

@FischLu
Copy link

FischLu commented Dec 28, 2024

I can confirm I have the same issue and the above mentioned method works

@dmgerman
Copy link

Yes, it feels like it reduces the loading time significantly.

@dmgerman
Copy link

dmgerman commented Mar 7, 2025

I did a bit of code reading. This issue with window filter is that it keeps track of the name of applications that are "problematic". Kind == 0 but will have never create windows.

The problem with the code is that it uses the app name instead of the bundleID. The issue appears to be that instances of com.apple.WebKit.WebContent operate under many different names. In practice, what needs to be blacklisted is the bundle ID, not the name. Essentially, as long as window.filter tracks the app name, this is a game of whack-a-mole.

To help maintain this list, the original author wrote: hs.window.filter._showCandidates()

you can run this function to see which app names are creating problems.

I think we need to add the ability to blacklist bundleIDs. This will need an equivalent to windowfilter.isGuiApp(appname) that uses bundleID instead of appname.

@dmgerman
Copy link

I worked on this today and I have a solution and some comments.

  1. I have implemented a way to blacklist bundleIDs in addition to applications. that will severely restrict the modifications needed to be done to the list of app names and will allow skipping all "com.apple.WebKit.WebContent" bundled applications

  2. The issue remains if there is an application that has two or more instances running under the same bundleID. This is a known problem with window.filter and there is already a patch for it. I want to test that patch with my additions and see if nothing is broken before I submit it.

In the meantime: if you have two instances of the same app, expect delays in the reload; if you use webkit apps, add the if condition in the issue description.

@avegetablechicken
Copy link
Author

I worked on this today and I have a solution and some comments.

  1. I have implemented a way to blacklist bundleIDs in addition to applications. that will severely restrict the modifications needed to be done to the list of app names and will allow skipping all "com.apple.WebKit.WebContent" bundled applications
  2. The issue remains if there is an application that has two or more instances running under the same bundleID. This is a known problem with window.filter and there is already a patch for it. I want to test that patch with my additions and see if nothing is broken before I submit it.

In the meantime: if you have two instances of the same app, expect delays in the reload; if you use webkit apps, add the if condition in the issue description.

It seems that hammerspoon's window filter module calls startAppFilter for all running applications. You have to manually blacklist "com.apple.WebKit.WebContent".

@dmgerman
Copy link

Here is the code I am working on:

https://github.com/dmgerman/hammerspoon/tree/window-filter

Please try it, and if it works, i'll submit the PR.

@avegetablechicken
Copy link
Author

Here is the code I am working on:

https://github.com/dmgerman/hammerspoon/tree/window-filter

Please try it, and if it works, i'll submit the PR.

Works perfectly

@avegetablechicken
Copy link
Author

@dmgerman It seems that your code has a bug. You should replace win.bundleID with win.app.bundleID at line 296.
BTW, if you add filter=self.filters[appname] or self.filters[bundleID] to line 314, then setAppFilter supports bundle identifiers as input as well. I eagerly want this feature.

@dmgerman
Copy link

@dmgerman It seems that your code has a bug. You should replace win.bundleID with win.app.bundleID at line 296. BTW, if you add filter=self.filters[appname] or self.filters[bundleID] to line 314, then setAppFilter supports bundle identifiers as input as well. I eagerly want this feature.

Thanks. I'll fix that.

Regarding the setAppFilter: I don't like functions that ambiguously do two different things. So I prefer to keep one for app Name and one for app BundleID.

Is there any advantage aside convinience

@avegetablechicken
Copy link
Author

avegetablechicken commented Mar 17, 2025

@dmgerman It seems that your code has a bug. You should replace win.bundleID with win.app.bundleID at line 296. BTW, if you add filter=self.filters[appname] or self.filters[bundleID] to line 314, then setAppFilter supports bundle identifiers as input as well. I eagerly want this feature.

Thanks. I'll fix that.

Regarding the setAppFilter: I don't like functions that ambiguously do two different things. So I prefer to keep one for app Name and one for app BundleID.

Is there any advantage aside convinience

Different applications may share the same names, but almost never share the same bundle identifiers. Providing two APIs is better, but I think the important thing is that you provide the one for bundle identifiers.

@dmgerman
Copy link

i have pushed the fix

Thanks. I'll fix that.
Regarding the setAppFilter: I don't like functions that ambiguously do two different things. So I prefer to keep one for app Name and one for app BundleID.
Is there any advantage aside convinience

Different applications may share the same names, but almost never share the same bundle identifiers. Providing two APIs is better, but I think the important thing is that you provide the one for bundle identifiers.

I was looking at the code and refactoring it to have bundleID and app separately seems like a pain that might create unexpected bugs.

What do you think of the following: if the application name has at least one period: then consider it a bundle id, otherwise it is an appname. We'll just make sure that any time a filter is created, we do this test.

@avegetablechicken
Copy link
Author

avegetablechicken commented Mar 18, 2025

@dmgerman
Some application has a period in its name. There is a not-so-elegant workaround: calling hs.application.pathForBundleID to test if it is a bundle identifier.

@dmgerman
Copy link

I spent a bit more time on this today.

I think I am going to drop the idea of keeping bundleIDs and application name separated.

Trying to keep different logics is making the code more complicated than it is already.

I also wrote a function to make sure that pids that no longer exist are released, so that should hopefully take care of the memory leaks.

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

No branches or pull requests

4 participants