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

Cb 4270 row counter add cancel button for big tables #2545

Merged
merged 41 commits into from
Apr 23, 2024

Conversation

sergeyteleshev
Copy link
Contributor

No description provided.

@sergeyteleshev sergeyteleshev self-assigned this Apr 11, 2024
@@ -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();
Copy link
Contributor Author

@sergeyteleshev sergeyteleshev Apr 12, 2024

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

Copy link
Member

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

Copy link
Member

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);
Copy link
Member

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 }) {
Copy link
Member

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'],
Copy link
Member

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

@sergeyteleshev sergeyteleshev requested a review from Wroud April 15, 2024 14:44
@sergeyteleshev sergeyteleshev requested a review from Wroud April 15, 2024 18:27

async closeResults(results: IDatabaseResultSet[]): Promise<void> {
await this.connectionExecutionContextService.load();

Copy link
Member

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

Comment on lines 169 to 170
model?.source.closeResults?.(model.getResults());
model?.cancel();
Copy link
Member

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();

@sergeyteleshev sergeyteleshev requested a review from Wroud April 16, 2024 07:34
@yagudin10 yagudin10 force-pushed the CB-4270-row-counter-add-cancel-button-for-big-tables branch from 65ef929 to d894010 Compare April 18, 2024 13:28
<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')}
Copy link
Member

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()]);
Copy link
Member

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()

Comment on lines 24 to 26
async cancel(): Promise<void> {
await Promise.all([this.cancelLoadTotalCount(), super.cancel()]);
}
Copy link
Member

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) {
Copy link
Member

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()]);
Copy link
Member

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();

@Wroud Wroud merged commit 51ef899 into devel Apr 23, 2024
4 of 5 checks passed
@Wroud Wroud deleted the CB-4270-row-counter-add-cancel-button-for-big-tables branch April 23, 2024 06:00
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.

5 participants