Skip to content

windows-set: avoid frame race condition#98

Merged
alphapapa merged 2 commits intoalphapapa:masterfrom
jdtsmith:fix-frame-race
Jul 10, 2024
Merged

windows-set: avoid frame race condition#98
alphapapa merged 2 commits intoalphapapa:masterfrom
jdtsmith:fix-frame-race

Conversation

@jdtsmith
Copy link
Contributor

In discussion #78 I suggested a loop over activity names, calling activities-resume. This doesn't work reliably, because of the HACK in activities--windows-set, which uses an "immediate" timer. When rapidly resuming activities in a loop, by the time the timer is called, the frame will have changed, and the activities' contents are bungled.

@jdtsmith
Copy link
Contributor Author

window-persistent-parameters will no longer be let-bound by the time the nil timer runs, so it needs to be set inside the timer callback; implemented that too.

@jdtsmith
Copy link
Contributor Author

jdtsmith commented Jul 8, 2024

Anything else need a look here?

@alphapapa
Copy link
Owner

Anything else need a look here?

Looking at this code again, now that you've apparently fixed an outstanding bug with the binding of window-persistent-parameters, I'm guessing that we should remove (setf window-persistent-parameters (copy-sequence activities-window-persistent-parameters)) from the top of the function; what do you think?

Also, forgive the nitpicking, but I'd suggest simplifying the frame binding one step further by making the lambda take the frame as its argument, and then passing (selected-frame) as the argument to the timer.

@alphapapa alphapapa added this to the 0.8 milestone Jul 9, 2024
@alphapapa alphapapa added the bug Something isn't working label Jul 9, 2024
@alphapapa alphapapa self-assigned this Jul 9, 2024
@jdtsmith
Copy link
Contributor Author

jdtsmith commented Jul 9, 2024

Looking at this code again, now that you've apparently fixed an outstanding bug with the binding of window-persistent-parameters, I'm guessing that we should remove (setf window-persistent-parameters (copy-sequence activities-window-persistent-parameters)) from the top of the function; what do you think?

I actually don't know what function that copying served vs. the let-binding already happening in activities--window-state; maybe it was an attempt to workaround this escaping-the-let-bind bug. It effectively overrode the user's persistent-parameters, which seems non-ideal, if other packages or features used those. Removing this doesn't seem to cause problems, but then again I don't save any exotic state, so maybe that needs testing.

I also note that activities-set-window-persistent-parameters was mentioned in the doc for activities-window-persistent-parameters, but doesn't exist, so I removed that. Vestigial?

Also, forgive the nitpicking, but I'd suggest simplifying the frame binding one step further by making the lambda take the frame as its argument, and then passing (selected-frame) as the argument to the timer.

Sure, done. In fact I went all the way and passed the copied/bufferized state as an argument too.

jdtsmith and others added 2 commits July 9, 2024 19:38
window-set: bind frame in top let for race-condition fix

windows-set: bind window-persistent-parameters in timer callback

windows-set: pass selected frame as argument, don't copy params

Remove reference to non-existent set-window-persistent-parameters

window-set: further simplify immediate timer call
@alphapapa alphapapa merged commit 20ef360 into alphapapa:master Jul 10, 2024
@alphapapa
Copy link
Owner

Thanks, merged.

@alphapapa alphapapa modified the milestones: v0.8, v0.7.1 Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants