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.

Sign up for GitHub

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

Jump to bottom

Support tasks presentation options received from plugins #9248

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Support tasks presentation options received from plugins #9248

merged 1 commit into from
Apr 1, 2021

Conversation

alvsan09
Copy link
Contributor

@alvsan09 alvsan09 commented Mar 24, 2021 •
edited
Loading

Support tasks presentation options received from plugins

This expands the functionality introduced in pull request #6814

The following presentation options are now available for use by
plugins:

presentationOptions.reveal
presentationOptions.focus
presentationOptions.echo
presentationOptions.showReuseMessage
presentationOptions.panel
presentationOptions.clear

Fixes: #9239

Signed-off-by: Alvaro Sanchez-Leon alvaro.sanchez-leon@ericsson.com

What it does

  • Enables transferring task presentation options from plugins
  • Translates between numerical values and 'RevealKind' and 'PanelKind' string values to convert to/from DTO

How to test

  1. Prepare a plugin that provides tasks with all available presentation options, you can use the following as an example:
    plugin example , checkout branch: presentation-options
  2. you can install your plugin source repository under the plugins folder of your theia installation or a .vsix (generated via vsce package)
  3. Update your presentation options and compile the plugin
  4. Start your theia application and run the task provided by your plugin, verify the behavior of the presentation options apply.
    see Expected Output behaviour
  5. Update your plugin provided options and repeat from step 3.

Review checklist

Reminder for reviewers

@alvsan09 alvsan09 force-pushed the task_presentation_plugins branch from cb0d6e7 to 9f4be4c Compare March 24, 2021 21:33
RomanNikitenko
RomanNikitenko reviewed Mar 25, 2021
View reviewed changes
packages/task/src/common/task-protocol.ts Outdated
Never = 'never'
Always = 1,
Silent = 2,
Never = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

String values were provided within https://github.com/eclipse-theia/theia/pull/7982/files
Alvaro, could you check that the changes do not lead to the bug described in the PR description #7982.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @RomanNikitenko,
Very good catch ! I was not aware of the previous issue,
I have left the Enums untouched to prevent compatibility issues and opted to use a maps to translate between config / DTO.
Comments are very welcome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on this, I tested this for regressions and it is working well for me, still getting the correct stringified values 👍

@vince-fugnitto vince-fugnitto added the tasks issues related to the task system label Mar 25, 2021
@alvsan09 alvsan09 force-pushed the task_presentation_plugins branch 2 times, most recently from e067a30 to efd6d33 Compare March 25, 2021 23:56
paul-marechal
paul-marechal reviewed Mar 29, 2021
View reviewed changes
packages/plugin-ext/src/main/browser/tasks-main.ts Outdated
import { TaskWatcher } from '@theia/task/lib/common/task-watcher';
import { TaskService } from '@theia/task/lib/browser/task-service';
import { TaskDefinitionRegistry } from '@theia/task/lib/browser';

const revealKindMap = new Map<number | string, string | number>(
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to use number | RevealKind here and number | PanelKind below?

Maybe a better notation:

const revealKindMap: Map<number, RevealKind> & Map<RevealKind, number> = new Map([
    [1, RevealKind.Always],
    [2, RevealKind.Silent],
    [3, RevealKind.Never],
    [RevealKind.Always, 1],
    [RevealKind.Silent, 2],
    [RevealKind.Never, 3]
]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!, Thanks !

@alvsan09 alvsan09 force-pushed the task_presentation_plugins branch from efd6d33 to bb67b6b Compare March 29, 2021 19:38
@alvsan09
Support tasks presentation received from plugins
fe32632 
Support tasks presentation options received from plugins

This expands the functionality introduced in pull request eclipse-theia#6814

The following presentation options are now available for use by
plugins:

presentationOptions.reveal
presentationOptions.focus
presentationOptions.echo
presentationOptions.showReuseMessage
presentationOptions.panel
presentationOptions.clear

Signed-off-by: Alvaro Sanchez-Leon <alvaro.sanchez-leon@ericsson.com>
@alvsan09 alvsan09 force-pushed the task_presentation_plugins branch from bb67b6b to fe32632 Compare April 1, 2021 12:27
@kenneth-marut-work
Copy link
Contributor

Tested with the example plugin and I am seeing the proper translation for PanelKind and RevealKind. I am also seeing the expected behavior of the presentation options

paul-marechal
paul-marechal approved these changes Apr 1, 2021
View reviewed changes
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM! I tested using your plugin and debugging, the data is correctly converted around.

@paul-marechal paul-marechal merged commit bebd599 into eclipse-theia:master Apr 1, 2021
@vince-fugnitto vince-fugnitto added this to the 1.13.0 milestone Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomanNikitenko RomanNikitenko RomanNikitenko left review comments

@paul-marechal paul-marechal paul-marechal approved these changes

@kenneth-marut-work kenneth-marut-work Awaiting requested review from kenneth-marut-work

Assignees
No one assigned
Labels
tasks issues related to the task system
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

Error using vscode Tasks presentationOptions reveal
5 participants
@alvsan09 @kenneth-marut-work @RomanNikitenko @paul-marechal @vince-fugnitto