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

fix(Tasks): task lifecycle managed in main #8144

Merged
merged 19 commits into from
Aug 26, 2024

Conversation

axel7083
Copy link
Contributor

@axel7083 axel7083 commented Jul 18, 2024

🔎 What does this PR do?

Makes all the Tasks being managed in the main.

📑 Notable change

new ipcs

  • tasks:clear-all -> window.clearTasks(): void
  • tasks:clear -> window.clearTask(taskId: string): void
  • tasks:execute -> window.executeTask(taskId: string): void

Interfaces change

Creating TaskInfo and NotificationTaskInfo in the api package.

ℹ️ Those objects are readonly, the renderer cannot modify any fields.

Creating Task and NotificationTask in the extension-api

ℹ️ Those objects are writable, and will be automatically notify the frontend if a field are edited. The logic to achieve this behaviour has been made through TaskImpl and NotificationTaskImpl classes.

⚠️ For now the Tasks and NotificationTask object cannot be obtain by the extensions, as this will be done in #7709

TaskManager change

  • No longer need to call taskManager.updateTask(task) as the Task intercept all setter inside TaskImpl class, it is automatically managed and propagated

ℹ️ This avoid several potential problem where the task are not updated properly.

🔨 Future work

This PR remove all the tasks created on the renderer side WITHOUT replacing them by equivalent on the main. This choice is to ease the testing of this PR.

The list of tasks to created has been listed here #7396 (comment)

🖼️ Screenshot / video of UI

task-management-backend-3.mp4

🐛 What issues does this PR fix or reference?

Fixes #7396
Prior work for #7709

Requires

How to test this PR?

🪲 Debug commands palette

You can add a series of debug commands to the palette to generate fake tasks.

Press F1 to open Command Palette

// packages/main/src/plugin/index.ts
    commandRegistry.registerCommand('debug-task-generate-info-notification', () => {
      taskManager.createNotificationTask({
        type: 'info',
        body: 'Hello world',
      });
    });

    commandRegistry.registerCommand('debug-task-generate-error-task', () => {
      const task = taskManager.createTask({
        title: 'Dummy error task'
      });
      task.error = 'Something went wrong on purpose :)';
      task.state = 'error';
    });

    commandRegistry.registerCommand('debug-task-generate-indeterminate-task', () => {
      taskManager.createTask({
        title: 'Dummy indeterminate task'
      });
    });

    commandRegistry.registerCommand('debug-task-generate-50-loading-task', () => {
      const task = taskManager.createTask({
        title: 'Dummy loading 50 task'
      });
      task.progress = 50;
    });

    commandRegistry.registerCommand('debug-task-generate-success-task', () => {
      const task = taskManager.createTask({
        title: 'Dummy indeterminate task'
      });
      task.state = 'success';
    });

    commandRegistry.registerCommand('debug-task-generate-navigation-task', () => {
      const task = taskManager.createTask({
        title: 'Dummy navigable task'
      });
      task.state = 'success';
      task.action = {
        name: 'Go to containers >',
        execute: (): void => {
          navigationManager.navigateToContainers();
        },
      };
    });

    commandRegistry.registerCommand('debug-task-generate-actionable-task', () => {
      const task = taskManager.createTask({
        title: 'Dummy actionable task'
      });
      task.action = {
        name: 'explode',
        execute: (): void => {
          messageBox
            .showMessageBox({
              type: 'info',
              title: 'Task action',
              message: 'Hello from task action in the main package!',
              buttons: ['Set task error', 'Clear task'],
            })
            .then(result => {
              console.log('showMessageBox', result);
              switch (result.response) {
                case 0:
                  task.state = 'error';
                  break;
                case 1:
                  task.dispose();
                  break;
              }
            })
            .catch((err: unknown) => {
              console.error(err);
            });
        },
      };
    });

    commandRegistry.registerCommandPalette(
      {
        command: 'debug-task-generate-info-notification',
        title: '[debug-task] generate-info-notification',
        icon: undefined,
      },
      {
        command: 'debug-task-generate-error-task',
        title: '[debug-task] generate-error-task',
        icon: undefined,
      },
      {
        command: 'debug-task-generate-indeterminate-task',
        title: '[debug-task] generate-indeterminate-task',
        icon: undefined,
      },
      {
        command: 'debug-task-generate-50-loading-task',
        title: '[debug-task] generate-50-loading-task',
        icon: undefined,
      },
      {
        command: 'debug-task-generate-success-task',
        title: '[debug-task] generate-success-task',
        icon: undefined,
      },
      {
        command: 'debug-task-generate-navigation-task',
        title: '[debug-task] generate-navigation-task',
        icon: undefined,
      },
      {
        command: 'debug-task-generate-actionable-task',
        title: '[debug-task] generate-actionable-task',
        icon: undefined,
      },
    );
  • Unit tests has been provided

@axel7083 axel7083 changed the title [WIP] fix(Tasks): task lifecycle managed in main fix(Tasks): task lifecycle managed in main Jul 18, 2024
@axel7083 axel7083 marked this pull request as ready for review July 18, 2024 13:22
@axel7083 axel7083 requested review from benoitf and a team as code owners July 18, 2024 13:22
@axel7083 axel7083 requested review from dgolovin, lstocchi, jeffmaury, cdrage, feloy and deboer-tim and removed request for a team July 18, 2024 13:22
@axel7083 axel7083 force-pushed the feature/improve-task-management branch from eccc79a to 73ddc86 Compare July 22, 2024 10:04
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

I see some changes that can't be done

  • Tasks leaking into API (we have notifications here, not tasks)
  • Tasks creation that have purely removed from renderer part (it says "to ease the PR") but it can't be merged using that approach.

@axel7083
Copy link
Contributor Author

axel7083 commented Jul 23, 2024

  • Tasks leaking into API (we have notifications here, not tasks)

I can place it in the packages/api while waiting for more discussion on #7709

  • Tasks creation that have purely removed from renderer part (it says "to ease the PR") but it can't be merged using that approach.

@benoitf my proposal was to make a follow-up PR which would replace those by creating them in the main. I will include them inside this PR; the list are available here #7396 (comment)

Here is a video demo with the latest commit

Screencast.from.2024-07-23.13-49-46.mp4

@axel7083 axel7083 requested a review from benoitf July 23, 2024 11:51
@axel7083 axel7083 force-pushed the feature/improve-task-management branch 2 times, most recently from 908791d to 3863310 Compare July 29, 2024 08:56
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Is it just me or the progressing tasks are not displayed? E.g if i stop a podman machine i don't see anything in the task manager

packages/main/src/plugin/index.ts Show resolved Hide resolved
packages/main/src/plugin/tasks/task-manager.ts Outdated Show resolved Hide resolved
@axel7083
Copy link
Contributor Author

Is it just me or the progressing tasks are not displayed? E.g if i stop a podman machine i don't see anything in the task manager

You were right, some tasks were missing. I updated the list of frontend tasks in #7396 (comment)

I think this PR is too big. I will put into draft, and see how to split it.

@axel7083 axel7083 marked this pull request as draft July 30, 2024 16:14
@axel7083 axel7083 force-pushed the feature/improve-task-management branch from 85ddcad to f6d1031 Compare July 31, 2024 07:22
@axel7083 axel7083 mentioned this pull request Jul 31, 2024
1 task
@axel7083 axel7083 force-pushed the feature/improve-task-management branch from f6d1031 to 04dd5dd Compare July 31, 2024 09:55
@axel7083
Copy link
Contributor Author

rebased after #8292

@axel7083 axel7083 marked this pull request as ready for review July 31, 2024 10:03
@axel7083 axel7083 requested a review from lstocchi July 31, 2024 12:27
@axel7083 axel7083 force-pushed the feature/improve-task-management branch from 588be29 to 2d0a344 Compare August 26, 2024 07:58
@axel7083
Copy link
Contributor Author

axel7083 commented Aug 26, 2024

Rebased after #8534

@benoitf
Copy link
Collaborator

benoitf commented Aug 26, 2024

it looks like we have some invalid code (unit test failing)

overall LGTM, lot of work 👍

@axel7083
Copy link
Contributor Author

it looks like we have some invalid code (unit test failing)

Yeah, some change introduced in other PRs appear after rebase. Fixed now ✔️

@axel7083 axel7083 requested a review from benoitf August 26, 2024 14:31
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

no changes in the external/extension-api so 👍

Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
…ovider status change etc.)

Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
Signed-off-by: axel7083 <42176370 [email protected]>
@axel7083 axel7083 force-pushed the feature/improve-task-management branch from 4024bcb to d2214eb Compare August 26, 2024 16:06
@axel7083 axel7083 enabled auto-merge (squash) August 26, 2024 16:06
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM

@axel7083 axel7083 merged commit 317abc7 into containers:main Aug 26, 2024
7 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.13.0 milestone Aug 26, 2024
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.

Merge all tasks management in main package
5 participants