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

feat: show preview for minimize/hidden windows #214

Closed
wants to merge 9 commits into from
Closed

feat: show preview for minimize/hidden windows #214

wants to merge 9 commits into from

Conversation

ShlomoCode
Copy link
Contributor

@ShlomoCode ShlomoCode commented Jul 18, 2024 •
edited
Loading

TODO:

  • Don't show tabs as separate windows
  • Make sure the window swicher is working properly

@ShlomoCode
Copy link
Contributor Author

Video: https://share.cleanshot.com/jqM6wlW3Tw0gxRD2VZkJ

@ShlomoCode ShlomoCode changed the title WIP: show preview for minimize/hidden windows feat: show preview for minimize/hidden windows Jul 18, 2024
ejbills
ejbills reviewed Jul 18, 2024
View reviewed changes
DockDoor/Utilities/KeybindHelper.swift Outdated
}
let apps = NSWorkspace.shared.runningApplications
let windows = apps.flatMap { app in
return WindowUtil.getRunningAppWindows(app: app)
Copy link
Owner

@ejbills ejbills Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ensure you use [weak self] in the closure like the original code (when referring to WindowUtil), otherwise there may be memory retention issues. The app previously had a memory leak/abandoned memory problem, which was resolved by using weak self references. The new code removes this safety measure, which could reintroduce memory management issues.

If you do decide to keep this change, I would just be sure to run lengthy tests to ensure that memory usage doesn't grow continuously as the app keeps running and being used.

@ShlomoCode
Copy link
Contributor Author

Yes, there is still a lot of work and cleaning to do here. I want to largely mimic the alt-tab architecture. it's WIP

@hasansultan92
Copy link
Contributor

Nice work @ShlomoCode!

@ShlomoCode ShlomoCode force-pushed the main branch 6 times, most recently from 86b3ea3 to 23f3bd9 Compare July 19, 2024 03:26
@ShlomoCode
Copy link
Contributor Author

@ejbills What do you think so far?

@ejbills
Copy link
Owner

ejbills commented Jul 19, 2024 •
edited
Loading

@ejbills What do you think so far?

I still want to stress the importance of ensuring the use of [weak self] when referring to WindowUtil (or other singletons).

It seems I am having trouble with clicking the window preview. The hover container window hides, but the window selected is not being brought to front for some reason.

Also, the window placement when the dock is set to the left seems to be placing the window in the incorrect place. I am regularly seeing some odd placement of the hover window, where it snaps to the bottom of the dock (it usually self corrects the placement, but sometimes it gets stuck at the bottom of the dock when I'm hovering on an icon on the top. (Though, I am unsure if this is an issue I was talking about with @chrisharper22 when he changed how the window placement works - input? Though, this doesn't seem to be present on the current main branch.)
https://streamable.com/ext69m

I experienced one crash and some warnings when running this version about SharedPreviewWindowCoordinator being referred to on non-main threads. NSWindow should only be referred to on the main thread, so just make sure that is covered.

In regards to the overall changed structure of the project and things being shuffled around, I think it looks great. I was able to jump right in and intuitively understand the new flow of the project. So, I think the refactoring is perfect so far! Short of the few bugs, but nothing major.

@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Jul 19, 2024 •
edited
Loading

Also, the window placement when the dock is set to the left seems to be placing the window in the incorrect place. I am regularly seeing some odd placement of the hover window, where it snaps to the bottom of the dock (it usually self corrects the placement, but sometimes it gets stuck at the bottom of the dock when I'm hovering on an icon on the top. (Though, I am unsure if this is an issue I was talking about with @chrisharper22 when he changed how the window placement works - input? Though, this doesn't seem to be present on the current main branch.)

I can't reproduce it... https://share.cleanshot.com/XYpcLXQBKFsPcB3T0Tl3

@ShlomoCode ShlomoCode force-pushed the main branch 2 times, most recently from c5a481b to 04073a7 Compare July 27, 2024 19:16
@ShlomoCode
Copy link
Contributor Author

@ejbills I think I fixed the following issues:

  • The week self-reference
  • The call to NSWindow from a non-main thread
  • The difficulties focusing on the window from the preview

Please let me know if there are any other concerns.

@ShlomoCode ShlomoCode marked this pull request as ready for review July 27, 2024 19:29
@ShlomoCode ShlomoCode marked this pull request as draft July 27, 2024 19:29
@ShlomoCode
Copy link
Contributor Author

Marked as a draft because of the 2 TODOs listed in the PR description

@ShlomoCode ShlomoCode force-pushed the main branch 2 times, most recently from 24f114c to 7c21c60 Compare July 28, 2024 22:20
@ejbills
Copy link
Owner

ejbills commented Jul 28, 2024

Marked as a draft because of the 2 TODOs listed in the PR description

See comment on this PR: #234 (comment)

@ShlomoCode
Copy link
Contributor Author

This PR now includes much more than hidden windows, please see the updated PR description

@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Jul 29, 2024 •
edited
Loading

Yes of course
You can try this PR yourself, it's fully functional but still needs some polishing (for example: back from the פreview to the dock icon, the preview should not close)

@hasansultan92
Copy link
Contributor

This PR was based off v1.1.4 or v1.1.2?

@ShlomoCode
Copy link
Contributor Author

v1.1.4 I think

@ShlomoCode ShlomoCode mentioned this pull request Jul 29, 2024
Merged
@ejbills ejbills mentioned this pull request Aug 3, 2024
Closed
@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Aug 5, 2024 •
edited
Loading

@ejbills @hasansultan92 Is there a big change coming? I want to start redoing the changes here as smaller PRs (I'll start with the performance thing).

@hasansultan92
Copy link
Contributor

@ShlomoCode no big changes are incoming. Also, we sorted out the performance issue on our end. It was due to the image resolution of the window preview being held in memory. I believe @ejbills pushed out a slider in the settings that controls the image preview resolution before the PR was merged into main. Users who want large resolutions of the window preview take the performance hit vs those who dont want the large resolutions dont take the performance hit.

@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Aug 5, 2024 via email •
edited
Loading

@ShlomoCode
Copy link
Contributor Author

@hasansultan92 See #190

@hasansultan92
Copy link
Contributor

@ShlomoCode apologies, I did not realize we were consuming so much CPU in idle state. I have been paying attention to the RAM since most of the work I did in the recent enhancements and features was more related to the logic and less to the preview display and cursor.

I like the approach as mentioned in the issue referenced. Is there a POC we can check out, the link there does not work anymore?

@ShlomoCode
Copy link
Contributor Author

@hasansultan92 It is included in this PR, you can just pull https://github.com/ShlomoCode/DockDoor/tree/main

@hasansultan92
Copy link
Contributor

Nice, just checked it out. I like the idle usage is down to 0%, beautiful. Though take a look at the screenshot from the logs, seems like this PR is not able to recognize Microsoft Edge windows. I havent dived into the code but I also did notice it was throwing an error along the lines of "some error in fetchWindowInfo" upon attempting to enable the window switcher.

image

@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Aug 5, 2024 •
edited
Loading

@hasansultan92 The Failed to get app URL or convert NSURL to URL error is normal, it happens when you hover over the trash icon that doesn't have a 'URL'

@hasansultan92
Copy link
Contributor

ok but Edge had no previews despite the window being open

@ShlomoCode
Copy link
Contributor Author

ShlomoCode commented Aug 5, 2024 •
edited
Loading

@hasansultan92 I installed the Edge browser (from https://www.microsoft.com/en-us/edge), it seems to work flawlessly 🤔
Video: https://share.cleanshot.com/0xNWFJ8h8nzlZJSTfgtX

@hasansultan92
Copy link
Contributor

Hmm not sure what's happening on my end. Regardless, I do like the 0% idle and seems to be working fine otherwise.

@ShlomoCode
Copy link
Contributor Author

@hasansultan92 Even when hovering on the dock the CPU usage is lower than before.
One thing that still needs to be fixed is that when you hover over an icon, go to the preview and return to the icon, the preview closes (and opens again after the time set in Hover Window Open Delay setting)

@ShlomoCode ShlomoCode mentioned this pull request Aug 5, 2024
Merged
3 tasks
@ShlomoCode ShlomoCode force-pushed the main branch 2 times, most recently from d033f5c to 148e81f Compare August 11, 2024 21:46
This was referenced Aug 13, 2024
Closed
Merged
@ShlomoCode
Revert "refactor: directly use NSEvent.mouseLocation"
ec474f3 
This reverts commit bbe8bb1.

# Conflicts:
#	DockDoor/logic/DockObserver.swift
@ShlomoCode ShlomoCode closed this Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejbills ejbills ejbills left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ShlomoCode @hasansultan92 @ejbills