7589-file-search-preview #8735

danarad05 · 2020-11-09T14:38:56Z

Signed-off-by: Dan Arad dan.arad@sap.com

What it does

Fixes #7589

How to test

search text on left search pane.
click on line found -> file opened in preview mode
click on different file line > file opened in preview mode instead of first one
If you do replace - file is opened in editor mode and not preview mode.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

vince-fugnitto

@danarad05 unfortunately, @theia/search-in-workspace should not be coupled with @theia/editor-preview. With these changes, a new coupling is created (without being listed as a dependency), and downstream applications now unnecessarily pull @theia/editor-preview into their application.

vince-fugnitto

I see the pull-request was updated, but @theia/search-in-workspace should not pull @theia/editor-preview, we do not want to introduce coupling.

danarad05 · 2020-11-10T07:52:39Z

I see the pull-request was updated, but @theia/search-in-workspace should not pull @theia/editor-preview, we do not want to introduce coupling

@vince-fugnitto
Hi Vince,

Thanks for input.
May I ask why not pull editor-preview? if we do not add editor-preview will need to more or less copy this code to "@theia/editor" from editor-preview. So I am wondering why duplicate that code, why have editor contain "editor-preview" functionality? isn't that editor-preview's purpose?

vince-fugnitto · 2020-11-10T13:38:19Z

May I ask why not pull editor-preview?

Sure, from the beginning we wanted the framework to be modular in a way that application developers can control which extensions are included in their overall applications, and the possibility to not include certain extensions (for example @theia/editor-preview). With these changes, any application that previously included @theia/search-in-workspace will now include @theia/editor-preview and extra measures would need to be taken to remove that unwanted functionality. If we start coupling extensions together, in the end the idea of having the control of what their apps are composed of will no longer be the case.

The same is true from the @theia/scm view, we would ultimately want to preview files opened from that tree, but we should not make scm depend on editor-preview.

If we do not add editor-preview will need to more or less copy this code to "@theia/editor" from editor-preview. So I am wondering why duplicate that code, why have editor contain "editor-preview" functionality? isn't that editor-preview's purpose?

The goal is not to copy the code/functionality into another package, but implement something similar to the #7589 (comment). The idea would be if an application includes @theia/editor-preview then previewing files from siw would be supported, but if the application does not then it would work as today. The goal would not be to explicitly depend and pull new dependencies, but rather contribute functionality which can be used by external packages through options (such as the preview flag).

paul-marechal · 2020-11-10T22:57:41Z

@vince-fugnitto are you saying that @danarad05 should set the EditorOpenerOptions.preview option here instead?

vince-fugnitto · 2020-11-10T23:18:09Z

@vince-fugnitto are you saying that @danarad05 should set the EditorOpenerOptions.preview option here instead?

That is the ultimate goal yes (as expressed in the original issue), to use the contributed functionality to preview editors rather than introduce a new hard dependency and coupling between extensions. We can likely inspire ourselves from the explorer which is able to preview editors, and do the same for siw (this pr), and scm in the future.

danarad05 · 2020-11-11T10:43:20Z

@vince-fugnitto @marechal-p
Thanks Vince & Paul,
I thought from issue 7589 that what is required is to add preview functionality like in editor-preview. I see now that in editor - preview functionality means that tab isn't in focus.
Anyway, please review again,
Thanks

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.tsx

danarad05 · 2020-11-16T07:48:22Z

@vince-fugnitto Please review, thanks.

vince-fugnitto

There are a few issues with the behavior when performing a search & replace.
The behaviour is buggy, and the editor title is not correctly rendered:

The following is when performing a double click:

The following is when performing a single click:

It would be good to confirm the issues on your side as well.

The initial "request changes" due to coupling has been resolved.

danarad05 · 2020-11-19T16:44:41Z

There are a few issues with the behavior when performing a search & replace.
The behaviour is buggy, and the editor title is not correctly rendered:

@vince-fugnitto

Fixed title not displaying
The buggy behavior I found (and I think you are referring to it) is single-click and double-click mixing up - as both event handlers exist on same node - so added delay to single-click event.

packages\core\src\browser\tree\tree-widget.tsx:

            onClick: event => this.handleClickEvent(node, event),
            onDoubleClick: event => this.handleDblClickEvent(node, event),

Please review again.
Thanks

danarad05 · 2020-12-16T13:19:06Z

@vince-fugnitto awaiting your review. Thanks

vince-fugnitto · 2020-12-16T17:11:10Z

2. The buggy behavior I found (and I think you are referring to it) is single-click and double-click mixing up - as both event handlers exist on same node - so added delay to single-click event.

I believe it is related to #8168 (comment), and in the comment it is expressed to not use timeouts. I'm not sure what the proper solution is at this time (and I believe we should not introduce timeouts). Not fixing the issue leads to the pull-request's feature being unfortunately unusable.

danarad05 · 2020-12-17T09:01:50Z

@vince-fugnitto
I understand your hesitance and respect it but will give you my point of view as I see it:

First, in regard to @akosyakov remark:
We should reconsider such handlers. We should be very careful with solving it though, since using timeouts make an event look as not triggered by a user and cause failures for calling DOM apis, in the case if a dbl click event never happens.

a) Anton doesn't say - IMO - that we shouldn't do it all but that we should be carefull implementing timeout solutions and that timeouts could mislead users.
b) In continuation of (a) - not doing anything - not changing code leaves current situation as the situation that is actually described by Anton as problematic. Without doing anything - "buggy" behavior remains and IMO doesn't serve the greater good and in fact it essentially is a decision to leave it in worse solution. This "buggy" behavior of onclick/ondoubleclick problem exists for all nodes and could arise (or does arise already) in additional situations/locations.

Second, if we keep onclick and ondoubleclick on same node - there is no other solution as I see it than to use a timer. My solution here is to add a timer which will check for a set period of time - 250ms in this case - to wait if an additional click has been done - if not proceed with onclick handler. IMO this solution is less of an "impact" than a setTimeout that is added in order to manipulate rendering like for here instance:

protected scrollToEditorElement(nodeID: string): void {
        if (nodeID) {
            const el = document.getElementById(`${nodeID}-editor`);
            if (el) {
                // Timeout to allow render cycle to finish.
                **setTimeout(() => el.scrollIntoView());**
            }
        }
    }

Third, another option is to place ondoubleclick on child element of node and catch and preventdefault on it. I could try that but I would have to say that IMO that is a "riskier" change because it could create issues and would need to be tested well. Of course it could work well but it is a little riskier (IMO) because it changes functionality that is already tested for some time.

Thanks,
-Dan

vince-fugnitto · 2020-12-17T13:28:59Z

@akosyakov @kittaakos would you mind providing feedback for the pull-request as well?

My main concerns were #8168 (comment) and how the changes (which impact core and all trees (framework and downstream)) will be affected. I'm not sure if I have the full knowledge of the potential impacts.

danarad05 · 2021-03-29T08:43:42Z

@vince-fugnitto bump

vince-fugnitto · 2021-03-29T12:30:48Z

@vince-fugnitto bump

@danarad05 I've already expressed my concerns #8735 (comment). It would be good to rebase the pull-request and have others review as well.

replace result in editor mode. Signed-off-by: Dan Arad <dan.arad@sap.com>

danarad05 · 2021-05-26T13:07:01Z

@vince-fugnitto bump

@danarad05 I've already expressed my concerns #8735 (comment). It would be good to rebase the pull-request and have others review as well.

Rebased. Thanks

colin-grant-work

I have a few comments on the functionality here. For the most part it's working pretty well. I still see some tabbar flashing when I do a double-click open in Electron, but I don't think that's the fault of the code here.

I don't find the delay on a single-click event disturbing, but I don't think that code belongs in this PR. There are other PR's aiming to solve the same problem regarding opening files (#9518, #8913) and I think if we want to have a more general solution, we could write a generalizable click-event adapter that takes two listeners and decides which to invoke depending on whether it receives a single or double click, but that wouldn't belong in the TreeWidget code either - it would be a generally available utility.

If you take the click event code out, then there isn't much to object to here, and I think it gets in without a problem.

colin-grant-work · 2021-05-26T23:26:33Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.tsx

+        const opener = await this.openerService.getOpener(fileUri, opts);
+        const editorWidget = await opener.open(fileUri, opts) as EditorWidget;


These can be replaced by a single call to the open() function available from '@theia/core/lib/browser'

colin-grant-work · 2021-05-26T23:31:22Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.tsx

@@ -985,10 +993,15 @@ export class SearchInWorkspaceResultTreeWidget extends TreeWidget {
            mode: 'reveal'


If we want to mimic the behavior of the Navigator's opening, this should be mode: preview ? 'reveal' : 'activate'. Alternatively, mode can be omitted: the EditorManager will activate, the EditorPreviewManager will reveal.

colin-grant-work · 2021-05-26T23:39:11Z

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.tsx

@@ -971,7 +979,7 @@ export class SearchInWorkspaceResultTreeWidget extends TreeWidget {
            fileUri = new URI(node.fileUri);
        }

-        const opts: EditorOpenerOptions | undefined = !DiffUris.isDiffUri(fileUri) ? {
+        let opts: EditorOpenerOptions | undefined = !DiffUris.isDiffUri(fileUri) ? {


The conditions for deciding whether to show a diff widget (l. 974 - GitHub won't let me comment on old code) produce some weird behavior. Single clicking will open a diff widget, but double-clicking will open a normal editor. That isn't consistent with VSCode, but might be desirable. VSCode has read-write diff widgets where you can edit the right side, but we don't (seem to), so if you want to edit the file, you need a way to open it in a normal editor. Still, if the focus of the PR is fixing the preview vs. non-preview behavior, it might be worth seeing whether we can closer to VSCode's behavior.

vince-fugnitto previously requested changes Nov 9, 2020

View reviewed changes

vince-fugnitto added the search in workspace issues related to the search-in-workspace label Nov 9, 2020

vince-fugnitto reviewed Nov 9, 2020

View reviewed changes

danarad05 force-pushed the 7589-file-search-preview branch 2 times, most recently from 6defaff to f2a523c Compare November 11, 2020 10:42

danarad05 requested a review from vince-fugnitto November 11, 2020 10:43

vince-fugnitto reviewed Nov 11, 2020

View reviewed changes

packages/search-in-workspace/src/browser/search-in-workspace-result-tree-widget.tsx Outdated Show resolved Hide resolved

danarad05 requested a review from vince-fugnitto November 12, 2020 10:29

vince-fugnitto reviewed Nov 16, 2020

View reviewed changes

danarad05 requested a review from vince-fugnitto November 19, 2020 16:44

danarad05 force-pushed the 7589-file-search-preview branch from f361a2f to d67fb58 Compare December 2, 2020 10:34

vince-fugnitto requested review from colin-grant-work and kittaakos March 29, 2021 12:30

7589-file-search-preview: open search result file in preview mode,

39d4e2d

replace result in editor mode. Signed-off-by: Dan Arad <dan.arad@sap.com>

danarad05 force-pushed the 7589-file-search-preview branch from d67fb58 to 39d4e2d Compare May 26, 2021 13:05

colin-grant-work reviewed May 26, 2021

View reviewed changes

This was referenced Jun 2, 2021

Don't try to run task many times at same time by clicking on tree element #9546

Closed

Add utility to handle profusion of clicks #9567

Closed

kittaakos removed their request for review July 18, 2021 12:22

Object-d approved these changes Jul 27, 2021

View reviewed changes

RomanNikitenko mentioned this pull request Aug 2, 2021

TaskService.runTask(...) not safe for parallel Execution #9806

Closed

msujew mentioned this pull request Sep 6, 2021

fix preview of search result #10054

Closed

1 task

JonasHelming assigned colin-grant-work Mar 3, 2022

JonasHelming added the decision_needed issues that require decisions on ways forward (ex: if they should be updated or closed) label Mar 3, 2022

colin-grant-work mentioned this pull request Mar 3, 2022

Open SIW results as previews #10839

Merged

1 task

colin-grant-work closed this in #10839 Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7589-file-search-preview #8735

7589-file-search-preview #8735

danarad05 commented Nov 9, 2020 •

edited

Loading

vince-fugnitto left a comment

vince-fugnitto left a comment

danarad05 commented Nov 10, 2020 •

edited

Loading

vince-fugnitto commented Nov 10, 2020

paul-marechal commented Nov 10, 2020

vince-fugnitto commented Nov 10, 2020

danarad05 commented Nov 11, 2020

danarad05 commented Nov 16, 2020

vince-fugnitto left a comment

danarad05 commented Nov 19, 2020 •

edited

Loading

danarad05 commented Dec 16, 2020

vince-fugnitto commented Dec 16, 2020

danarad05 commented Dec 17, 2020

vince-fugnitto commented Dec 17, 2020

danarad05 commented Mar 29, 2021

vince-fugnitto commented Mar 29, 2021

danarad05 commented May 26, 2021

colin-grant-work left a comment •

edited

Loading

colin-grant-work May 26, 2021

colin-grant-work May 26, 2021

colin-grant-work May 26, 2021

		const opener = await this.openerService.getOpener(fileUri, opts);
		const editorWidget = await opener.open(fileUri, opts) as EditorWidget;

		@@ -985,10 +993,15 @@ export class SearchInWorkspaceResultTreeWidget extends TreeWidget {
		mode: 'reveal'

7589-file-search-preview #8735

7589-file-search-preview #8735

Conversation

danarad05 commented Nov 9, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

danarad05 commented Nov 10, 2020 • edited Loading

vince-fugnitto commented Nov 10, 2020

paul-marechal commented Nov 10, 2020

vince-fugnitto commented Nov 10, 2020

danarad05 commented Nov 11, 2020

danarad05 commented Nov 16, 2020

vince-fugnitto left a comment

Choose a reason for hiding this comment

danarad05 commented Nov 19, 2020 • edited Loading

danarad05 commented Dec 16, 2020

vince-fugnitto commented Dec 16, 2020

danarad05 commented Dec 17, 2020

vince-fugnitto commented Dec 17, 2020

danarad05 commented Mar 29, 2021

vince-fugnitto commented Mar 29, 2021

danarad05 commented May 26, 2021

colin-grant-work left a comment • edited Loading

Choose a reason for hiding this comment

colin-grant-work May 26, 2021

Choose a reason for hiding this comment

colin-grant-work May 26, 2021

Choose a reason for hiding this comment

colin-grant-work May 26, 2021

Choose a reason for hiding this comment

danarad05 commented Nov 9, 2020 •

edited

Loading

danarad05 commented Nov 10, 2020 •

edited

Loading

danarad05 commented Nov 19, 2020 •

edited

Loading

colin-grant-work left a comment •

edited

Loading