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

Feature/Question: Fade on Destroy #704

Closed
pijulius opened this issue Oct 4, 2021 · 18 comments
Closed

Feature/Question: Fade on Destroy #704

pijulius opened this issue Oct 4, 2021 · 18 comments
Milestone

Comments

@pijulius
Copy link
Contributor

pijulius commented Oct 4, 2021

Hi Guys,

I know we have the fade-out option when unmapping windows and that works perfectly fine. The questions I would have is that would it be possible to do the same when destroying windows too?

Played around a bit and was able to make it happen but unfortunately it's not 100% accurate, sometimes it does fade but most of the times it doesn't. I imagine it's because window in reality isn't there anymore but we do have the image of it, wouldn't it be possible to animate that static image in this case?

What I did for quick testing is simply replace destroy_win_start() content with unmap_win_start() and when WSTATE_UNMAPPED continue with the destroy.

What do you think guys? would this be possible at all or is the reason for not working 100% because X simply clears the window and there isn't anything we can do about it because we can't just animate a picture on the server without having the window?

Thank you in advance!

@pijulius
Copy link
Contributor Author

Ok, found the solution.

The solution is in picom.c itself, when a window is unmapped it will set the to_paint to false and so the fading will never happen because it isn't painted.

A simple if like this:
if (!*fade_running || w->opacity == w->opacity_target)
to_paint = false;

fixes this and so now fading out will animate even when closing windows.
A commit can be seen in my fork but please note that also includes the animations implemented by Daniel.

pijulius@7b674f9

Please let me know if you wish to make a pull request on this or if at all this is something you would want to commit to the original picom.

@TiZ-HugLife
Copy link

I'm not sure it's quite as simple as you say. I tried to merge whatever I could of that particular commit into yshui/next. I couldn't get the first part in because preprocess_paint seems to be an entirely different function. But with the rest of it, which I could get in, windows still instantly blink away when they are closed. I also tried only making the change to picom.c as suggested, but that doesn't make it work either.

Here's a patch of what I tried to do:

diff --git a/src/picom.c b/src/picom.c
index 7127fd7..4f3561b 100644
--- a/src/picom.c
+++ b/src/picom.c
@@ -719,6 +719,8 @@ static struct managed_win *paint_preprocess(session_t *ps, bool *fade_running) {
                    unlikely(w->base.id == ps->debug_window ||
                             w->client_win == ps->debug_window)) {
                        to_paint = false;
+                       if (!*fade_running || w->opacity == w->opacity_target)
+                               to_paint = false;
                } else if (!w->ever_damaged && w->state != WSTATE_UNMAPPING &&
                           w->state != WSTATE_DESTROYING) {
                        // Unmapping clears w->ever_damaged, but the fact that the window
diff --git a/src/win.c b/src/win.c
index af16273..20a9b30 100644
--- a/src/win.c
+++ b/src/win.c
@@ -1169,6 +1169,9 @@ void win_on_win_size_change(session_t *ps, struct managed_win *w) {

        // Invalidate the shadow we built
        win_set_flags(w, WIN_FLAGS_IMAGES_STALE);
+               if (w->state != WSTATE_DESTROYING)
+               win_set_flags(w, WIN_FLAGS_IMAGES_STALE);
+
        ps->pending_updates = true;
        free_paint(ps, &w->shadow_paint);
 }
@@ -2750,5 +2753,6 @@ win_stack_find_next_managed(const session_t *ps, const struct list_node *i) {
 /// Return whether this window is mapped on the X server side
 bool win_is_mapped_in_x(const struct managed_win *w) {
        return w->state == WSTATE_MAPPING || w->state == WSTATE_FADING ||
-              w->state == WSTATE_MAPPED || (w->flags & WIN_FLAGS_MAPPED);
+              w->state == WSTATE_MAPPED || w->state == WSTATE_UNMAPPING ||
+              w->state == WSTATE_DESTROYING || (w->flags & WIN_FLAGS_MAPPED);
 }

@yshui
Copy link
Owner

yshui commented Oct 28, 2021

Did you perhaps set no-fading-openclose to true?

@TiZ-HugLife
Copy link

No, it's explicitly set to false. All regular windows fade in on open, but instantly disappear on close. Menus and tooltips are the only types of windows that aren't doing that.

@pijulius
Copy link
Contributor Author

Please let me know if you want to have a pull request for this and will create one just wasn't sure if you are interested in this.

@yshui
Copy link
Owner

yshui commented Oct 29, 2021

OK, this is not supposed to happen. This is a bug then.

Yes, please open a PR for this, thanks!

TiZ-HugLife added a commit to TiZ-HugLife/configs-scripts that referenced this issue Nov 11, 2021
Just waiting on yshui/picom#704 for fades to be perfect.
@Exordio
Copy link

Exordio commented Jun 13, 2023

bug is still here...

@Onred
Copy link

Onred commented Aug 2, 2023

Any progress on this? This issue is pretty much the last bit of jank I need to work out of my system for it to be beautiful.

@allusive-dev
Copy link

allusive-dev commented Oct 29, 2023

Ok, found the solution.

The solution is in picom.c itself, when a window is unmapped it will set the to_paint to false and so the fading will never happen because it isn't painted.

A simple if like this: if (!*fade_running || w->opacity == w->opacity_target) to_paint = false;

fixes this and so now fading out will animate even when closing windows. A commit can be seen in my fork but please note that also includes the animations implemented by Daniel.

pijulius@7b674f9

Please let me know if you wish to make a pull request on this or if at all this is something you would want to commit to the original picom.

This issue is still present.

I am working on updating my fork of pijulius picom to yshui's picom v10.2.
So far everything is working with almost zero issues however, windows neither fade out nor have a unmap animation.

I have spent over 10 hours analyzing all the code side by side with meld and confirming that there are no key differences that are causing this.

No obvious errors are outputted when running picom and I checked that it wasn't an issue reading the config by creating a runtime argument which manually sets the unmap animation.

The line that pijulius mentioned as fixing the issue has not resolved it for me and from the looks of the recent comments it hasn't resolved it for others either.

Here is the repository if either of you are interested.

@allusive-dev
Copy link

Hello Again Everyone.

I have come back to let everyone know that I have resolved the issue.

I believe the issue was being caused by that in yshui/picom fade steps is a long long but in mine it is just long.
Aswell as a few other small things that could have resolved this.
I also noticed that unmap animations actually do not render if you have fading = false;

With that out of the way... Picom Allusive v1.0.0 will be releasing within 10 minutes of this comment.
I don't want to just completely self promote here so if you are interested please check out picom-allusive here

@yshui
Copy link
Owner

yshui commented Jan 20, 2024

hey, sorry for neglecting this.

I tried several simple apps (xev, xclock, xeyes, gedit, etc.) under awesomeWM and they all fade correctly when closed.

what's the app and WM combination that have this problem?

@yshui yshui added this to the v12 milestone Jan 20, 2024
@jasper-at-windswept
Copy link

I believe the issue was being caused by that in yshui/picom fade steps is a long long but in Compfy it is just long.

@yshui I think that this is the cause of the issue or at least related.

@DarioDarko
Copy link
Sponsor

i am using endeavourOS with openbox.

i tested alacritty, xev, pcmanfm, librewolf and nitrogen. theres little to no difference between these programs, only alacritty for some reason shows the animation more often. the fadeout is way more likely to happen, if theres only 1 window on the (virtual) desktop. as soon as theres 2+ windows on the desktop theyll fadeout very, very rarely.

this also affects animations of closing a window on all picom forks ive tested, besides picom-allusive/compfy. so at least for my setup i can confirm that jaspers fix solved the issue 100%. i was running his fork for the last couple of months without a single occurance of the problem.

if you need more information/testing from my side let me know.

@yshui
Copy link
Owner

yshui commented Jan 21, 2024

I believe the issue was being caused by that in yshui/picom fade steps is a long long but in Compfy it is just long.

@yshui I think that this is the cause of the issue or at least related.

hmm, interesting. so maybe the steps underflowed, or otherwise got corrupted somewhere and became a huge number? a wild guess.

@absolutelynothelix
Copy link
Collaborator

absolutelynothelix commented Jan 21, 2024

are there any warnings from picom when closing a window?

@yshui
Copy link
Owner

yshui commented Jan 21, 2024

OK, I've found the root cause.

@jasper-at-windswept it has absolutely nothing to do with long vs long long.

openbox destroys the client window before its frame window. When the client window is destroyed, we detach it from the frame window, now the frame window has client_win == 0. this unfortunately means it becomes equal to ps->debug_window when debug mode is not used. so this condition is hit:

w->client_win == ps->debug_window)) {

compfy doesn't have this problem because it added an extra condition in that branch:

https://github.com/allusive-dev/compfy/blob/b92d8b18771700a001f07de3a2f8748d91447c3b/src/compfy.c#L967

@yshui yshui closed this as completed in 81d137a Jan 21, 2024
@yshui
Copy link
Owner

yshui commented Jan 21, 2024

Should be fixed in next

yshui added a commit that referenced this issue Jan 21, 2024
This is a follow up to 81d137a and
bug #704. Basically a window will have a `XCB_NONE` as `client_win` if its
previous client_win detached and then the window itself is immediately
destroyed. Because the window is destroyed we couldn't call
`win_recheck_client` so its `client_win` will remain `XCB_NONE`.

However, it turns out we have a convention of setting `client_win` to
the window itself if windows that don't have a client window. So make
sure this convention is followed even for destroyed windows.

Doesn't really fix anything, just to make sure an invariant holds.

Related: #704

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
@jasper-at-windswept
Copy link

compfy doesn't have this problem because it added an extra condition in that branch:
https://github.com/allusive-dev/compfy/blob/b92d8b18771700a001f07de3a2f8748d91447c3b/src/compfy.c#L967

Ah alright. Glad you figured it out!

yshui added a commit that referenced this issue Jan 22, 2024
Fixes #704

Was: 81d137a
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
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

9 participants