-
Notifications
You must be signed in to change notification settings - Fork 23
Run transport check in a new thread to prevent UI freeze #291
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
base: master
Are you sure you want to change the base?
Run transport check in a new thread to prevent UI freeze #291
Conversation
IAdtTransportCheckData checkData = AbapGitWizardPull.this.transportService.check(checkRef, packageRef.getPackageName(), | ||
true); | ||
AbapGitWizardPull.this.transportPage.setCheckData(checkData); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor refactoring and exception handling suggestions:
- Shall we move the transport check into a separate method which returns the check data?
monitor.done
can be added in a finally block- There are mainly two exceptions from the process, InterruptedException ( caused if it is cancelled ) and InvocationTargetException ( when any exception occurs inside the code block ). Added some suggestions for the handling of these. Please have a look and also test it once.
private IAdtTransportCheckData getTransportCheckData(IAdtObjectReference packageRef) {
WizardPage currentPage = ((WizardPage) getContainer().getCurrentPage());
IAdtObjectReference checkRef = IAdtCoreFactory.eINSTANCE.createAdtObjectReference();
checkRef.setUri(packageRef.getUri());
final IAdtTransportCheckData[] checkData = new IAdtTransportCheckData[1];
try {
getContainer().run(true, true, monitor -> {
try {
monitor.beginTask(Messages.AbapGitWizardPageTransportSelection_transport_check, IProgressMonitor.UNKNOWN);
checkData[0] = AbapGitWizardPull.this.transportService.check(checkRef, packageRef.getPackageName(), true);
} finally {
monitor.done();
}
});
return checkData[0];
} catch (Exception exception) {
handleError(exception, currentPage);
}
return null;
}
private void handleError(Exception exception, WizardPage currentPage) {
currentPage.setPageComplete(false);
Throwable cause = exception instanceof InvocationTargetException ? exception.getCause() : exception;
if (cause != null && cause instanceof OutDatedClientException) {
AdtUtilUiPlugin.getDefault().getAdtStatusService().handle(cause, null);
} else {
currentPage.setMessage(cause != null ? cause.getMessage() : exception.getMessage(), DialogPage.ERROR);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I will modularize the transport check data code
org.abapgit.adt.ui/src/org/abapgit/adt/ui/internal/wizards/AbapGitWizardPull.java
Show resolved
Hide resolved
private void handleOutdatedClientException(Exception e) { | ||
|
||
Display.getDefault().asyncExec(() -> { | ||
WizardDialog d = (WizardDialog) getContainer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d -> dialog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
} | ||
} | ||
}); | ||
AdtUtilUiPlugin.getDefault().getAdtStatusService().handle(e, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done after d.close() inside the thread? Will that work? So that only after closing the wizard we will open this dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that should work.
page.setPageComplete(false); | ||
Throwable cause = exception instanceof InvocationTargetException ? exception.getCause() : exception; | ||
if (cause != null && cause instanceof OutDatedClientException) { | ||
Display.getDefault().asyncExec(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could call the new method for handling the OutDatedClientException right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I forgot to change this implementation with method name
fix: Run transport check in a new thread to prevent UI freeze
The previous implementation of the transport check was running on the main UI
thread, which caused the application to become unresponsive during long
operations.
This change moves the transport check to a ModalContext thread, ensuring the
UI remains responsive and providing visual feedback to the user via a modal
progress dialog.