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

Persist window state to directory #761

Closed
wants to merge 0 commits into from
Closed

Conversation

ayroblu
Copy link
Contributor

@ayroblu ayroblu commented Jan 3, 2021

In a first attempt to tackle #371, I thought that writing to the filesystem would be useful.

Menu layout:
image

(I'm using a ram disk as volatile fyi)

I then hooked this up to alfred and it worked as expected!:

image

TODO:

  • Allow selecting any directory rather than using a hardcoded Documents/alt-tab-windows.json, probably with button + NSPathControl
  • Move this persist to another preference tab, maybe Advanced?
  • Fix layout with NSPathControl shrinking when longer path selected as well as handling long paths
  • Cocopods update + workspace version updates shouldn't be in this PR
  • Define what actually should be exported rather than just exporting the random fields I wanted (spaceIndex looks wrong, and maybe others)
  • Hook on to all the correct events, I only put it on one update statement, perhaps it's reasonable to put it more / different places
  • Benchmarks for the performance degradation
  • Tests?
  • Use async
  • Write better swift (no experience here, hope it's okay?)

@ayroblu ayroblu marked this pull request as draft January 3, 2021 20:33
@lwouis
Copy link
Owner

lwouis commented Jan 4, 2021

Hey thanks for sharing this first draft! It's nice to see you got useful results so fast!

Regarding the preferences, I'm a bit worried as I've learned on this project how they can grow out of control. I'm thinking that an opt-in checkbox is necessary because of the potential perf impact, but also the certain "intrusiveness" (as in, the user may expect the app to not write anything on disk, given its functionality).

Regarding your todo list:

Benchmarks for the performance degradation
Write better swift (async? etc)

I think that you should use async IO to write to disk. Then I expect the performance impact should be almost null.

To benchmark it, you could use Instruments and do a before/after the change. That's how I get a general performance understanding of changes that I suspect would impact performance.

Tests?

Because this app is 100% side-effects from OS interactions, I gave up on automated testing from the get go. I kind of regret it sometimes, as probably some unit tests here and there would have helped, but at the same time even in retrospect I feel I made the right call back then. So there are no automated tests today; it's all manual QA. I have a section if you're curious to see the kind of fun macOS had in store when I built this app.

Define what actually should be exported rather than just exporting the random fields I wanted (spaceIndex looks wrong, and maybe others)

You may want to discuss this with @v-braun and see what kind of usage he would have of it, to decide what to expose. A good start would be to expose the list of windows observed, with all their attributes. Other useful stuff may include the info on Spaces, Screens, running Applications.

By the way, I realized that your approach of exporting the state of the app as JSON is useful to get info on the state, but will not allow the user to interact with the windows or other elements. For the window for instance, even if you exported their ID, and passed it to some AppleScript, it couldn't work in all cases. I'm using private APIs and private SPIs to focus windows, as there is no public API to do it correctly for some edge-cases (like windows on other Spaces, or minimized in the Dock on other Spaces, I forgot)

Hook on to all the correct events, I only put it on one update statement, perhaps it's reasonable to put it more / different places

If you serialization code is fast enough, you probably want to rewrite the whole file on any change in the state, instead of updating specific lines by doing delta changes. You may put the update code after this switch. If you're exposing Applications state, you may want to add the update code here as well.

Cocopods update + workspace version updates shouldn't be in this PR

Yes, please! These updates are really tricky as they usually break CI, thus involve extra work/care to fix it. I have setup a full CI > CD pipeline that releases apps to the public, so breaking it is a critical thing. Unless an update is trully useful/necessary, I would avoid updating cocoapods 😅

@ayroblu
Copy link
Contributor Author

ayroblu commented Jan 4, 2021

Preferences: This is what I have right now, I thought I need the button, but actually I might not for the path control + I need to make it show the icons, it's a bit bland right now. I think it also fails silently on permissions issues right now to I need to put that in somewhere too:

image

This worked well enough for me with osascript for alfred (including changing to windows in different spaces) (yes ALOT of stackoverflow there). I never minimize / hide so I haven't tested those specifically

on alfred_script(q)

set argv to extract_argv(q, "|||||")

set proc to item 1 of argv
set windowId to item 2 of argv as integer
set windowName to item 3 of argv

tell application "System Events" to tell process proc
    set frontmost to true
    windows where title contains windowName
    if result is not {} then perform action "AXRaise" of item 1 of result
	click (first menu item whose name contains windowName) of menu "Window" of menu bar 1
end tell

end alfred_script

on extract_argv(source_string, new_delimiter)
	set backup to AppleScript's text item delimiters
	set AppleScript's text item delimiters to new_delimiter
	set argv to every text item of source_string
	set AppleScript's text item delimiters to backup
	return argv
end extract_argv

Regarding cocoapods, I definitely had to upgrade my targeting atleast, I might not have needed to do the pod install, so I can try revert that, but xcode won't compile for me without some config change. I'm using Big Sur, and I installed XCode yesterday (it took like 2 hours 😭️) @lwouis

@ayroblu
Copy link
Contributor Author

ayroblu commented Jan 4, 2021

Oh looks like I didn't need to pod install, sorry I must've been mistaken!

Not a blocking error:
image

@Stvad
Copy link

Stvad commented Mar 28, 2021

@ayroblu I see you've discarded this approach in favor of #768. I'm not quite sure what's the state of that one, but I given that you got to a working Alfred WF here I was wondering if you had this somewhere on the branch that I can build for myself? 🙂

@ayroblu
Copy link
Contributor Author

ayroblu commented Mar 28, 2021

Any particular reason you don't want to use #768 with a HTTP? Otherwise I can see if I can revert the changes on the reflog for this PR

@Stvad
Copy link

Stvad commented Mar 28, 2021

Mainly laziness 😅. You have an Alfred WF for this one & all 😛. Do you have a WF that works with the other PR? 🙂
Also, I think it can be nice to have code of this preserved for posterity 🙂

(also got scared by other pr's size initially, but it seems most of the code is copying source of a library? btw I'm Swift ignorant, so I'm curious on why you'd want to do that vs dependency?).

@ayroblu
Copy link
Contributor Author

ayroblu commented Mar 29, 2021

@Stvad I made #886 (sorry couldn't do it to this pr) for posterity. I've only made my workflow work for the HTTP one now so see that PR for details.

Dependencies are checked into git, otherwise they work in a similar way to other languages. I'm not sure if it's strictly necessary, but this is called "vendoring" and it's pretty common especially for lower level languages. Obviously most useful if you want to make changes, whether for performance or compatibility reasons, or for more of a "it just works" when sharing a repo

@Stvad
Copy link

Stvad commented Mar 30, 2021

@ayroblu thanks a lot!

@lwouis
Copy link
Owner

lwouis commented Apr 8, 2021

@ayroblu

This worked well enough for me with osascript for alfred (including changing to windows in different spaces) (yes ALOT of stackoverflow there). I never minimize / hide so I haven't tested those specifically

I'm pretty sure your osacript will not work in every case, unfortunately. AltTab uses private API and private SPI to enable focusing every window in every situation. Maybe good enough for your particular usage though, which is already great!

@ayroblu
Copy link
Contributor Author

ayroblu commented Apr 8, 2021

Yes! I think with the http server implementation in #768 it was so easy to hook in to the APIs that I just did that instead 😅
You can see it here
Then it's just an http call away!

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.

3 participants