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

Adding functionality to change the delay time to expand the tree while searching. #1824

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Apr 17, 2024

Fixes (eclipse-jdt/eclipse.jdt.ui#1337)

Related Commit to eclipse-jdt/eclipse.jdt.ui#1246. (This needs to be merged before the mentioned pull request.)

@fedejeanne
Copy link
Contributor

I noticed there is also a FilteredTree for E4 (org.eclipse.e4.ui.dialogs.filteredtree.FilteredTree). I think these improvements would also make sense in there.

I would pass the timeout as a constructor parameter and make it final. Make sure to let the default (no-args) constructor call the new (1-arg) constructor with the current value of 200 to preserve the current functionality.

Copy link
Contributor

github-actions bot commented Apr 18, 2024

Test Results

   920 files  +  307     920 suites  +307   40m 25s ⏱️ + 14m 35s
 7 524 tests ±    0   7 373 ✅ ±    0  150 💤 ± 0  1 ❌ ±0 
22 150 runs  +6 407  21 764 ✅ +6 335  385 💤 +72  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 83b00bd. ± Comparison against base commit 3c86ecf.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Looks good in general, I only have one minor change request about preserving the @since information in a constructor and some nit-picking suggestions to reduce code duplication in other constructors (I think it improves readability too when one calls other constructors).

Please also squash commits.

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.

Recent changes do not reflect my initial comment (#1824 (comment)):

As an additional note: the public API should not expose an information about a "refresh job". The public API could state that there is some delay after which the tree contents are refreshed, but doing that via a job is an implementation detail that should be hidden from the public API.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Ready to go on my side (after removing the last unnecessary empty line).

Please address Heiko's comment though.

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 two further comments. One is only about clarity, the other is about incorrect versioning, if I am not mistaken.

@HeikoKlare
Copy link
Contributor

Please also adapt the commit message according to the information in the CONTRIBUTING.MD, i.e., with a one-line title and and a short description of what the commit does. In particular, when referring to an issue in the commit message it should be placed in the "official" GitHub repositories (and not, e.g., in https://github.com/vi-eclipse/ like in the current commit message).

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.

Functionally, this looks fine now, thanks!

Sorry for being nitpick about the commit message, but it is currently rather confusing: the header states something about the move refactoring, but the commit is about making the refresh delay for the FilteredTree configurable, so that's what the commit message should state. It does not really matter for this commit that the (first) consumer of this functionality will be the move refactoring of JDT. In addition, the issue reference in the commit message is wrong. It points to #1337. You can keep the link to the JDT issue in the commit message description, but the short link #1337 in the commit message header will not point to a JDT issue but to platform UI. So please remove the #1337 reference from the commit message header.

@HeikoKlare
Copy link
Contributor

I am sorry, but there is still a reference to an unrelated issue (#1337 of platform.ui instead of jdt.ui) in the commit message.

@ShahzaibIbrahim
Copy link
Contributor Author

I am sorry, but there is still a reference to an unrelated issue (#1337 of platform.ui instead of jdt.ui) in the commit message.

Sorry for that. The issue was linked by itself. All I wanted was just plain-text issue reference that I added below.

…e searching.

The api consumer can pass the delay time to expand the tree while searching, the change was made for the issue eclipse-jdt#1337 as this will be its first consumer

eclipse-jdt/eclipse.jdt.ui#1337
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.

Thank you for the contribution.

@HeikoKlare
Copy link
Contributor

Failing test is unrelated: #1005

@HeikoKlare HeikoKlare merged commit d70c02a into eclipse-platform:master Apr 24, 2024
14 of 16 checks passed
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.

3 participants