CHE-5174 Improving search results #5874

vparfonov · 2017-08-02T09:45:45Z

What does this PR do?

Improving search results:

line numbers are shown for matches
multiple matches in the same file are shown individually
when you click on a search result, the editor loads the file AND jumps straight to the matching line with all instances of the search term already highlighted

What issues does this PR fix or reference?

related to eclipse-che/che-dependencies#50

Changelog

ProjectService#search() now return different objects, now it will be List<SearchResultDto> instead of List<ItemReference>

Release Notes

Improving search results:

line numbers are shown for matches
multiple matches in the same file are shown individually
when you click on a search result, the editor loads the file AND jumps straight to the matching line with all instances of the search term already highlighted

NOTE: ProjectService#search() now return different objects, now it will be List<SearchResultDto> instead of List<ItemReference>

RomanNikitenko · 2017-08-02T10:00:44Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/project/ProjectServiceClient.java

@@ -10,13 +10,15 @@
 *******************************************************************************/
 package org.eclipse.che.ide.api.project;

+import org.eclipse.che.api.project.shared.dto.SearchResultDto;


unused import

RomanNikitenko · 2017-08-02T10:14:29Z

...che-core-ide-api/src/main/java/org/eclipse/che/ide/api/project/ProjectServiceClientImpl.java

+                            for (SearchResultDto dto : arg) {
+                                results.add(new SearchResult(dto));
+                            }
+                            return results;


We can do it like return arg.stream().map(SearchResult::new).collect(toList());, but up to you - what is more readable...

Agree with @RomanNikitenko .. @vparfonov can you take a look?

public Promise<List<? extends SearchResult>> search(QueryExpression expression) {

RomanNikitenko · 2017-08-02T10:16:57Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/resources/Container.java

@@ -14,13 +14,16 @@
 import com.google.common.base.Optional;

 import org.eclipse.che.api.core.model.project.type.ProjectType;
+import org.eclipse.che.api.project.shared.dto.SearchResultDto;


unused import

RomanNikitenko · 2017-08-02T10:22:24Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/resources/SearchResult.java

+        path = searchResultDto.getItemReference().getPath();
+        project = searchResultDto.getItemReference().getProject();
+        final List<Link> links = searchResultDto.getItemReference().getLinks();
+        if (!links.isEmpty() && links.contains(Constants.LINK_REL_GET_CONTENT)) {


Are you sure the check links.contains(Constants.LINK_REL_GET_CONTENT) is correct?

RomanNikitenko · 2017-08-02T10:27:01Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/resources/impl/ContainerImpl.java

@@ -14,6 +14,7 @@
 import com.google.common.base.Optional;

 import org.eclipse.che.api.core.model.project.ProjectConfig;
+import org.eclipse.che.api.project.shared.dto.SearchResultDto;


unused import

RomanNikitenko · 2017-08-02T10:29:14Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/resources/impl/ResourceManager.java

@@ -22,6 +22,7 @@
 import org.eclipse.che.api.core.model.project.ProjectConfig;
 import org.eclipse.che.api.core.model.project.SourceStorage;
 import org.eclipse.che.api.core.rest.shared.dto.Link;
+import org.eclipse.che.api.project.shared.dto.SearchResultDto;


unused import

RomanNikitenko · 2017-08-02T10:30:15Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/resources/tree/FileNode.java

@@ -19,6 +19,7 @@
 import org.eclipse.che.ide.api.data.tree.settings.NodeSettings;
 import org.eclipse.che.ide.api.editor.EditorAgent;
 import org.eclipse.che.ide.api.resources.File;
+import org.eclipse.che.ide.api.resources.SearchResult;


unused import

RomanNikitenko · 2017-08-02T10:35:43Z

...-core-ide-app/src/main/java/org/eclipse/che/ide/search/presentation/FindResultGroupNode.java

+import org.eclipse.che.ide.api.editor.EditorAgent;
+import org.eclipse.che.ide.api.resources.SearchResult;
+import org.eclipse.che.ide.search.factory.FindResultNodeFactory;
+import org.eclipse.che.ide.ui.smartTree.TreeStyles;
 import org.eclipse.che.ide.ui.smartTree.compare.NameComparator;


please remove a few unused imports above

RomanNikitenko · 2017-08-02T10:39:10Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/search/presentation/FoundItemNode.java

+    protected Promise<List<Node>> getChildrenImpl() {
+        List<Node> fileNodes = new ArrayList<>();
+        List<SearchOccurrence> occurrences = searchResult.getOccurrences();
+        occurrences.sort(Comparator.comparingInt((SearchOccurrence searchOccurrence) -> searchOccurrence.getLineNumber()));


Can be simplified (SearchOccurrence searchOccurrence) -> searchOccurrence.getLineNumber()) -> SearchOccurrence::getLineNumber

RomanNikitenko · 2017-08-02T10:48:49Z

...-core-ide-app/src/main/java/org/eclipse/che/ide/search/presentation/FoundOccurrenceNode.java

+
+    /** {@inheritDoc} */
+    @Override
+    public NodePresentation getPresentation(boolean update) {


We will do updatePresentation(nodePresentation) twice when nodePresentation == null and update is true. Is it OK?

RomanNikitenko · 2017-08-02T11:01:47Z

...nt/che-core-api-project/src/main/java/org/eclipse/che/api/project/server/ProjectService.java


        final SearchResult result = searcher.search(expr);
        final List<SearchResultEntry> searchResultEntries = result.getResults();
-        final List<ItemReference> items = new ArrayList<>(searchResultEntries.size());
+        final List<SearchResultDto> s = new ArrayList<>(searchResultEntries.size());


May be makes sense something more readable as name of variable instead of s, WDYT?

codenvy-ci · 2017-08-02T11:52:37Z

Build # 3250 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3250/ to view the results.

svor · 2017-08-02T15:31:42Z

ide/che-core-ide-app/src/test/java/org/eclipse/che/ide/search/FullTextSearchPresenterTest.java

+//        verify(view).showSelectPathDialog();
+//        verify(view, never()).getSearchText();
+//    }
+//}


Maybe should to remove this code or rework

svor · 2017-08-02T15:33:25Z

...re-api-project-shared/src/main/java/org/eclipse/che/api/project/shared/SearchOccurrence.java

+     * @return
+     */
+    String getLineContent();
+}


need to fix javadoc

svor · 2017-08-02T15:33:54Z

...project-shared/src/main/java/org/eclipse/che/api/project/shared/dto/SearchOccurrenceDto.java

+     */
+    SearchOccurrenceDto withLineContent(String lineContent);
+
+}


need to fix javadoc

svor · 2017-08-02T15:35:12Z

wsagent/che-core-api-project/pom.xml

@@ -80,6 +80,11 @@
        </dependency>
        <dependency>
            <groupId>org.apache.lucene</groupId>
+            <artifactId>lucene-highlighter</artifactId>
+            <version>5.2.1</version>


I think need to move version to the parent pom

yes, will be fixed after eclipse-che/che-dependencies#50

svor · 2017-08-02T15:37:23Z

...nt/che-core-api-project/src/main/java/org/eclipse/che/api/project/server/ProjectService.java

+     * @param searchResultEntries
+     * @return
+     * @throws ServerException
+     */


fix java doc

svor · 2017-08-02T15:43:44Z

...t/che-core-api-project/src/main/java/org/eclipse/che/api/vfs/search/impl/LuceneSearcher.java

+
+                            if ((endOffset > txt.length()) || (startOffset > txt.length())) {
+                                throw new ServerException(
+                                        "Token " + termAtt.toString() + " exceeds length of provided text sized " + txt.length());


typo sized -> size

codenvy-ci · 2017-08-02T15:49:30Z

Build # 3256 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3256/ to view the results.

codenvy-ci · 2017-08-03T12:03:36Z

Build # 3266 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3266/ to view the results.

line numbers are shown for matches multiple matches in the same file are shown individually when you click on a search result, the editor loads the file AND jumps straight to the matching line with all instances of the search term already highlighted Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com>

skabashnyuk · 2017-08-03T12:34:22Z

...che-core-ide-api/src/main/java/org/eclipse/che/ide/api/project/ProjectServiceClientImpl.java

+                            for (SearchResultDto dto : arg) {
+                                results.add(new SearchResult(dto));
+                            }
+                            return results;


Agree with @RomanNikitenko .. @vparfonov can you take a look?

skabashnyuk · 2017-08-03T12:34:47Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/resources/SearchResult.java

+import java.util.List;
+
+/**
+ * @author Vitalii Parfonov


Actually class represents a POJO with a readable name.
javadoc is not necessary

skabashnyuk · 2017-08-03T12:37:37Z

...he-core-api-project/src/test/java/org/eclipse/che/api/vfs/search/impl/ReadFileUtilsTest.java

+        ReadFileUtils.getLine(tempFile.toFile(), 10000);
+
+    }
+


too many new lines.

codenvy-ci · 2017-08-03T13:07:36Z

Build # 3268 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3268/ to view the results.

tolusha · 2017-08-03T13:56:10Z

Do we need selenium tests to keep up to date?

vparfonov · 2017-08-03T14:09:05Z

@tolusha yes, @SkorikSergey already start work on it

Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com>

codenvy-ci · 2017-08-04T07:44:58Z

Build # 3277 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3277/ to view the results.

Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com>

This reverts commit 83150f4.

codenvy-ci · 2017-08-14T14:24:45Z

Build # 3367 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3367/ to view the results.

* CHE-5714:Improving search results: line numbers are shown for matches multiple matches in the same file are shown individually when you click on a search result, the editor loads the file AND jumps straight to the matching line with all instances of the search term already highlighted Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com> * Code cleanup Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com>

…e-che#5988) This reverts commit 83150f4.

vparfonov requested review from evidolob, svor, skabashnyuk, dkuleshov, azatsarynnyy and RomanNikitenko August 2, 2017 09:45

vparfonov requested a review from tolusha as a code owner August 2, 2017 09:45

RomanNikitenko reviewed Aug 2, 2017

View reviewed changes

vparfonov changed the title ~~[WIP] CHE-5174 Improving search results~~ CHE-5174 Improving search results Aug 2, 2017

svor reviewed Aug 2, 2017

View reviewed changes

vparfonov force-pushed the searchwithpossitions branch from 1e04c5e to 90e5866 Compare August 3, 2017 12:08

evidolob approved these changes Aug 3, 2017

View reviewed changes

svor approved these changes Aug 3, 2017

View reviewed changes

skabashnyuk approved these changes Aug 3, 2017

View reviewed changes

RomanNikitenko approved these changes Aug 3, 2017

View reviewed changes

tolusha approved these changes Aug 3, 2017

View reviewed changes

Code cleanup

700d470

Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com>

vparfonov added 2 commits August 14, 2017 15:11

Merge with master

5edb9f1

Signed-off-by: Vitalii Parfonov <vparfonov@redhat.com>

Merge branch 'master' into searchwithpossitions

de1cf92

vparfonov requested a review from slemeur August 14, 2017 12:12

vparfonov merged commit 83150f4 into master Aug 14, 2017

vparfonov deleted the searchwithpossitions branch August 14, 2017 13:09

vparfonov added a commit that referenced this pull request Aug 14, 2017

Revert "CHE-5174 Improving search results (#5874)"

8c896b8

This reverts commit 83150f4.

vparfonov mentioned this pull request Aug 14, 2017

Revert "CHE-5174 Improving search results " #5988

Merged

vparfonov added a commit that referenced this pull request Aug 14, 2017

Revert "CHE-5174 Improving search results (#5874)" (#5988)

3cf436f

This reverts commit 83150f4.

vparfonov restored the searchwithpossitions branch August 14, 2017 13:18

dmytro-ndp pushed a commit to dmytro-ndp/che that referenced this pull request Aug 16, 2017

Revert "CHE-5174 Improving search results (eclipse-che#5874)" (eclips…

a8c164d

…e-che#5988) This reverts commit 83150f4.

dmytro-ndp mentioned this pull request Sep 12, 2017

Rework FindTextFeatureTest acording to changes #6232

Closed

CHE-5174 Improving search results #5874

CHE-5174 Improving search results #5874

Conversation

vparfonov commented Aug 2, 2017 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Aug 2, 2017

codenvy-ci commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Aug 3, 2017

tolusha commented Aug 3, 2017

vparfonov commented Aug 3, 2017

codenvy-ci commented Aug 4, 2017

codenvy-ci commented Aug 14, 2017

vparfonov commented Aug 2, 2017 •

edited

Loading