Add Filter Mechanism for Target in Move Refactoring#18

[ShahzaibIbrahim]

To let user filter the projects or packages from a search bar in a move refactor dialog.

Fixes vi-eclipse/Eclipse-JDT#18

What it does

User can now search from the large tree of projects and packages to move the file to.

How to test

Go to any file, right click --> Refactor --> Move --> Dialog Box will open with a treeview of Project and Packages --> Type in the search box and see if the filters are working fine

• [Yes ] I have thoroughly tested my changes
• [ Yes] The change is following the coding conventions
• [ Yes] I have signed the Eclipse Contributor Agreement (ECA)

[fedejeanne]

I tested it and it's pretty slow on a very big WS (around 1800 plugins): almost 1 minute to input 2 characters in the search bar: Slow filter in move refactoring.zip

[fedejeanne]

And the search field should be as wide as the dialog...

[HeikoKlare]

I have re-evaluated the proposal with our workspace and still have similar results:

I do not upload another snapshot file, as it is basically the same as before.

From the sampling, it looks as if the problem is the call to expandAll() when modifying the text. This is particularly expensive when you have entered few characters, because the tree is pretty large then. Some ideas on that:

• Is it really necessary to do the expandAll()? Can't that be something the user has to do on their own (like now)?
• Would it be possible to defer the expansion to be executed some time after the last character was entered? (e.g., wait one second after the last text modification before starting the filter calculation and/or expansion)
• Can we provide a progress for the expansion operation, which the user might cancel if it takes too long (in particular in combination with the deferred expansion mentioned above)?

[ShahzaibIbrahim]

The idea behind filtering the tree:

First Check if Search String is matching proj or any package of that proj -> yes -> Keep it open (for now) will filter a/c to package in another level -> No -> Remove the project entirely from the tree

Then going down on package level check each package and remove any that doesn't match

Analysis ------------------------------------

Search String: "org"

Package Name: src -> Checking Upper level -> Dont Keep
Package To String: src [in LestProj]
(...)
LestPackage (...)
Package Name: src -> Checking Upper level --> Keep
Package To String: src [in TestProj]
(...)
org (...)
org.kava (...)
Package Filtering : src [in TestProj]
(...) --> Don't Keep
org (...) --> Keep
org.kava (...) --> Keep

[HeikoKlare]

I've reevaluated the current implementation. Unfortunately, it is still unusable with our workspace. There are two reasons:

1. The implementation of the "debounce delay" is incorrect, so that updates are always performed, even if the user is still typing. This is because on first text modification, the delay does never apply: To identify whether the update shall be delayed because of current user input, the time of the last keystroke is used. However, on first input, this value is not set, i.e., it is "0". And the current time is always more than 1 second behind "0", which is why the update is performed. If the update then takes longer than 1 second, you have no chance to even trigger the delay mechanism for further keystrokes. So either a start time of "0" has to be used as a kind of magic value to identify that this is the first input, or an explicit "typing" variable has to be added that identifies whether the user is currently typing.
2. The expandAll operation still takes far too long. At least this is no surprise, because if I am not mistaken the implementation was not changed in this regard. In combination with (1), this makes it still unusable for us, but even with (1), I don't think that we can accept calculation times of several seconds once we stopped making changes to the filter for one second.
   

When testing the current implementation, I started wondering why we try to do an expandAll at all. Imagine you have a large tree where the topmost node matches your filter but all the childs do not. Then the whole tree will be expanded, even though the filter just applies to the topmost node. So I would expect the expansion to only happen until that node.
More precisely: If we do an automatic expansion, I would expect that it only expands to every child that matches the filter.

Another option would be to at least only expand source folders, as other folders are unlikely to be used as a target for a move operation of a compilation unit. Still I would say that expansion is nothing that should happen completely automatically (as its performance is obviously too bad). In my opinion, we should just drop it for now and maybe later add some additional expansion button (e.g., next to the filter input field) that triggers this operation explicitly.

Note that there are also build failures because of discouraged type accesses. This also has to be fixed.

[HeikoKlare]

I just came across org.eclipse.ui.dialogs.FilteredTree, which already seems to provide most (if not all) of the filter functionality required here. In particular, it already implements deferred refresh (when input did not change for some time period), And there is also a maximum time used to auto-expand the tree, such that the UI will not freeze but expanding will just stop when taking too much time. Maybe we need some customizing of how the filter applies to the tree, but that should be easier to do than reimplementing the rest of what FilteredTree already provides.

[ShahzaibIbrahim]

I just came across org.eclipse.ui.dialogs.FilteredTree, which already seems to provide most (if not all) of the filter functionality required here. In particular, it already implements deferred refresh (when input did not change for some time period), And there is also a maximum time used to auto-expand the tree, such that the UI will not freeze but expanding will just stop when taking too much time. Maybe we need some customizing of how the filter applies to the tree, but that should be easier to do than reimplementing the rest of what FilteredTree already provides.

Sounds good. I can try that.

[ShahzaibIbrahim]

Closing this PR, found already integrated solution with FilteredTree.

New PR: #1246