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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(animations): add window move animations #597

Closed
wants to merge 6 commits into from

Conversation

LGUG2Z
Copy link
Owner

@LGUG2Z LGUG2Z commented Nov 25, 2023

No description provided.

@LGUG2Z LGUG2Z force-pushed the animate-window-move branch 2 times, most recently from 482a8b8 to 5651986 Compare November 27, 2023 00:12
@thearturca
Copy link

thearturca commented Nov 27, 2023

@LGUG2Z Hi there. I was watching your videos and came up with idea to animate border too. The idea is to shrink border in size to be smaller than current focused window and then raise it up in new focused window position. So we have 2 part of transition - out and in.

I did some progress on that but get stuck on several issue.

Border demo (WIP)

border-animation.mp4

Issues

  1. I'm confused by this line. Why does it subtract left from right?
    Because of that I cant get right calculations for border animations target postions.
    In case of moving border from left window to top-right window (check video above) I'm getting window rect where left is bigger than right. And when we calculate in_start_rect, its way left than the target rect. But border end up positioned in right place. I dont get it 馃し
// window rect
[komorebi\src\border.rs:197] window_rect = Rect {
    left: 961,
    top: 58,
    right: 948,
    bottom: 501,
}
// current border rect. Before transition
[komorebi\src\border.rs:198] prev_border_rect = Rect {
    left: 14,
    top: 54,
    right: 942,
    bottom: 1012,
}
// calculating center point in current focused window
[komorebi\src\border.rs:199] out_new_center_point_top = 479
[komorebi\src\border.rs:200] out_new_center_point_left = 464
// calculated rect for first part of transition. It should be smaller than current window rect by 50%
[komorebi\src\border.rs:201] out_new_rect = Rect {
    left: 239,
    top: 266,
    right: 703,
    bottom: 745,
}
// calculating center point in new focused window
[komorebi\src\border.rs:202] in_start_center_point_top = 224
[komorebi\src\border.rs:203] in_start_center_point_left = -11
// calculated rect for second part of transition. It should be smaller than target rect by 50%
[komorebi\src\border.rs:204] in_start_rect = Rect {
    left: 477,
    top: 139,
    right: 465,
    bottom: 363,
}
// target border rect. After transition
[komorebi\src\border.rs:205] rect = Rect {
    left: 964,
    top: 54,
    right: 942,
    bottom: 502,
}
  1. When I change focus by mouse click update focus event fires only one time and border transition works normally. But when I change focus via whkd (alt+direction) or when move windows (alt+shift+direction), Komorebi get two events at the same time and transitions brokes. Need to debug that to understand whats going on

I dont know should I dig further or give up with that idea because it's produces more bugs and issue? 馃樅

@LGUG2Z
Copy link
Owner Author

LGUG2Z commented Nov 27, 2023

I'm confused by this line. Why does it subtract left from right?

This is a pretty old piece of code and I don't quite remember at this point 馃槄 However, if you remove the transactions the border goes absolutely way too wide to the right and to the bottom lol.

I think the doubled events relates to the focus() being triggered by komorebic focus here: https://github.com/LGUG2Z/komorebi/blob/master/komorebi/src/window_manager.rs#L1203

Which then triggers another focus() here: https://github.com/LGUG2Z/komorebi/blob/master/komorebi/src/process_event.rs#L218

Maybe we can add another WindowManagerEvent to be called in the first instance, which would trigger the handler for this in the process_event.rs file? 馃

@LGUG2Z
Copy link
Owner Author

LGUG2Z commented Nov 27, 2023

Can you try making this change in window_manager.rs and see if the events are still doubled for you?

        // self.focused_window_mut()?.focus(self.mouse_follows_focus)?;

        WINEVENT_CALLBACK_CHANNEL
            .lock()
            .0
            .send(WindowManagerEvent::UpdateFocusedWindowBorder(
                *self.focused_window()?,
            ))?;

@LGUG2Z
Copy link
Owner Author

LGUG2Z commented Nov 27, 2023

https://github.com/LGUG2Z/komorebi/blob/master/komorebi/src/window_manager.rs#L1329

I think you can also try removing this line for moves, which is the culprit for the focus_container() being called twice on MoveWindow 馃

@LGUG2Z
Copy link
Owner Author

LGUG2Z commented Nov 27, 2023

@thearturca I've been noticing quite a lot of issues with the ALT key "sticking" (for lack of a better term) when using this branch; have you experienced this at all?

@thearturca
Copy link

@LGUG2Z You've missed komorebic commands. Its still Animate instead of Animation
https://github.com/LGUG2Z/komorebi/pull/597/files#diff-669eb0fce6967fea49390c1ec784c3c4df5a95f95853066d97720a33947503ccR1082-R1090

@thearturca
Copy link

Can you try making this change in window_manager.rs and see if the events are still doubled for you?

        // self.focused_window_mut()?.focus(self.mouse_follows_focus)?;

        WINEVENT_CALLBACK_CHANNEL
            .lock()
            .0
            .send(WindowManagerEvent::UpdateFocusedWindowBorder(
                *self.focused_window()?,
            ))?;

Can you try making this change in window_manager.rs and see if the events are still doubled for you?

        // self.focused_window_mut()?.focus(self.mouse_follows_focus)?;

        WINEVENT_CALLBACK_CHANNEL
            .lock()
            .0
            .send(WindowManagerEvent::UpdateFocusedWindowBorder(
                *self.focused_window()?,
            ))?;

I've tried that but it didnt help. Still getting 3 calls of border.set_position(...). From both move window and change focus via whkd. Gonna dig through that later.

I have some news about first issue that mentioned here. From in Rect struct is converting top, left, bottom, right to top, left, height, width. And WndowsApi::set_pos(...) takes top, left, height, width. Maybe I cant get right calculations for border now. But Rect struct has invalid properties names. It should be top, left, height, width instead of top, left, bottom, right. That tricked me 馃檪

@LGUG2Z LGUG2Z force-pushed the animate-window-move branch 3 times, most recently from c57f79b to a012262 Compare December 2, 2023 23:02
thearturca and others added 6 commits December 3, 2023 09:06
Commit to pull changes from master
At current state animation is working. Configs are available for it.
animation enable/disable
animation-duration {duration in ms}
animation-ease -e {ease func. See --help}

Two major problems for my implementation:
1. Google Chrome window is broken and I don't know how to implement fix
   for it.
2. Border window does not update properly with window. Dont know how to
   fix it.
This commit fixes an issue where the active window border would not
properly update when animations were enabled. Various clippy lints have
also been addressed, but I still need to look into the infinite loop
lint that has been marked with a todo comment.
This commit ensures that the UpdateFocusedWindowBorder
WindowManagerEvent which is handled in process_event is used to position
the mouse correctly on the moved window once the animation has been
completed.

A pending clippy lint in animation.rs has also been addressed.
@LGUG2Z
Copy link
Owner Author

LGUG2Z commented Dec 4, 2023

With all of the latest changes from master merged in I think that the animations code for windows seems stable enough to merge into the master branch. I do sometimes get issues with how the windows are drawn, particularly web view windows, but it's quite rare and I think that we can be explicit about this possibility in the documentation if users decide to enable animations.

What do you think about getting this work merged and then opening a separate PR for border animations?

@thearturca
Copy link

With all of the latest changes from master merged in I think that the animations code for windows seems stable enough to merge into the master branch. I do sometimes get issues with how the windows are drawn, particularly web view windows, but it's quite rare and I think that we can be explicit about this possibility in the documentation if users decide to enable animations.

What do you think about getting this work merged and then opening a separate PR for border animations?

@LGUG2Z
Sorry for delay in replying to that PR
I think we still have to implement animation canceling or other way to prevent this bug:

chrome_bug.mp4

Its occurs when komorebi perform two animation in different direction. Maybe we should add some WIN32 flag to position method idk.
Imo its critical issue for now. Because you have to restart chrome or just dont use it 馃檪

@mmikeww
Copy link
Contributor

mmikeww commented Feb 9, 2024

Its occurs when komorebi perform two animation in different direction

If you are moving two+ windows simultaneously, you could look into using DeferWindowPos which should be faster/smoother

@LGUG2Z LGUG2Z force-pushed the master branch 3 times, most recently from 6a03849 to f519cba Compare February 18, 2024 23:15
LGUG2Z added a commit that referenced this pull request Feb 25, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Feb 25, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
@LGUG2Z
Copy link
Owner Author

LGUG2Z commented Feb 25, 2024

Continuing this in #685 - rebasing with all of the commits was a nightmare so I just took the changes onto a new branch with a single commit so I only had to resolve conflicts once.

@LGUG2Z LGUG2Z closed this Feb 25, 2024
raggi pushed a commit to raggi/komorebi that referenced this pull request Feb 27, 2024
This commit contains all the changes of LGUG2Z#597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Feb 27, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Feb 27, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
raggi pushed a commit that referenced this pull request Feb 27, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
raggi pushed a commit that referenced this pull request Feb 27, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
raggi pushed a commit that referenced this pull request Feb 27, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Mar 26, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Mar 26, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Mar 26, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 21, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 21, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 21, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 23, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 26, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 28, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 30, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 30, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 30, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.
LGUG2Z added a commit that referenced this pull request Apr 30, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.

fix(animation): Fixed cancelling logic
(57e9b2f)

Added static animation state manager for tracking "in_progress" and
"is_cancelled" states. The idea is not to have states in Animation
struct but to keep them in HashMap<hwnd, AnimationState> behind
reference (Arc<Mutex<>>). So we each animation frame we have access to
state and can cancel animation if we have to.

Need review and testings

refactor(animation): avoid unwrap
(fa6d5bb)
LGUG2Z added a commit that referenced this pull request May 1, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.

fix(animation): Fixed cancelling logic
(57e9b2f)

Added static animation state manager for tracking "in_progress" and
"is_cancelled" states. The idea is not to have states in Animation
struct but to keep them in HashMap<hwnd, AnimationState> behind
reference (Arc<Mutex<>>). So we each animation frame we have access to
state and can cancel animation if we have to.

Need review and testings

refactor(animation): avoid unwrap
(fa6d5bb)

fix(animation): Move cancel call to Animation struct
(306513f)

Only focused window was cancelling its animation because we call cancel
in window::set_position and waiting for its cancelling. And because we
waiting for cancelling second window is still moving. Second window will stop
moving only after the first window. So I moved `cancel` call to
Animation struct so its happening in its own thread and doesn't block
others animation moves and cancels.
LGUG2Z added a commit that referenced this pull request May 12, 2024
This commit contains all the changes of #597 to make it easier to rebase
with the latest changes on master post-v0.1.21.

fix(animation): Fixed cancelling logic
(57e9b2f)

Added static animation state manager for tracking "in_progress" and
"is_cancelled" states. The idea is not to have states in Animation
struct but to keep them in HashMap<hwnd, AnimationState> behind
reference (Arc<Mutex<>>). So we each animation frame we have access to
state and can cancel animation if we have to.

Need review and testings

refactor(animation): avoid unwrap
(fa6d5bb)

fix(animation): Move cancel call to Animation struct
(306513f)

Only focused window was cancelling its animation because we call cancel
in window::set_position and waiting for its cancelling. And because we
waiting for cancelling second window is still moving. Second window will stop
moving only after the first window. So I moved `cancel` call to
Animation struct so its happening in its own thread and doesn't block
others animation moves and cancels.
thearturca added a commit to thearturca/komorebi that referenced this pull request May 18, 2024
This commit contains all the changes of LGUG2Z#597 to make it easier to rebase
with the latest changes on master post-v0.1.21.

fix(animation): Fixed cancelling logic
(57e9b2f)

Added static animation state manager for tracking "in_progress" and
"is_cancelled" states. The idea is not to have states in Animation
struct but to keep them in HashMap<hwnd, AnimationState> behind
reference (Arc<Mutex<>>). So we each animation frame we have access to
state and can cancel animation if we have to.

Need review and testings

refactor(animation): avoid unwrap
(fa6d5bb)

fix(animation): Move cancel call to Animation struct
(306513f)

Only focused window was cancelling its animation because we call cancel
in window::set_position and waiting for its cancelling. And because we
waiting for cancelling second window is still moving. Second window will stop
moving only after the first window. So I moved `cancel` call to
Animation struct so its happening in its own thread and doesn't block
others animation moves and cancels.
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.

None yet

3 participants