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

Remove cancel dialog during Eclipse shutdown #1290

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Nov 7, 2023

Removes the dialog during Eclipse shutdown which only flashes up in my Eclipse SDK build without any option to read it or interact with it.

The currently dialog behavior is also highly inconsistent or non-functional.

The cancel dialog during Eclipse shutdown, does not allow to cancel the shutdown, it allow affects the history pruning in the hard-code assumption that pruning happens in (Math.abs(total - 4.0)).

Fixes #1269

@vogella vogella marked this pull request as draft November 7, 2023 22:34
Removes the dialog during Eclipse shutdown which only flashes up in my
Eclipse SDK build without any option to read it or interact with it.

The currently dialog behavior is also highly inconsistent or
non-functional.

The cancel dialog during Eclipse shutdown, does not allow to cancel the
shutdown, it allow affects the history pruning in the hard-code
assumption that pruning happens in  (Math.abs(total - 4.0)).

Fixes #1269
@vogella vogella force-pushed the flashy-cancel-dialog-during-shutdown branch from 5fb4762 to 9497226 Compare November 7, 2023 22:35
Copy link
Contributor

github-actions bot commented Nov 7, 2023

Test Results

0 files   -      900  0 suites   - 900   0s ⏱️ - 51m 54s
0 tests  -   7 468  0 ✔️  -   7 315  0 💤  - 153  0 ±0 
0 runs   - 23 553  0 ✔️  - 23 044  0 💤  - 509  0 ±0 

Results for commit 9497226. ± Comparison against base commit 5b62380.

@HeikoKlare
Copy link
Contributor

I will have a look once we have agreed on the solution concept in #1269 to not have separate discussions until we have agreed on an approach.

@basilevs
Copy link
Contributor

basilevs commented Nov 8, 2023

Is not this the only way to resolve a live lock between hanged jobs? I remember this dialog saving me a couple of times, because each job has its own cancel button (red square) and plugins often fight each other for workspace (and other) resources. I could stop less important jobs and allow more important jobs (like saving resources) to complete without waiting for their opponents.

@HeikoKlare
Copy link
Contributor

Is not this the only way to resolve a live lock between hanged jobs? I remember this dialog saving me a couple of times, because each job has its own cancel button (red square) and plugins often fight each other for workspace (and other) resources. I could stop less important jobs and allow more important jobs (like saving resources) to complete without waiting for their opponents.

This is only about a progress dialog for the save operation performed during workbench shutdown, so it will not affect any other job progress dialog shown. @basilebs You probably refer to progress dialogs popping up while using the IDE and not during shutdown, right?

@basilevs
Copy link
Contributor

basilevs commented Nov 8, 2023

You probably refer to progress dialogs popping up while using the IDE and not during shutdown, right?

Nope, a dialog with a list of jobs pops up on shutdown, if jobs are long running.

@HeikoKlare
Copy link
Contributor

I see, but then you probably refer to the "ordinary" progress dialog that shows all the jobs that are still running during shutdown? The one in this PR is really only a single progress bar for the save operation in which you can currently cancel one subtask of the save operation. It does not allow to cancel any other jobs.

@basilevs
Copy link
Contributor

basilevs commented Nov 8, 2023

The one in this PR is really only a single progress bar for the save operation in which you can currently cancel one subtask of the save operation

As long as there is only a single progress bar, it is indeed different from the one I've thought about. Thanks and sorry for the confusion.

@HeikoKlare
Copy link
Contributor

Thanks for the feedback, then the usecase you mention should not be affected. And no problem, thanks for asking! Could have also been that we missed some use case of the removed functionality.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

I like this simplification and the removal of the broken opportunity to cancel the history cleaning step. I would only propose a change to avoid a freezing UI in case the save operation takes rather long by using a busy cursor, which automatically opens a progress dialog in case the operation takes too long.

Comment on lines 470 to 476
try {
final ProgressMonitorJobsDialog p = new CancelableProgressMonitorJobsDialog(
null);

final boolean applyPolicy = ResourcesPlugin.getWorkspace()
.getDescription().isApplyFileStatePolicy();

IRunnableWithProgress runnable = monitor -> {
try {
if (applyPolicy)
monitor = new CancelableProgressMonitorWrapper(monitor, p);

status.merge(((Workspace) ResourcesPlugin.getWorkspace()).save(true, true, monitor));
} catch (CoreException e) {
status.merge(e.getStatus());
}
};
status.merge(((Workspace) ResourcesPlugin.getWorkspace()).save(true, true, new NullProgressMonitor()));

p.run(true, false, runnable);
} catch (InvocationTargetException e) {
status
.merge(new Status(IStatus.ERROR,
IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
IDEWorkbenchMessages.InternalError, e
.getTargetException()));
} catch (InterruptedException e) {
} catch (CoreException e) {
status.merge(new Status(IStatus.ERROR,
IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
IDEWorkbenchMessages.InternalError, e));
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned here, I would propose to execute the save operation in a busyCursorWhile of the ProgressManager , so that the UI does not freeze in case saving takes long. Instead a busy cursor will be shown first and when it takes a "long time" (currently more than 800ms), it will open a progress dialog.

ProgressManager.getInstance().busyCursorWhile(monitor -> {
	try {
		status.merge(((Workspace) ResourcesPlugin.getWorkspace()).save(true, true, monitor));
	} catch (CoreException e) {
		status.merge(new Status(IStatus.ERROR,
					IDEWorkbenchPlugin.IDE_WORKBENCH, 1,
					IDEWorkbenchMessages.InternalError, e));
	}
});

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

Successfully merging this pull request may close these issues.

Closing Eclipse IDE shows flashing popup if closing is really fast
3 participants