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.

Sign up for GitHub

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

Jump to bottom

Replace IProcess.WaitForProcessExitAsync with WaitForExitAsync #10802

Merged
merged 2 commits into from Mar 19, 2023
Merged

Replace IProcess.WaitForProcessExitAsync with WaitForExitAsync #10802

merged 2 commits into from Mar 19, 2023

Conversation

mstv
Copy link
Member

@mstv mstv commented Mar 15, 2023

Fixes #10398

Proposed changes

  • Replace IProcess.WaitForProcessExitAsync with cancellable variant of WaitForExitAsync as proposed by @vjdw

Screenshots

N/A

Test methodology

  • existing tests

Test environment(s)

  • Git Extensions 33.33.33
  • Build 70ebb97
  • Git 2.39.2.windows.1
  • Microsoft Windows NT 10.0.19045.0
  • .NET 6.0.14
  • DPI 96dpi (no scaling)
  • Microsoft.WindowsDesktop.App Versions
    Microsoft.WindowsDesktop.App 6.0.14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 7.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Mar 15, 2023
RussKie
RussKie reviewed Mar 16, 2023
View reviewed changes
Plugins/GitUIPluginInterfaces/IProcess.cs Outdated
/// <param name="token">An optional token to cancel the asynchronous operation.</param>
/// <returns>A task that will complete when the process has exited, cancellation has been requested, or an error occurs.</returns>
Task WaitForProcessExitAsync(CancellationToken token);
/// <returns>A task that yields the process's exit code, or <c>null</c> if this object was disposed before the process exited.</returns>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null? This would be extremely unexpected.
Also, the signature is suggesting the result is non-nullable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've copied this from WaitForExitAsync(void) - without checking. :(

RussKie
RussKie reviewed Mar 16, 2023
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 modulo the comment

@RussKie
Copy link
Member

RussKie commented Mar 16, 2023

Artifacts for validation: https://ci.appveyor.com/project/gitextensions/gitextensions/builds/46515268/artifacts

@RussKie RussKie mentioned this pull request Mar 16, 2023
Closed
gerhardol
gerhardol reviewed Mar 16, 2023
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem to work fine for me, regarding minimization and normal use.
From what I see, the APIs are used OK.
Nice!
(RussKie comments remains, null annotation there?)

gerhardol
gerhardol approved these changes Mar 19, 2023
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@mstv mstv merged commit e3229c9 into gitextensions:master Mar 19, 2023
@mstv mstv deleted the fix/i10398_minimized branch March 19, 2023 21:14
@ghost ghost added this to the vNext milestone Mar 19, 2023
@gerhardol
Copy link
Member

Note: I have seen this problem also in builds with this fix. However, the explicit Restore is done by window. Not reverting directly.

The workaround #10398 (comment)
should probably be renamed, so it does not kick in by default after update.

@gerhardol
Copy link
Member

After this change the problem occur less often, but often enough to be really irritating.
Only for minimized applications (was maybe that before too.)
A difference is that the context menu item Restore works in the same way as clicking. When the problem occur, I have to click twice on the miniature to restore it.

@vjdw
Copy link

vjdw commented Mar 23, 2023

After this change the problem occur less often, but often enough to be really irritating. Only for minimized applications (was maybe that before too.) A difference is that the context menu item Restore works in the same way as clicking. When the problem occur, I have to click twice on the miniature to restore it.

I'm finding the same thing intermittently. It's definitely better, I can now click a second time and the window will restore. Before it would never restore.

Maybe it's just a bad idea to start an external process while Windows is waiting on Git Extensions to process a WM_ACTIVATE request?

@gerhardol
Copy link
Member

Maybe it's just a bad idea to start an external process while Windows is waiting on Git Extensions to process a WM_ACTIVATE request?

I have a private patch to not run ls-files (to find if the repo status is changed externally) when activating, I still see the problem.
(git-status is not called when the app is minimized.)

@RussKie
Copy link
Member

RussKie commented Mar 24, 2023

I think we need to remove WM_ACTIVATE until we come up with a better solution rather than try to hack.

@gerhardol gerhardol mentioned this pull request Mar 28, 2023
Closed
gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Apr 1, 2023
@gerhardol
Restore from minimized: Configurable avoid async Git
ea40ffc 
Add an experimental setting to not run git-ls-files when the
window is minimized on activation, as async commands when
minimized seem to block the app from being restored.

Rename configuration added in gitextensions#10119 to
deactivate at upgrade as this setting may give a negative
experience after gitextensions#10802 mostly fixes the problem.
gerhardol added a commit to gerhardol/gitextensions that referenced this pull request Apr 1, 2023
@gerhardol
Restore from minimized: Configurable avoid async Git
66bf6d3 
Add an experimental setting to not run git-ls-files when the
window is minimized on activation, as async commands when
minimized seem to block the app from being restored.

Rename configuration added in gitextensions#10119 to
deactivate at upgrade as this setting may give a negative
experience after gitextensions#10802 mostly fixes the problem.
@gerhardol gerhardol mentioned this pull request Apr 1, 2023
Merged
gerhardol added a commit that referenced this pull request Apr 6, 2023
@gerhardol
Restore from minimized: Configurable avoid async Git (#10847)
cccfbc9 
Add an experimental setting to not run git-ls-files when the
window is minimized on activation, as async commands when
minimized seem to block the app from being restored.

Rename configuration added in #10119 to
deactivate at upgrade as this setting may give a negative
experience after #10802 mostly fixes the problem.
This was referenced Apr 7, 2023
Closed
Merged
@pmiossec pmiossec mentioned this pull request Apr 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie left review comments

@gerhardol gerhardol gerhardol approved these changes

Assignees

@mstv mstv

Labels
None yet
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

minimize Application doesn't open anymore in latest version
4 participants
@mstv @RussKie @gerhardol @vjdw