Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Filter Mechanism for Target in Move Refactoring#18 #1246

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShahzaibIbrahim
Copy link

@ShahzaibIbrahim ShahzaibIbrahim commented Mar 9, 2024

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

Fixes #1337

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

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

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested the provided functionality and found it to be, as expected, almost equivalent to the one proposed in #981. But since it reuses the existing FilteredTree, it is much cleaner and avoids duplication.

This implementation does still not perform well on our workspace. Once I stop typing, it takes several seconds to update the viewer. This seems to be because the generic filter logic of FilteredTree (and the used getChildren() implementation of the refactoring's content provider) is quite expensive to calculate. But since that is not an issue of newly added functionality but given by the implementation of FilteredTree, we can address that independently via improvements to FilteredTree afterwards.
image

I see one option for slightly improving the behavior when having a large workspace in which the filter update takes a long time: the debounce delay might be increased from FilterTree's default setting of 200ms, as that value is quite low and can easily be exceeded between two character inputs. The value could be increased to something like 500ms by overwriting FilteredTree.getRefreshJobDelay() to return an according value.

I would even be in favor of the changes without that modification, as this is a quick win for having a filter mechanism in the move refactoring. And no one is even forced to use it.

@ShahzaibIbrahim
Copy link
Author

I see one option for slightly improving the behavior when having a large workspace in which the filter update takes a long time: the debounce delay might be increased from FilterTree's default setting of 200ms, as that value is quite low and can easily be exceeded between two character inputs. The value could be increased to something like 500ms by overwriting FilteredTree.getRefreshJobDelay() to return an according value.

@HeikoKlare I have applied your suggestion which gives us slightly more time before debouncing. The value for it is 500ms now. There is a pull request in platform.ui which needs to be merged before this PR now. eclipse-platform/eclipse.platform.ui#1824

Text Search added to search withing the tree while moving (refactoring) the file to different package

eclipse-jdt#1337
@jjohnstn
Copy link
Contributor

@ShahzaibIbrahim I tried a rebased version of this patch on Linux (Fedora 40) via child Eclipse and unfortunately it was unusable for me. After typing I would have to wait a long time for any results to show and I got multiple UI freezes where I had to Wait or Force Quit. This included just showing characters in the text entry when adding or erasing. I tried the alternate version of this patch (#981) and it worked much better for me. Response was reasonable. The only thing I ran into was that typing some characters in and then later removing them all caused a total freeze and I was forced to do a force quit (probably a simple check is missing somewhere for empty text). I also noted that in the original patch, there were markers for discouraged access to various classes rather than using their interfaces (e.g. using JavaProject instead of IJavaProject).

Copy link
Contributor

@jjohnstn jjohnstn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI freezes need to be addressed on Linux.

@HeikoKlare
Copy link
Contributor

@jjohnstn do you experience similar issues with other usages of FilteredTree, such as in the product's/IDE's properties dialog (when filtering for property pages)? Instead of adopting the custom solution of #981, I think we should better find out why the already implemented FilteredTree has these performance problems and how we can resolve them, so that every existing and future usage of that tree can benefit from it rather than only a custom one in the move refactoring.

@jjohnstn
Copy link
Contributor

@HeikoKlare In preferences, using filtering in the top left, filtering in the General->Keys, and the Java->Compiler->Errors/Warnings page all work fine for me without freezing both in my parent Eclipse and my child Eclipse. I didn't confirm if those pages use FilteredTree.

@jjohnstn
Copy link
Contributor

jjohnstn commented Jul 2, 2024

@ShahzaibIbrahim Have you looked at caching the data, overriding any input provider, so that you prevent a refetch? I imagine the original dialog didn't care about caching because it was only expecting to have the original list and never calling the input provider after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Filter Mechanism for Target in Move Refactoring
3 participants