[GH-ISSUE #881] Playback buttons are disabled after "Save as" #685

Closed
opened 2026-02-26 09:32:17 +03:00 by kerem · 6 comments
Owner

Originally created by @pawlos on GitHub (May 6, 2021).
Original GitHub issue: https://github.com/NickeManarin/ScreenToGif/issues/881

Originally assigned to: @pawlos on GitHub.

I've noticed a weird behavior of the editor. After saving an animation all of the Replay buttons are disabled (they were enabled before ofc).

Did some checking and the responsible for this is the _applyAction == null in the following code

private void Playback_CanExecute(object sender, CanExecuteRoutedEventArgs e)
{
    e.CanExecute = Project != null && Project.Frames.Count > 1 && !IsLoading && _applyAction == null;
}

as when Save is executed the _applyAction is set to _applyAction = SaveAsButton_Click; in the finally of SaveAsButton_Click . After that apparently it never gets nullified again. I guess this is done to prevent playing the video while saving/encoding is performed (although my simple/quick test with commenting out this line didn't show any issues of playing while saving).

I guess the fix would be to set _applyAction to null when encoding is done, but not sure if that's possible in the current architecture of the system?

Originally created by @pawlos on GitHub (May 6, 2021). Original GitHub issue: https://github.com/NickeManarin/ScreenToGif/issues/881 Originally assigned to: @pawlos on GitHub. I've noticed a weird behavior of the editor. After saving an animation all of the Replay buttons are disabled (they were enabled before ofc). Did some checking and the responsible for this is the `_applyAction == null` in the following code private void Playback_CanExecute(object sender, CanExecuteRoutedEventArgs e) { e.CanExecute = Project != null && Project.Frames.Count > 1 && !IsLoading && _applyAction == null; } as when Save is executed the `_applyAction` is set to `_applyAction = SaveAsButton_Click;` in the `finally` of `SaveAsButton_Click` . After that apparently it never gets nullified again. I guess this is done to prevent playing the video while saving/encoding is performed (although my simple/quick test with commenting out this line didn't show any issues of playing while saving). I guess the fix would be to set `_applyAction` to `null` when encoding is done, but not sure if that's possible in the current architecture of the system?
kerem 2026-02-26 09:32:17 +03:00
Author
Owner

@pawlos commented on GitHub (May 9, 2021):

@NickeManarin I could work on a fix for this, but as mentioned not sure if there's a clean way to do this with the current architecture of the app. Don't see it's doable without rewriting the big chunk of code. Any guidelines how you would try to solve this?

<!-- gh-comment-id:835804116 --> @pawlos commented on GitHub (May 9, 2021): @NickeManarin I could work on a fix for this, but as mentioned not sure if there's a clean way to do this with the current architecture of the app. Don't see it's doable without rewriting the big chunk of code. Any guidelines how you would try to solve this?
Author
Owner

@NickeManarin commented on GitHub (May 9, 2021):

The _applyAction should be set to null when the side panels are closed.
Like this, I believe:

if (await Task.Run(() => SaveAsync(preset)))
    ClosePanel(false, true);

I want to rework the Editor, with a new architecture in mind.
The current codebase is slow and hard to change.

Some of the behaviors (like this one that we are discussing, for example) made a bit of sense back then, but I believe that it can be removed with little adverse consequences.

And yes, this _applyAction is being used in a CanExecute to prevent the playback of the preview when a panel is open. But the primary use is to allow the user to press Apply/Save.

<!-- gh-comment-id:835829613 --> @NickeManarin commented on GitHub (May 9, 2021): The `_applyAction` should be set to null when the side panels are closed. Like this, I believe: ```cs if (await Task.Run(() => SaveAsync(preset))) ClosePanel(false, true); ``` I want to rework the Editor, with a new architecture in mind. The current codebase is slow and hard to change. Some of the behaviors (like this one that we are discussing, for example) made a bit of sense back then, but I believe that it can be removed with little adverse consequences. And yes, this `_applyAction` is being used in a `CanExecute` to prevent the playback of the preview when a panel is open. But the primary use is to allow the user to press Apply/Save.
Author
Owner

@pawlos commented on GitHub (May 10, 2021):

Thanks for the info. I think it wont be that easy as ClosePanel(false, true); as in this case ClosePanel(false, true); is called before setting the _applyAction in the finally block. But I'll look around more anyway.

Taking into the consideration the following I want to rework the Editor, with a new architecture in mind. should I not try to fix it in the current arch? Or the new one is long in the future?

<!-- gh-comment-id:837066053 --> @pawlos commented on GitHub (May 10, 2021): Thanks for the info. I think it wont be that easy as `ClosePanel(false, true);` as in this case `ClosePanel(false, true);` is called before setting the `_applyAction` in the finally block. But I'll look around more anyway. Taking into the consideration the following `I want to rework the Editor, with a new architecture in mind.` should I not try to fix it in the current arch? Or the new one is long in the future?
Author
Owner

@NickeManarin commented on GitHub (May 11, 2021):

I'm planning a redesign for the editor for v3.
I think that it's going to take a while for me to finish it.


Thanks for the info. I think it wont be that easy as ClosePanel(false, true); as in this case ClosePanel(false, true); is called before setting the _applyAction in the finally block. But I'll look around more anyway.

Ahh, sorry. I didn't see that.

<!-- gh-comment-id:837563698 --> @NickeManarin commented on GitHub (May 11, 2021): I'm planning a redesign for the editor for v3. I think that it's going to take a while for me to finish it. --------------------- > Thanks for the info. I think it wont be that easy as ClosePanel(false, true); as in this case ClosePanel(false, true); is called before setting the _applyAction in the finally block. But I'll look around more anyway. Ahh, sorry. I didn't see that.
Author
Owner

@pawlos commented on GitHub (May 12, 2021):

@NickeManarin I've created a PR. It's not the fix I was expecting to do, but I went through the code and I think this check for _applyAction in Playback was unnecessary in the first place (maybe before it looked differently). Did the testing and I don't see any unwanted behavior w/o this check. The playback is disabled as long as the side panel is opened and get enabled when it's being closed.

Please feel free to comment if you see something's not being right.

<!-- gh-comment-id:839866468 --> @pawlos commented on GitHub (May 12, 2021): @NickeManarin I've created a PR. It's not the fix I was expecting to do, but I went through the code and I think this check for `_applyAction` in Playback was unnecessary in the first place (maybe before it looked differently). Did the testing and I don't see any unwanted behavior w/o this check. The playback is disabled as long as the side panel is opened and get enabled when it's being closed. Please feel free to comment if you see something's not being right.
Author
Owner

@NickeManarin commented on GitHub (May 15, 2021):

Thanks. I merged the PR.

I could not find any other issue with it too.

<!-- gh-comment-id:841588504 --> @NickeManarin commented on GitHub (May 15, 2021): Thanks. I merged the PR. I could not find any other issue with it too.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/ScreenToGif#685
No description provided.