dreamcat4 / skippy-xd

A full-screen Exposé-style standalone task switcher for X11.
GNU General Public License v2.0
100 stars 12 forks source link

Animations isn't switched off cleanly #45

Closed dreamcat4 closed 1 year ago

dreamcat4 commented 1 year ago

Setting animationDuration = 0... this is supposed to switch off animations feature. However there is remaining still either 1 or 2 frames before actually displaying the final skippy mainwin state.

This is comparing the previous old (stable version) of skippy before animation feature was merged. I think it should be a solvable problem? If some extra special case handling were to be added. To avoid such glitching, but especially the '2nd frame' state is what is most distracting. The initial screen blank frame might be ok to keep, however it also is a benefit to remove too. Since then they latency of displaying the skippy window is non-existant. It just immedately appear without and delay whatsoever.

So if we can manage to keep animations feature, but add back those old behaviour (i.e. skip over code path). Then that would be pretty good outcome.

felixfung commented 1 year ago

Good point. I am working on the daemon+iconized code, which involves some refactoring of the do_layout() code, which is somewhat related to this code. So I can do this after completing that one.

Meanwhile, feel free to work on this issue, on top of my head we can add an additional bool to indicate complete switch off of animation, or probably a more logical way would also do.

felixfung commented 1 year ago

Taking another look at the code, a one line change from

if (animating)

to

if (animating && ps->o.animationDuration != 0)

should do the trick.

dreamcat4 commented 1 year ago

should do the trick.

The solution (as suggested) did not actually work, it seems to be insufficient to achieve the stated goal. In order to come to a better solution is necessary to go back and take deeper look at what else is happening in your new animations code. And work out how best to improve / include the feature.

felixfung commented 1 year ago

Will do. As said above, I am reworking the main loop, which might change this bit of code as well.

felixfung commented 1 year ago

Can you please try moving this 4 lines https://github.com/dreamcat4/skippy-xd/blob/61f979b16ec6b67d94f7956670c3c35c36e7bdc4/src/skippy.c#L612

to https://github.com/dreamcat4/skippy-xd/blob/61f979b16ec6b67d94f7956670c3c35c36e7bdc4/src/skippy.c#L749

dreamcat4 commented 1 year ago

ah ok. indeed seems good idea... and looks like it should help some of those observed issue.

just i also thought i noticed some additional other places / assumptions elsewhere added by these new animations code. so perhaps more changes are needed (in addition)

should have noted that down but was then reviewing old diff of only the original 1st animations PR.... which has already had subsequent changes / fixes by now

will take another look and be trying this, and report back tomorrow

felixfung commented 1 year ago

just i also thought i noticed some additional other places / assumptions elsewhere added by these new animations code. so perhaps more changes are needed (in addition)

In my current code the change above seems to have fixed the intermediate frames. But that is intermingled with a lot of other changes though, including animation code.

dreamcat4 commented 1 year ago

Tried again today (on master). There's a variety of different specifics i tried. (perhaps not quite correctly). Anyhow the short answer is: it still didn't work. Either with or without lazy transitions was a different resulting behaviour.

With lazyTrans = true it does render out the mainwin, but it's empty (black screen). No clientwins are actually being drawn to screen. However i can still use skippy to change windows. The program just isn't displaying them visually

With lazyTrans = false - then nothing is shown(at all), and skippy program exits. It just quits itself:

[id:~/.dev/skippy-xd] master(+18/-9)+ ± skippy-xd --activate
config_load(): config file found. using "/etc/xdg/skippy-xd.rc"
activate_window_picker(): Activating window picker...
send_command_to_daemon_via_fifo(): Sending command...
mainloop(): Received pipe command: 1
mainloop(): case PIPECMD_ACTIVATE_WINDOW_PICKER:
mainloop(): activate = true;
mainloop(): PIPEHUP on pipe "/tmp/skippy-xd-fifo".
θ61° [id:~/.dev/skippy-xd] master(+18/-9)+ ± 

Here is my diff, it's slightly different to your suggestion (but that might not impact)

diff --git a/src/skippy.c b/src/skippy.c
index d10d21b..72da433 100644
--- a/src/skippy.c
+++ b/src/skippy.c
@@ -609,11 +609,6 @@ skippy_run_init(MainWin *mw, Window leader) {
                return false;
        }

-       /* Map the main window and run our event loop */
-       if (!ps->o.lazyTrans)
-               mainwin_map(mw);
-       XFlush(ps->dpy);
-
        return true;
 }

@@ -684,8 +679,8 @@ mainloop(session_t *ps, bool activate_on_start) {
                        }

                        first_animated = time_in_millis();
-                       if (ps->o.animationDuration <= 0) {
-                               ps->o.animationDuration = 1;
+                       if (ps->o.animationDuration < 0) {
+                               ps->o.animationDuration = 0;
                        }
                }
                if (mw)
@@ -732,7 +727,7 @@ mainloop(session_t *ps, bool activate_on_start) {
                        return;

                // animation!
-               if (animating) {
+               if (animating && ps->o.animationDuration != 0) {
                        if(time_in_millis() - last_animated > mw->poll_time) {
                                last_animated = time_in_millis();
                                long timeslice = time_in_millis() - first_animated;
@@ -745,10 +740,24 @@ mainloop(session_t *ps, bool activate_on_start) {
                                        focus_miniw_adv(ps, mw->client_to_focus,
                                                        ps->o.movePointerOnStart);
                                }
-
                        }
+                       if (activate_on_start && ps->mainwin) {
+                               /* Map the main window and run our event loop */
+                               if (!ps->o.lazyTrans)
+                                       mainwin_map(ps->mainwin);
+                               XFlush(ps->dpy);
+                       }
+
                        continue; // while animating, do not allow user actions
                }
+               else {
+                       if (activate_on_start && ps->mainwin) {
+                               /* Map the main window and run our event loop */
+                               // if (!ps->o.lazyTrans)
+                                       mainwin_map(ps->mainwin);
+                               XFlush(ps->dpy);
+                       }
+               }

                // Poll for events
                int timeout = (pending_damage && mw && mw->poll_time > 0 ?
[id:~/.dev/skippy-xd] master(+18/-9)+ ± 

My feeling is maybe there is some other things you did when actually calculating the clientwin positions to render each frame. That is now wrong (or does not work) for 0 frames of animation. This was code in the original animations PR (which sorry i didn't review at the time, but have since encountering this issue).

You know, you changed some of the stuff in there too, which is not really my area of expertise i'm afraid. So cannot really finish off fixing this... where is it now. Either animate() isn't being called (at all, of course it would not when 0, code is skipped)

Or in clienwin_move() function, here was original changes to it

https://github.com/dreamcat4/skippy-xd/pull/21/files#diff-493c111b9f7ab46b0063c9ee71e6b0aec88057f86a1c4028ae86de16945a2f93R404

Sorry it's probably not much help to find the solution (in old code), and some distractions for you (i do not wish to distract unduly, for some less significant matters).

felixfung commented 1 year ago

Then it must be a combination of refactoring I am currently doing that may be fixing this issue.

felixfung commented 1 year ago

@dreamcat4 can you confirm whether #41 have fixed this?

dreamcat4 commented 1 year ago

Thanks @felixfung indeed the iconized code PR did entirely solve this issue for me too. Very grateful, and your PR for that work is merged now into master.