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

Implementation of SelectionRange and SelectionRangeProvider API #7534

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Apr 8, 2020

What it does

This changes proposal adds missing API for SelectionRange and SelectionRangeProvider.

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

TODO

How to test

See SelectionRangeProviderSample for full details.

Steps to test

  • build sample with yarn command
  • you'll get eclipse_che_selection_range_provider_sample.theia
  • build Theia from current PR
  • move eclipse_che_selection_range_provider_sample.theia to Theia's plugins folder
  • run Theia
  • create simple text file, e.g. test.txt
  • insert any text into opened editor, e.g. Lorem Ipsum
  • tag any piece of text with {SHOULD_BE_SELECTED}any text{/SHOULD_BE_SELECTED}

theia — Theia Browser Example 2020-04-17 12-37-52

  • locate cursor between above tags
  • call Expand Selection command, it will select word under cursor
  • call Expand Selection command again, it will select the whole piece of text arround tag SHOULD_BE_SELECTED

theia — Theia Browser Example 2020-04-17 12-44-01

Difference between master and this version:

  • in master, Theia will select the word on first execution of Expand Selection and then the whole paragraph
  • in this PR, Theia will select the word on first execution of Expand Selection, on second, piece of text surround by SHOULD_BE_SELECTED tag and then the whole paragraph[1]

[1] this works also for multi cursor

Review checklist

Reminder for reviewers

@paul-marechal
Copy link
Member

Just tried this PR with the vscode-clangd from LLVM which does not work with Theia's master, and it now seems to work! This is a bit of a shallow test, but definitely promising.

@marcdumais-work
Copy link
Contributor

I tired it as well - thanks @vzhukovskii!

FYI you can install vscode-clangd in the example application using the new extensions view

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@vzhukovskii Why draft? It looks good, no? :)

packages/plugin-ext/src/main/browser/languages-main.ts Outdated Show resolved Hide resolved
packages/plugin/src/theia.d.ts Show resolved Hide resolved
@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Apr 9, 2020
@akosyakov
Copy link
Member

@vzhukovskii What should one do with mentioned extension in How to test? Please clarify?

I see that there is no errors anymore from typescript anymore, but Expand/Shrink Selection does not work properly:

  • put a cursor in the body of ApplicationShell.onBeforeAttach and try to use expand selection multiple times
  • compare how it works in VS Code and in Theia, strangely enough it works well on master

Please make sure that you don't reinvent anything, copy an implementation from the VS Code extension host process code and file a CQ.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Apr 9, 2020

What should one do with mentioned extension in How to test? Please clarify?

I'm just checked that it doesn't fail during Theia start. Still looking for the way, how it can be fully checked.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Apr 9, 2020

compare how it works in VS Code and in Theia, strangely enough it works well on master

I'll check it

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Apr 9, 2020

Linked CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21891 🤞

// Update: CQ has been approved

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Apr 9, 2020

Linked CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=21891

// Update: CQ has been approved

Hi @vzhukovskii . I think the CQ is incorrect - it probably should be "Type_B", which is a deeper check, required to fork code. I added a comment directly on the CQ.

On step 3 in this section did you chose type of contribution: "Contribution of code to be maintained by an Eclipse Foundation project"? The IP people at the Foundation may be able to update the type for you and do the extra checks required.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Apr 9, 2020

On step 3 in this section did you chose type of contribution: "Contribution of code to be maintained by an Eclipse Foundation project"?

Yep, that's correct. I chose:
Project Content (Content to be maintained by an Eclipse Foundation Project)
can't understand, how it detects as Type A. Maybe it should be described in description.

@marcdumais-work
Copy link
Contributor

Yep, that's correct. I chose:
Project Content (Content to be maintained by an Eclipse Foundation Project)
can't understand, how it detects as Type A. Maybe it should be described in description.

From what I remember you may not even get to chose - the type may just map to the choice you make in the wizard, when filling-up the CQ. Let's see if someone from the IP team responds on the CQ.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Apr 9, 2020

Yep, that's correct. I chose:
Project Content (Content to be maintained by an Eclipse Foundation Project)
can't understand, how it detects as Type A. Maybe it should be described in description.

From what I remember you may not even get to chose - the type may just map to the choice you make in the wizard, when filling-up the CQ. Let's see if someone from the IP team responds on the CQ.

As I remember, there was an ability to choose the type (A or B), but at this moment I didn't see such options.

Waiting for the IP team response... 🙄

@marcdumais-work
Copy link
Contributor

As I remember, there was an ability to choose the type (A or B), but at this moment I didn't see such options.

Waiting for the IP team response...

Hi @vzhukovskii ,

I think it might be quicker to withdraw CQ 21891 and open a new one for "Type B" check.

@vzhukovs
Copy link
Contributor Author

I think it might be quicker to withdraw CQ 21891 and open a new one for "Type B" check.

I'm contacting now with the IP team to find out whether we can update type of CQ w/o creating the new one. I'm not 100% sure that new CQ will be automatically detected as Type B.

@akosyakov I've updated PR and Expand Selection and Shrink Selection behavior now is the same as VS Code has(updated type converter).

Now I'm working on how to test section.

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs
Copy link
Contributor Author

@akosyakov @marcdumais-work I've updated how to test section in PR description. Seems everything works fine. Still waiting for CQ analysis. IP team updated type from A to B.

@akosyakov
Copy link
Member

@vzhukovskii CQ has checkin label meaning it is good to merge. Please move this PR ready for review. I will test after the lunch.

@vzhukovs vzhukovs marked this pull request as ready for review April 17, 2020 09:58
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@vzhukovskii it works great, thank you! please merge

@vzhukovs vzhukovs merged commit ebf7b78 into master Apr 17, 2020
@vzhukovs vzhukovs deleted the theia#6623 branch April 17, 2020 11:29
@kittaakos
Copy link
Contributor

Please see remark: ebf7b78#r38560694

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants