Bug 548518 - contribute the quick search (from Pivotal) to the platform
Summary: contribute the quick search (from Pivotal) to the platform
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.13   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.13 M3   Edit
Assignee: Kris De Volder CLA
QA Contact:
URL:
Whiteboard:
Keywords: Documentation, noteworthy, usability
: 434139 (view as bug list)
Depends on: 550326 549961 550058 550101 550261 550327
Blocks:
  Show dependency tree
 
Reported: 2019-06-21 11:30 EDT by Martin Lippert CLA
Modified: 2019-11-27 07:39 EST (History)
19 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Lippert CLA 2019-06-21 11:30:46 EDT
Several people contacted me and proposed to me that we (the Spring tools team here at Pivotal) should contribute our Quick Search feature to the Eclipse platform. And we are happy to do that now... :-)

Here is the existing feature on the Eclipse Marketplace (in case you would like to give it a try and explore it a bit):

https://marketplace.eclipse.org/content/quick-search-eclipse

We will go ahead and prepare a gerrit patch for the contribution, of that is fine with you all. If there is anything to discuss upfront, feel free to comment here.
Comment 1 Andrey Loskutov CLA 2019-06-21 11:57:27 EDT
Martin, not sure I understand for *what* is this search, and in which scope?
Comment 2 Martin Lippert CLA 2019-06-21 12:03:10 EDT
Here are a bit more details about the search:
https://spring.io/blog/2013/07/11/eclipse-quick-search

It is basically a dialog that shows you results as-you-type for your convenience. It searches text files within your workspace.

It is not something new, you can achieve basically the same "search" with the existing search capabilities in Eclipse, but the quick search is super handy and super fast, which makes it so useful... :-)
Comment 3 Mickael Istria CLA 2019-06-21 13:10:38 EDT
+1!
Comment 4 Lars Vogel CLA 2019-06-21 15:01:50 EDT
+1
Comment 5 Martin Lippert CLA 2019-06-24 09:08:28 EDT
I think we would contribute a single bundle containing the implementation. Here are my questions that I would like to discuss quickly upfront:

git repo:
- eclipse.platform.text

bundle:
- org.eclipse.search.quicksearch

packages:
- org.eclipse.search.quicksearch (.ui, .internal)

Any opinions or feedback on this? What do you think?

I would also propose a separate feature for this. Opinions?
Comment 6 Mickael Istria CLA 2019-06-24 09:12:04 EDT
How much code is it made of (number of classes mainly)?
Do you foresee this bundle providing APIs or is it all internal?

What would be your arguments against having it part of the org.eclipse.search bundle?
Comment 7 Martin Lippert CLA 2019-06-24 09:18:43 EDT
> How much code is it made of (number of classes mainly)?

I expect this to be 30-40 classes (rough estimate).

> Do you foresee this bundle providing APIs or is it all internal?

No API, all internal.

> What would be your arguments against having it part of the
> org.eclipse.search bundle?

No general objections against making it part of the search bundle. It doesn't really share code with the general search implementation and if people or distributions don't like it, they could opt-out by not installing the feature, but those are more thoughts than strong opinions. Whatever the platform team would prefer... :-)
Comment 8 Mickael Istria CLA 2019-06-24 09:24:10 EDT
(In reply to Martin Lippert from comment #7)
> It doesn't really share code with the general search implementation

Ok, I thought it was an alternative frontend for the same TextSearchEngine.

> and if people or distributions don't like it, they could opt-out by not installing the feature, but those are more thoughts than strong opinions. 

If this is not intrusive in UX (only *new* things that can be disabled basically by untriggering a shortcut) and all internal, it might be better and simpler to ship it in an existing bundle. Do you foresee that there will be some RCP products or users that could be annoyed if this is in by default?
Comment 9 Martin Lippert CLA 2019-06-24 09:49:19 EDT
> > It doesn't really share code with the general search implementation
> 
> Ok, I thought it was an alternative frontend for the same TextSearchEngine.
> 

No, it does NOT use the same search engine, it comes with its own way to search text files in a very optimized (and simplistic) way.

> > and if people or distributions don't like it, they could opt-out by not installing the feature, but those are more thoughts than strong opinions. 
> 
> If this is not intrusive in UX (only *new* things that can be disabled
> basically by untriggering a shortcut) and all internal, it might be better
> and simpler to ship it in an existing bundle. Do you foresee that there will
> be some RCP products or users that could be annoyed if this is in by default?

The shortcut for this could be disabled, in which case there would just nothing happen at all. There is no expensive index being built or stored, so there should be really nothing happen at all if this isn't triggered from the UI.

So would you still vote for including it into the search bundle even if it is a totally separate implementation?

I am not sure about the bundle dependencies right from the top of my head, so those might bring additional arguments for or against the inclusion on the table, I assume.
Comment 10 Mickael Istria CLA 2019-06-24 09:53:37 EDT
If it's not using the same search engine, then I agree with your initial proposal that a dedicated bundle is better.
If it's only text search, I suggest to name the bundle org.eclipse.text.quicksearch.

About having a separate feature, I don't think it's necessary; and we can discuss that later once the bundle is in and more easily available for testing.
Comment 11 Roland Grunberg CLA 2019-06-24 11:07:17 EDT
From a quick look over the sources, and a running code coverage on basic functionality :

- quicksearch bundle manifest shows it requires guava, but doesn't seem to use it, so possibly one less dependency
- there's a dependence to jgit's path matcher. I guess it might be nice if something in platform provided identical or similar functionality. 
- seems to depend on 'org.springsource.ide.eclipse.commons.livexp' bundle for MaxLineLengthSection, QuickSearchIgnoreSection, QuickSearchPreferencesPage but these don't seem to be triggered in the "general case" so I guess mainly for more detailed preference settings.

With the above, it seems closer to ~20 classes and 4k LOC. Of course, I'm not saying we should strip everything "extra" but it seems like the core functionality could be moved over very easily.
Comment 12 Kris De Volder CLA 2019-06-24 18:16:09 EDT
> quicksearch bundle manifest shows it requires guava, but doesn't seem to use it

If I remove it, then the code no longer compilese because guava types are indirectly uses via the livexp dependency.

However, I gather we want to get rid of that dependency prior to contributing the quicksearch plugin to eclipse.

> seems to depend on 'org.springsource.ide.eclipse.commons.livexp' bundle for MaxLineLengthSection, QuickSearchIgnoreSection, QuickSearchPreferencesPage but these don't seem to be triggered in the "general case" so I guess mainly for more detailed preference settings.

I've re-implemented the preferences page using Eclipse `org.eclipse.jface.preference.FieldEditorPreferencePage` and removed the dependency on livexp bundle.

Caveat, I made a modified copy of 'StringFieldEditor' because I needed something that is capable of presenting a multi-line text field. I think that's still preferable over pulling a bunch of code from the 'livexp' bundle.

Probably it would be possible to avoid this 'StringFieldEditor' copy if the code in StringFieldEditor could be generalized to allow for this directly. But I just wanted to get things working without first doing a PR on Eclipse core platform to fix `StringFieldEditor` in this way.

How strongly do people feel strongly about this copied code?
Comment 13 Mickael Istria CLA 2019-06-25 02:48:31 EDT
(In reply to Kris De Volder from comment #12)
> > quicksearch bundle manifest shows it requires guava, but doesn't seem to use it
> 
> If I remove it, then the code no longer compilese because guava types are
> indirectly uses via the livexp dependency.
> 
> However, I gather we want to get rid of that dependency prior to
> contributing the quicksearch plugin to eclipse.

If it's possible to remove the dependency to guava without introducing some much bigger maintenance cost, that would be preferred.
But we could make Platform using Guava if it's better.

> Caveat, I made a modified copy of 'StringFieldEditor' because I needed
> something that is capable of presenting a multi-line text field. I think
> that's still preferable over pulling a bunch of code from the 'livexp'
> bundle.

Ok, I agree.

> Probably it would be possible to avoid this 'StringFieldEditor' copy if the
> code in StringFieldEditor could be generalized to allow for this directly.
> But I just wanted to get things working without first doing a PR on Eclipse
> core platform to fix `StringFieldEditor` in this way.

Making pull request against JFace code isn't too hard. I suggest you do both in paralllel: submit change to StringFieldEditor and prepare the quicksearch commit, each one with its own Gerrit patch. As I guess the former is much simpler, there could be high chances it gets merged promptly so it could be used in quicksearch before we merge it.

> How strongly do people feel strongly about this copied code?

Not too bad at the moment, but duplication and copied code for workarounds are typical things we cannot really affort to maintain on the mid-term because 1. it's more expansive to maintain and 2. it places value in non-shared places, so it's usually a missed opportunity for more profitable improvements fixing directly the source of the copy.
Comment 14 Andrey Loskutov CLA 2019-06-25 02:51:52 EDT
(In reply to Mickael Istria from comment #13)
> If it's possible to remove the dependency to guava without introducing some
> much bigger maintenance cost, that would be preferred.
> But we could make Platform using Guava if it's better.

Ideally we will not use Guava in platform. The Guava API changes too fast in too incompatible way, ask XText people how they feel about it. As a downstream XText consumer I always have trouble upgrading XText without upgrading Guava or vice versa. I honestly don't wont to see this trouble in platform too.
Comment 15 Kris De Volder CLA 2019-06-25 11:39:45 EDT
I guess I didn't make this clear. I already removed the guava dependency when I removed the livexp dependency (since the only reason the guava dependency was needed was because of livexp).

Points about copied code taken. So I will look at changing StringFieldEditor to support a multi-line Text widget then comeback to quicksearch after that is done.
Comment 16 Kris De Volder CLA 2019-06-25 18:12:54 EDT
I've created a bug and submitted a patchset for the StingFieldEditor changes https://git.eclipse.org/r/#/c/144891/

Next step I think would be to take a copy of our STS quicksearch code in its current form into its eventual home, then massage it into something acceptable before merging / accepting that patch set.

I think the agreement is that:

a) it will go into `eclipse.platform.text` repo (https://github.com/eclipse/eclipse.platform.text)
b) as a new bundle called `org.eclipse.text.quicksearch`

Correct me if I'm wrong.

To avoid going back and-forth on this I'm holding of on doing this next step for now until the StringFieldEditor changes have been accepted.
Comment 17 Kris De Volder CLA 2019-07-02 14:10:41 EDT
I'm starting to work on the quicksearch contribution to eclipse.platform.text. My WIP is here:

https://github.com/eclipse/eclipse.platform.text/pull/22

I understand that this probably has to go through Gerrit eventually but I just rather not deal with Gerrit for now. 

Once I got something a bit more final I'll rebase and do the gerrit thing.
Comment 18 Eclipse Genie CLA 2019-07-02 16:16:19 EDT
New Gerrit change created: https://git.eclipse.org/r/145336
Comment 19 Eclipse Genie CLA 2019-07-02 16:27:40 EDT
New Gerrit change created: https://git.eclipse.org/r/145336
Comment 20 Kris De Volder CLA 2019-07-02 17:50:47 EDT
Not sure what the problem is, but the CI build bot doesn't like my changeset still.

Shows this error when building the 'quicksearch' bundle:

14:44:23 [INFO] --- tycho-eclipserun-plugin:1.4.0:eclipse-run (api-analysis) @ org.eclipse.text.quicksearch ---
14:44:23 [INFO] {osgi.os=linux, osgi.ws=gtk, org.eclipse.update.install.features=true, osgi.arch=x86_64}
14:44:23 [ERROR] Cannot resolve project dependencies:
14:44:23 [ERROR]   You requested to install 'org.eclipse.equinox.p2.iu; org.eclipse.text.quicksearch 0.0.0' but it could not be found
1

I'm not really sure what the problem is. Why is it trying resolve the bundle its currently building? And how do I fix this?
Comment 21 Alexander Fedorov CLA 2019-07-02 23:34:30 EDT
(In reply to Kris De Volder from comment #20)
> 
> I'm not really sure what the problem is. Why is it trying resolve the bundle
> its currently building? And how do I fix this?

I guess you need to add your new bundle to some feature.

BTW, WDYT about splitting the bundle for "core" and "ui" part?
Comment 22 Mickael Istria CLA 2019-07-03 02:06:40 EDT
(In reply to Alexander Fedorov from comment #21)
> BTW, WDYT about splitting the bundle for "core" and "ui" part?

As it's all internal and not expected to be reused anywhere at the moment, this separation would IMO be extra work without added value. The org.eclipse.search bundle doesn't have a core/ui separation and it doens't seem like an issue.
IMO, it's better to leave it as it and wait for actual use-cases requiring this feature to implement what's necessary (no more no less).
Comment 23 Alexander Fedorov CLA 2019-07-03 02:53:54 EDT
(In reply to Mickael Istria from comment #22)

> As it's all internal and not expected to be reused anywhere at the moment,
> this separation would IMO be extra work without added value. The
> org.eclipse.search bundle doesn't have a core/ui separation and it doens't
> seem like an issue.
> IMO, it's better to leave it as it and wait for actual use-cases requiring
> this feature to implement what's necessary (no more no less).

For me the separation is always good as it simplifies unit testing, opens more space for reuse and provides the proof of design to some extent. 

And the current org.eclipse.search doesn't look good from this perspective: it is too coupled with IResource and E3 UI that makes it hard to reuse for headless and E4 RCPs. So, it is "internal" because it is hard to reuse and people are forced to implement alternative search approaches, that we are discussing now :)

But you are right, this discussion should not prevent this cool contribution to go forward.
Comment 24 Mickael Istria CLA 2019-07-03 03:08:18 EDT
(In reply to Alexander Fedorov from comment #23)
> And the current org.eclipse.search doesn't look good from this perspective:
> it is too coupled with IResource and E3 UI that makes it hard to reuse for
> headless and E4 RCPs. So, it is "internal" because it is hard to reuse and
> people are forced to implement alternative search approaches, that we are
> discussing now :)

Is there any technical or other blocker that would prevent from improving the legacy Search bundle and APIs? Making it more reusable seems doable, it just requires people who need to reuse it to invest in it specifically to increase it reusability; it would be the same for QuickSearch.
Let me explicit that I don't think we should expect QuickSearch to be some form of replacement for regular search, especially from API POV, and if anyone sees issues with regular search, then it should be improved where it belongs rather than ignored and assuming QuickSearch is the place where development can happen.
Comment 25 Alexander Fedorov CLA 2019-07-03 03:25:25 EDT
(In reply to Mickael Istria from comment #24)
> 
> Is there any technical or other blocker that would prevent from improving
> the legacy Search bundle and APIs? Making it more reusable seems doable, it
> just requires people who need to reuse it to invest in it specifically to
> increase it reusability; it would be the same for QuickSearch.

It will not be an easy task but seems doable. Currently org.eclipse.search has circular dependencies between core and ui types. So, the plan may be to extract "pure stream scanning" types somewhere to org.eclipse.search.core and re-export it to not break everything. And then the more complex stage will be to resolve the dependencies between "text" and "search" packages/bundles.

> Let me explicit that I don't think we should expect QuickSearch to be some
> form of replacement for regular search, especially from API POV, and if
> anyone sees issues with regular search, then it should be improved where it
> belongs rather than ignored and assuming QuickSearch is the place where
> development can happen.

I also do not think the QuickSearch as a replacement for the regular search. For me the Quick search is one of the "application" for the "common search engine".
Comment 26 Martin Lippert CLA 2019-07-03 06:11:12 EDT
(In reply to Alexander Fedorov from comment #25)
> I also do not think the QuickSearch as a replacement for the regular search.
> For me the Quick search is one of the "application" for the "common search
> engine".

I agree, the quick search IS DEFINITELY NOT a replacement for the regular search. But, as mentioned above, it is also not strictly speaking an application for the common search engine, since it uses a different approach and implementation. Just thought I should mention that again to avoid surprises... :-)

I would also like to suggest to move the discussion around changes to the existing search bundle (separation of core/ui etc) to a different bug item.
Comment 27 Kris De Volder CLA 2019-07-03 11:48:59 EDT
> BTW, WDYT about splitting the bundle for "core" and "ui" part?

To be honest, I don't see the point. Why? 

If there is a good reason to do it, say... something that can't be implemented without this split up, then sure. But unless there is a pressing need to do it, I'd rather not spend effort on splitting the code up like that. The bundle is small and splitting it up into two even smaller bundles is just busy work to me that doesn't seem to bring any immediate benefit. In fact, it seems to have only immediate drawbacks to me:

- one more bundle cluttering up the workspace, the build etc.
- potential for breaking things in the refactoring

So I'd rather spend effort where it has a direct benefit that users can see.
Comment 28 Lars Vogel CLA 2019-07-03 12:05:13 EDT
+1 to Kris in https://bugs.eclipse.org/bugs/show_bug.cgi?id=548518#c27 

If everything is internal we can later split it still later, if we see a use case for it.

So +1 for merge without split.
Comment 29 Alexander Fedorov CLA 2019-07-03 12:18:19 EDT
(In reply to Lars Vogel from comment #28)
> 
> So +1 for merge without split.

+1 for merge without split, we discussed it enough in the thread above
Comment 30 Paul Pazderski CLA 2019-07-03 12:22:53 EDT
Should the internal be represented in the package names? Also is https://wiki.eclipse.org/Export-Package still up to date?
Comment 31 Mickael Istria CLA 2019-07-03 12:26:03 EDT
(In reply to Paul Pazderski from comment #30)
> Should the internal be represented in the package names? Also is
> https://wiki.eclipse.org/Export-Package still up to date?

Yes, .internal in package names, all packages exported and marked with x-internal:=true on MANIFEST.MF
Comment 32 Paul Pazderski CLA 2019-07-03 12:44:32 EDT
Another point is the key binding. STS users know quick search with Ctrl+Shift+L but it overlaps with org.eclipse.ui.window.showKeyAssist command. In case we change the show key assist binding some help pages need an update. https://help.eclipse.org/2019-06/advanced/searchView.jsp?searchWord=Ctrl+Shift+L
Comment 33 Kris De Volder CLA 2019-07-03 13:19:49 EDT
> Another point is the key binding. STS users know quick search with Ctrl+Shift+L but it overlaps with org.eclipse.ui.window.showKeyAssist command

It probably makes more sense to change quicksearch keybinding rather than the other way around. The trouble is... finding a keybinding that is not yet taken and doesn't require pressing more than 3 keys (arguably 3 keys is already a lot :-). Trouble is all the good/short ones are pretty much already taken.
Comment 34 Alexander Kurtakov CLA 2019-07-03 13:22:19 EDT
Changing org.eclipse.ui.window.showKeyAssist is very bad idea as there are many many places over the internet referring to Ctrl+Shift+L showing key bindings dialog.
Comment 35 Mickael Istria CLA 2019-07-03 16:09:17 EDT
(In reply to Kris De Volder from comment #33)
> > Another point is the key binding. STS users know quick search with Ctrl+Shift+L but it overlaps with org.eclipse.ui.window.showKeyAssist command
> 
> It probably makes more sense to change quicksearch keybinding rather than
> the other way around. The trouble is... finding a keybinding that is not yet
> taken and doesn't require pressing more than 3 keys (arguably 3 keys is
> already a lot :-). Trouble is all the good/short ones are pretty much
> already taken.

For this iteration, unless we find an obvious good shortcut, we can just remove it and track it in another ticket after this is merged.
Comment 36 Kris De Volder CLA 2019-07-03 19:10:56 EDT
I figured that a longer shortcut is still better than no shortcut. I changed the shortcut to CTRL+SHIFT+ALT+L. All combination of CTRL+SHIFT+<letter> seem to be in use.
Comment 37 Kris De Volder CLA 2019-07-03 19:17:31 EDT
(In reply to Alexander Fedorov from comment #21)
> (In reply to Kris De Volder from comment #20)
> > 
> > I'm not really sure what the problem is. Why is it trying resolve the bundle
> > its currently building? And how do I fix this?
> 
> I guess you need to add your new bundle to some feature.
> 

Doesn't seem to be any feature.xml inside of the eclipse.platform.text feature. So where should I add it exactly?
Comment 38 Kris De Volder CLA 2019-07-03 19:33:14 EDT
(In reply to Mickael Istria from comment #31)
> (In reply to Paul Pazderski from comment #30)
> > Should the internal be represented in the package names? Also is
> > https://wiki.eclipse.org/Export-Package still up to date?
> 
> Yes, .internal in package names, all packages exported and marked with
> x-internal:=true on MANIFEST.MF

Latest changeset adopts this convention.
Comment 39 Lars Vogel CLA 2019-07-04 01:23:54 EDT
(In reply to Kris De Volder from comment #36)
> I figured that a longer shortcut is still better than no shortcut. I changed
> the shortcut to CTRL+SHIFT+ALT+L. All combination of CTRL+SHIFT+<letter>
> seem to be in use.

Does this work on Windows? IIRC Markus Keller once explained that windows has special usage of CTRL+SHIFT+ALT. Can't find the reference though.
Comment 41 Alexander Fedorov CLA 2019-07-04 15:41:17 EDT
Thanks for your effort!
For the jgit let's see how it will work with build. TBH I don't know for sure the current situation with jgit availability in the platform, may be it is a part of platform feature already. In the worst case I think you can create an internal copy of required function to be resolved later.

Please try to disable API analysis as Mickael suggested

> put a <skipAPIAnalysis>true</skipAPIAnalysis> in the pom.xml for this bundle

An example is here https://git.eclipse.org/c/platform/eclipse.platform.ui.tools.git/tree/bundles/org.eclipse.e4.tools.emf.ui/pom.xml?id=e7079464f0cade1a4e8fb2b62cfd6af43d67fcd2
Comment 42 Kris De Volder CLA 2019-07-04 18:19:53 EDT
(In reply to Kris De Volder from comment #40)
> Except for the jgit issue, I think I have either:
> 
> - adressed all the comments or...
> - created a follow up bugzilla ticket as requested.
> 
> Here are the new tickets:
> 
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=548986
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=548987
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=548988
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=548989
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=548990
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=548991

Oops! One follow up ticket fell through the cracks. Created it now:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=548993
Comment 43 Mickael Istria CLA 2019-07-05 03:20:35 EDT
As it's a major contribution (~5000 lines) coming  from a 3rd party project, we need a CQ to verify and track some IP. I first need an answer to the following question:
"""
please provide contact details for each of the contributors: name, organization, email address and/or phone number, percent of contribution authored.
"""
Please also mention who is the copyright owner for those contributions. If it appears all are Pivotal employee and Pivotal owns the copyright for all the work, then it make it easier to get the patch merged because Pivotal is a member company.
Comment 44 Kris De Volder CLA 2019-07-08 13:31:38 EDT
>please provide contact details for each of the contributors: name, organization, email address and/or phone number, percent of contribution authored.

The percentage numbers are just a stab in the dark, but most of the code is / was written by me, so let's say 90% is me:

Kris De Volder <kdevolder@pivotal.io>

And the remaining 10% is attributable to my coleagues

Martin Lippert <mlippert@pivotal.io>
Alex Boyko <aboyko@pivotal.io>
Nieraj Singh <nsingh@pivotal.io>

All of them are Pivotal employee's and Pivotal owns the copyright of all the work.

For completeness sake, there is also a small portion of the code that was not authored solely by us but was copied from Eclipse platform itself. Whenever this kind of copying took place the original copyright headers would have been retained.

I assume this copying is not a problem as the copied code already belonged to Eclipse and was available under EPL for us to copy and re-use.
Comment 45 Kris De Volder CLA 2019-07-08 17:34:02 EDT
Last sticky point (jgit dependency) should now be resolved in latest patch set. So... good to merge now? Or is there anything else you'd want me to do first?
Comment 46 Alexander Fedorov CLA 2019-07-09 13:28:40 EDT
(In reply to Kris De Volder from comment #45)
> Last sticky point (jgit dependency) should now be resolved in latest patch
> set. So... good to merge now? Or is there anything else you'd want me to do
> first?

This week is pre-M1, no big merge is allowed.
Comment 47 Mickael Istria CLA 2019-07-23 15:57:16 EDT
I've been away for some time.
What's the status here? What would be preventing a merge?
Comment 48 Thomas Wolf CLA 2019-07-23 18:31:15 EDT
(In reply to Mickael Istria from comment #47)
> I've been away for some time.
> What's the status here? What would be preventing a merge?

As mentioned on Gerrit: double-check the version ranges in the manifests. Also: does the test bundle also need an Automatic-Module-Name?

Dependency-wise it should be OK now; as Kris said, the jgit dependency is gone (replaced by ant for the path matching).

Otherwise: try it and check no obvious problems crop up.
Comment 49 Kris De Volder CLA 2019-07-25 16:29:24 EDT
> does the test bundle also need an Automatic-Module-Name

Added it in latest changeset.
Comment 51 Mickael Istria CLA 2019-07-29 03:41:55 EDT
Main patch is merged.
We need to wait for next I-Build to add it to the right feature.
Comment 52 Eclipse Genie CLA 2019-07-30 11:12:02 EDT
New Gerrit change created: https://git.eclipse.org/r/146800
Comment 54 Mickael Istria CLA 2019-07-31 11:25:18 EDT
So the bundle and its payload are available in latest SDK build ( https://download.eclipse.org/eclipse/downloads/ ), that's a great addition!

Some comments:
* We should replace the "Go!" label by a more sober "Open" (with a mnemonic key)
* The Go/Open button should be made default (so it appears as such on Linux at least). @Eric: can you describe how this can be achieved
* I'm not fully sure I understand the meaning of the "Refresh" button, so I would like to find a better term. Also, this button seems kind of secondary in the workflow (compared to open). Maybe it could be a smaller button on the same line as the search bar. To save space, it could just show the "Refresh" icon that's used in some other places of the IDE.

What do you think?
Comment 55 Eric Williams CLA 2019-07-31 11:31:52 EDT
(In reply to Mickael Istria from comment #54)
> * The Go/Open button should be made default (so it appears as such on Linux
> at least). @Eric: can you describe how this can be achieved

Sure -- it's in SWT's Decorations.setDefaultButton(). So on the Shell of the search window, call setDefaultButton() and provide the Go/Open button as the parameter.
Comment 56 Andrey Loskutov CLA 2019-08-01 01:04:35 EDT
Please check releng test fails:
https://download.eclipse.org/eclipse/downloads/drops4/I20190731-1800/testresults/html/org.eclipse.releng.tests_ep413I-unit-mac64-java8_macosx.cocoa.x86_64_8.0.html

Plugin directory missing required files: /Users/genie.releng/jenkins_agent/workspace/ep413I-unit-mac64-java8/workarea/I20190731-1800/eclipse-testing/test-eclipse/Eclipse.app/Contents/Eclipse/plugins/org.eclipse.text.quicksearch.source_1.0.0.v20190729-0718.jar;
Comment 57 Eclipse Genie CLA 2019-08-01 04:51:25 EDT
New Gerrit change created: https://git.eclipse.org/r/146901
Comment 59 Eclipse Genie CLA 2019-08-05 02:50:21 EDT
New Gerrit change created: https://git.eclipse.org/r/147034
Comment 61 Mickael Istria CLA 2019-08-06 13:17:43 EDT
@Kris: can you please add a note to https://www.eclipse.org/eclipse/news/4.13/platform.php ? This can be done by editing files in this Git repo: https://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/
Comment 62 Eclipse Genie CLA 2019-08-06 14:31:05 EDT
New Gerrit change created: https://git.eclipse.org/r/147141
Comment 63 Kris De Volder CLA 2019-08-06 14:34:33 EDT
I've added a note to where (I think) it should go. See gerrit changeset above.
Comment 64 Mickael Istria CLA 2019-08-07 03:59:58 EDT
Additionally to N&N (that is under review), please also try adding a note about it in the documentation.
See possible files to modify:
* https://git.eclipse.org/c/platform/eclipse.platform.common.git/tree/bundles/org.eclipse.platform.doc.user/gettingStarted/qs-36.htm (and qs-36a, qs-36b) should also mention the Quick Text Search linking to a dedicated page (such as a qs-36c, cloned and adapted from qs-36a)
* https://git.eclipse.org/c/platform/eclipse.platform.common.git/tree/bundles/org.eclipse.platform.doc.user/tasks/tasks-84.htm to mention the Quick Text Search as an alternative. This could be 2 sections of the same page "Using the general Search command" and "using the Quick Text Search".

Once N&N and docs are in, we'll mark this issue as resolved.
Comment 66 Eclipse Genie CLA 2019-08-07 18:44:47 EDT
New Gerrit change created: https://git.eclipse.org/r/147239
Comment 68 Mickael Istria CLA 2019-08-08 14:43:38 EDT
Ok, I think we're good to mark it merged: feature is in, working and providing value, is in N&N and documentation.
Let's mark this one as resolved and open further tickets for specific issues.

Thanks a lot to Kris, it's a very very good addition to the IDE!
Comment 69 Sarika Sinha CLA 2019-08-12 04:22:44 EDT
One observation -
Progress view keeps on flickering till the search operation is completed.
Steps to reproduce:
1. Have a medium sized workspace.
2. Open Progress view.
3. Open Quick Search dialog and type a word like"quick", we can see the flicker in the "Progress view".
Comment 70 Mickael Istria CLA 2019-08-12 04:28:21 EDT
(In reply to Sarika Sinha from comment #69)
> One observation -
> Progress view keeps on flickering till the search operation is completed.
> Steps to reproduce:
> 1. Have a medium sized workspace.
> 2. Open Progress view.
> 3. Open Quick Search dialog and type a word like"quick", we can see the
> flicker in the "Progress view".

Thanks Sarika.
Can you please report it in a dedicated ticket?
Comment 71 Dani Megert CLA 2019-08-14 05:45:48 EDT
Please see bug 550058 for name mismatch.
Comment 72 Noopur Gupta CLA 2019-08-22 05:45:01 EDT
The "Quick Search" preference page can be moved under "Preferences > General > ..." instead of adding it as a separate item in the preferences. If this wasn't already discussed, I can open a new bug report for this.
Comment 73 Mickael Istria CLA 2019-08-22 06:01:36 EDT
(In reply to Noopur Gupta from comment #72)
> The "Quick Search" preference page can be moved under "Preferences > General
> > ..." instead of adding it as a separate item in the preferences. If this
> wasn't already discussed, I can open a new bug report for this.

I don't think it was discussed and it'd be fine to improve it. Please open a new ticket with this proposal.
Comment 74 Sarika Sinha CLA 2019-08-27 06:32:35 EDT
(In reply to Eclipse Genie from comment #65)
> Gerrit change https://git.eclipse.org/r/147141 was merged to [master].
> Commit:
> http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/
> ?id=ae427d8d046ad5853976efed621021983552e8bd

The width of the image in N&N is more than 720 pixels, this needs to be fixed.
Comment 75 Rafael Chaves CLA 2019-11-27 07:39:42 EST
*** Bug 434139 has been marked as a duplicate of this bug. ***