-
Notifications
You must be signed in to change notification settings - Fork 397
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
Cb 4270 row counter add cancel button for big tables #2545
Cb 4270 row counter add cancel button for big tables #2545
Conversation
webapp/packages/plugin-data-viewer/src/DatabaseDataModel/IDatabaseDataSource.ts
Outdated
Show resolved
Hide resolved
webapp/packages/plugin-data-viewer/src/DataViewerTableService.ts
Outdated
Show resolved
Hide resolved
webapp/packages/plugin-data-viewer/src/DatabaseDataModel/DatabaseDataSource.ts
Outdated
Show resolved
Hide resolved
@@ -163,6 +163,7 @@ export class SqlQueryResultService { | |||
|
|||
// TODO: we need to dispose table model, but don't close execution context, so now we only | |||
const model = this.tableViewerStorageService.get(group.modelId); | |||
model?.source.cancelLoadTotalCount(); |
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.
since there is no dispose, we need to cancel it in result set tab manually on close
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.
then it's a bug cuz we need to call dispose for each unused table model
but in SQL editor we have special behavior where we can reuse the same model for different queries so check it please
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.
you can move cancelLoadTotalCount call to cancel fn
const cancelled = model.source.cancelLoadTotalCountTask?.cancelled; | ||
|
||
if (!cancelled) { | ||
notificationService.logException(e, 'data_viewer_total_count_cancel_failed_title', typeof e === 'string' ? e : undefined); |
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.
why do we need typeof e === 'string' ? e : undefined here?
return <TotalCountAction loading={loading} resultIndex={resultIndex} model={model} onClick={loadTotalCount} />; | ||
}); | ||
|
||
const CancelTotalCountAction = observer(function CancelTotalCountAction({ onClick }: { onClick: VoidFunction }) { |
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.
Please do not declare multiple components in one file
@@ -39,6 +46,7 @@ export default [ | |||
['data_viewer_refresh_result_set', 'Refresh result set'], | |||
['data_viewer_total_count_tooltip', 'Get total count'], | |||
['data_viewer_total_count_failed', 'Failed to get total count'], | |||
['data_viewer_total_count_cancel_failed_title', 'Failed to cancel getting of total count'], |
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.
I am not sure that we should create new message token as "Task was cancelled" is throw by the task itself
webapp/packages/plugin-data-viewer/src/DatabaseDataModel/DatabaseDataSource.ts
Outdated
Show resolved
Hide resolved
webapp/packages/plugin-data-viewer/src/DatabaseDataModel/IDatabaseDataSource.ts
Outdated
Show resolved
Hide resolved
…extended classes
|
||
async closeResults(results: IDatabaseResultSet[]): Promise<void> { | ||
await this.connectionExecutionContextService.load(); | ||
|
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.
await this.connectionExecutionContextService.load();
can be removed
model?.source.closeResults?.(model.getResults()); | ||
model?.cancel(); |
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.
model.source.closeResults(model.getResults());
model.cancel();
65ef929
to
d894010
Compare
<span className={styles.cancelText}>{translate('ui_processing_cancel')}</span> | ||
<Container | ||
className={s(style, { action: true })} | ||
title={loading ? translate('ui_processing_canceling') : translate('ui_processing_cancel')} |
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.
translate(loading ? 'ui_processing_canceling' : 'ui_processing_cancel')
if (this.currentTask) { | ||
await this.currentTask.cancel(); | ||
} | ||
await Promise.all([this.currentTask?.cancel(), super.cancel()]); |
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.
await super.cancel();
await this.currentTask?.cancel()
async cancel(): Promise<void> { | ||
await Promise.all([this.cancelLoadTotalCount(), super.cancel()]); | ||
} |
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.
await super.cancel();
await this.cancelLoadTotalCount();
loading: boolean; | ||
} | ||
|
||
export const CancelTotalCountAction = observer(function CancelTotalCountAction({ onClick, loading }: Props) { |
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.
please use observer<Props>
if (this.currentTask) { | ||
await this.currentTask.cancel(); | ||
} | ||
await Promise.all([this.currentTask?.cancel(), super.cancel()]); |
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.
await super.cancel();
await this.currentTask?.cancel();
No description provided.