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.

Sign up for GitHub

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

Jump to bottom

Make JFaceResources#managerFor public #1218

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Make JFaceResources#managerFor public #1218

merged 1 commit into from
Oct 18, 2023

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Oct 13, 2023

No description provided.

@vogella vogella mentioned this pull request Oct 13, 2023
Merged
@iloveeclipse
Copy link
Member

Ideally every new API should have a test added.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2023 •
edited
Loading

Test Results

     899 files  ±0       899 suites  ±0   1h 30m 4s ⏱️ -28s
  7 450 tests +1    7 293 ✔️ +1  156 💤 ±0  1 ❌ ±0 
21 941 runs  +3  21 545 ✔️ +3  395 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 014ba18. ± Comparison against base commit e3d651a.

♻️ This comment has been updated with latest results.

HannesWell
HannesWell reviewed Oct 14, 2023
View reviewed changes
tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/resources/JFaceResourcesTest.java
Display d = Display.getDefault();
Shell shell = new Shell(d);
Composite composite = new Composite(shell, SWT.NONE);
LocalResourceManager localResourceManager = JFaceResources.managerFor(composite);
Copy link
Member

Choose a reason for hiding this comment

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

Could we check here if localResourceManager.getDevice() returns the display d?
Just to have any kind of validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably dispose the shell, too. And check that the resource manager is disposed afterwards?

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/JFaceResources.java
*/
static LocalResourceManager managerFor(Control owner) {
static public LocalResourceManager managerFor(Control owner) {
Copy link
Member

Choose a reason for hiding this comment

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

The usual convention is to mention the visibility first.

Suggested change
static public LocalResourceManager managerFor(Control owner) {
public static LocalResourceManager managerFor(Control owner) {

@szarnekow
Copy link
Contributor

Since the original ticket was motivated by repeated code, it would be great if the new API was used throughout the codebase (local to this repository).

@vogella
Copy link
Contributor Author

vogella commented Oct 17, 2023

Since the original ticket was motivated by repeated code, it would be great if the new API was used throughout the codebase (local to this repository).

I agree, could be a good follow-up PR.

@vogella
Copy link
Contributor Author

vogella commented Oct 17, 2023

Ideally every new API should have a test added.

Done

@vogella
Copy link
Contributor Author

vogella commented Oct 18, 2023

Virtual test failure fails randomly, see #1005

@vogella vogella merged commit fbe0fac into master Oct 18, 2023
13 of 18 checks passed
@vogella vogella deleted the jfaceresources2 branch October 18, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szarnekow szarnekow szarnekow left review comments

@HannesWell HannesWell HannesWell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vogella @iloveeclipse @szarnekow @HannesWell