Add ability search text in the terminal widget. #5471

AndrienkoAleksandr · 2019-06-14T12:16:43Z

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

vince-fugnitto · 2019-06-14T12:30:58Z

packages/terminal-search/src/browser/terminal-search-contribution.ts

+export class TerminalSearchFrontendContribution implements CommandContribution, KeybindingContribution {
+
+    constructor(
+        @inject(ApplicationShell) protected readonly shell: ApplicationShell,


Is there a specific reason for injecting through the constructor instead of as a property?

vince-fugnitto · 2019-06-14T12:35:22Z

Out of curiosity, why not add the functionality directly in the @theia/terminal package?
This means that apps will not need to explicitly depend on the @theia/terminal-search package just to get search features.

Also, do you believe we can generalise the widget so that it can be used across Theia (ex: output, problems) ?

AndrienkoAleksandr · 2019-06-19T06:10:03Z

Out of curiosity, why not add the functionality directly in the @theia/terminal package?

It's not a problem to move code to the @theia/terminal. If other reviewers like this idea, I will do it.

akosyakov · 2019-06-19T11:20:20Z

Please merge in the terminal extension.

akosyakov · 2019-06-19T11:29:46Z

@AndrienkoAleksandr looks good, but keyboard navigation does not work properly, try to use keyboard to navigate within the find widget in VS Code and Theia, in VS Code elements are sequential focused, in Theia nothing happens.

Another thing the search box does not have enough contrast in the dark theme. It is hard to see its border. Could you try to align styles with VS Code?

akosyakov · 2019-06-19T11:29:58Z

@AlexTugarev @svenefftinge could you have a look as well please

paul-marechal

LGTM, only detail I miss is being able to go to the next find by pressing enter in the text box.

paul-marechal · 2019-06-19T21:37:45Z

And like the previous commenters, it will be better in the terminal extension.

AndrienkoAleksandr · 2019-07-06T23:21:13Z

@akosyakov: Please merge in the terminal extension.

Done.

@marechal-p: only detail I miss is being able to go to the next find by pressing enter in the text box.

Done. Next search result -> Enter, previous search result -> Shift + Enter

@akosyakov: but keyboard navigation does not work properly

Good catch, I fixed bug with moving focus by TAB(move focus to next widget element) and Shift+TAB (move to previously focused element)

@akosyakov: Another thing the search box does not have enough contrast in the dark theme. It is hard to see its border. Could you try to align styles with VS Code?

I aligned selection color with VSCode.
Should I do the same for background and foreground terminal colors too(VSCode has separated colors for terminal in the Theme model)?

akosyakov · 2019-07-08T08:01:36Z

@AndrienkoAleksandr please review and align the PR with https://github.com/theia-ide/theia/pull/5616/files

I'm testing it again right now.

packages/core/src/browser/style/variables-bright.useable.css

packages/terminal/src/browser/search/terminal-search-keybinding-context.ts

akosyakov · 2019-07-08T08:13:41Z

packages/terminal/src/browser/search/terminal-search-keybinding-context.ts

+    protected terminalSearchWidgetFactory: TerminalSearchWidgetFactory;
+
+    isEnabled(): boolean {
+        if (!(this.shell.activeWidget instanceof TerminalWidgetImpl)) {


use TerminalWidget, don't program against implementation, review all new code

packages/terminal/src/browser/search/terminal-search-widget.tsx

packages/terminal/src/browser/search/terminal-search.css

packages/terminal/src/browser/search/terminal-search-widget.tsx

packages/terminal/src/browser/terminal-frontend-module.ts

packages/terminal/src/browser/terminal-frontend-contribution.ts

akosyakov · 2019-07-08T09:04:26Z

Should I do the same for background and foreground terminal colors too(VSCode has separated colors for terminal in the Theme model)?

having specific colors sounds good, easy to customize for products without messing with generic styles

Keyboard navigation works well, but match items don't have outline when selected (or maybe where are not selected when i focus and press Enter)?
Theia:

VS Code:

Aligning arrows would be nice that it matches to the search box in the editor.

akosyakov

It looks and behaves well. The code looks much simpler now.

Please address comments, clean up git history, it should not contain merge commits and clean up commits for changes introduced by this PR: https://github.com/theia-ide/theia/blob/master/doc/pull-requests.md#checklist-commit-history

After that you can merge it. 🚀

packages/terminal/src/browser/style/terminal-search.css

packages/terminal/src/browser/base/terminal-search-box.ts

packages/terminal/src/browser/terminal-keybinding-contexts.ts

packages/terminal/src/browser/search/terminal-search-widget.tsx

packages/terminal/src/browser/terminal-frontend-contribution.ts

akosyakov · 2019-09-12T05:27:15Z

@AndrienkoAleksandr do you need help with cleaning up outstanding commits and squashing git history before merging?

AndrienkoAleksandr · 2019-12-06T15:33:21Z

@akosyakov thanks for great review. Sorry for long delay.

akosyakov · 2019-12-11T11:26:27Z

We need to apply color theming to the find widget similar as it is done in VS Code: https://github.com/microsoft/vscode/blob/0dfa355b3ad185a6289ba28a99c141ab9e72d2be/src/vs/workbench/contrib/codeEditor/browser/find/simpleFindWidget.ts#L28

It has to be done after theming is landed. We are targeting next release for theming, so this PR will be on hold till then and has to be reworked to apply styles after.

akosyakov · 2019-12-11T11:35:23Z

We also should see whether we can extract a reusable simple find widget, i.e. to use it in webviews. But it can be done as another step.

AndrienkoAleksandr · 2020-02-10T09:10:01Z

I resolved merge conflict and fixed styles according to theme changes, also applied arrow icons, but I lost push access...

paul-marechal · 2020-02-10T15:37:07Z

@AndrienkoAleksandr while this is an annoying situation, you can always reopen the PR from a fork, moving your branch there. If you do not want to reopen a new PR form, I can push onto the the main repo on your behalf if you push your changes to your fork.

AlexTugarev · 2020-02-10T15:39:02Z

I resolved merge conflict and fixed styles according to theme changes, also applied arrow icons, but I lost push access...

:-( sad news.

akosyakov · 2020-02-11T10:50:04Z

@AndrienkoAleksandr are you commiter? if not, let's nominate you, could you ask some of your teammates?

AndrienkoAleksandr · 2020-02-11T11:03:36Z

@AndrienkoAleksandr are you commiter? if not, let's nominate you, could you ask some of your teammates?

In progress https://projects.eclipse.org/projects/ecd.theia/elections/election-alexander-andrienko-committer-eclipse-theia

AndrienkoAleksandr · 2020-02-18T13:43:59Z

Hello. Current screenshots:

AndrienkoAleksandr · 2020-02-18T20:21:20Z

Is it ok to merge it?

vince-fugnitto · 2020-02-19T13:27:35Z

packages/core/src/browser/icons/arrow-down-bright.svg

+      Red Hat, Inc. - initial API and implementation
+
+-->
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


@AndrienkoAleksandr where were these icons taken from?
I don't think we can only use EPL-2.0, in general we use:

SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0

I drawn them in the Inkscape for this issue, removed not needed tags with some meta information not related to the rendering. If it is not OK, I can try to find similar svg icons in the vscode repo.

I think there are errors in the svg file, nothing shows up:

I guess licence header break it.

I will take a look.

@AndrienkoAleksandr we can use the icons present from the vscode-icons repository at the given commit: microsoft/vscode-icons@9c90ce8 (the repo is already covered by CQ for this given commit).

There are icons for arrows (the same used by the monaco editor for it's find):

Ex: arrow-down

Ok, thanks, I will do it.

akosyakov

I think it is good, as usual get rid of commits which fix issues in original commits and good to merge.

I've noticed that cursor should be a pointer when you hover over actions in the search box.

packages/terminal/src/browser/terminal-keybinding-contexts.ts

akosyakov · 2020-02-21T09:09:05Z

BTW monaco already has internally reusable search widget 🙈 we probably should have exposed and used it instead of implementing kind of the same widget. I would be fine to merge this PR anyway and open the issue to reuse Monaco find widget instead later.

packages/terminal/src/browser/style/terminal-search.css

akosyakov · 2020-02-24T10:18:54Z

@AndrienkoAleksandr I think it looks super good now, except #5471 (comment) Feel free to fix it and merge.

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>

AndrienkoAleksandr requested review from svenefftinge, akurinnoy and evidolob June 14, 2019 12:16

vince-fugnitto reviewed Jun 14, 2019

View reviewed changes

AndrienkoAleksandr requested a review from akosyakov June 19, 2019 06:10

akosyakov requested a review from AlexTugarev June 19, 2019 11:19

paul-marechal reviewed Jun 19, 2019

View reviewed changes

akosyakov added the terminal issues related to the terminal label Jul 8, 2019