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

Offer simplified version to create LocalResourceManager #1213

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Oct 11, 2023

Frequently LocalResourceManager(JFaceResources.getResources(), parent) is used in client code. This add a helper method on JFaceResource to simplify such call.

Frequently LocalResourceManager(JFaceResources.getResources(), parent)
is used in client code. This add a helper method on JFaceResource to
simplify such call.

Fixed #1208
@github-actions
Copy link
Contributor

Test Results

     900 files  ±0       900 suites  ±0   1h 32m 57s ⏱️ - 17m 50s
  7 449 tests ±0    7 292 ✔️  - 1  156 💤 ±0  1 +1 
23 493 runs  ±0  22 977 ✔️  - 1  515 💤 ±0  1 +1 

For more details on these failures, see this check.

Results for commit 1539272. ± Comparison against base commit 16cb21c.

@vogella
Copy link
Contributor Author

vogella commented Oct 11, 2023

Random failing test already known, see #926

Planning to merge soon cc @laeubi @HannesWell

* registry.
*/
static LocalResourceManager managerFor(Control owner) {
return new LocalResourceManager(getResources(), owner);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use control.getData/setData to cache the instance created so multiple calls could be performed without creating multiple managers.

Copy link
Member

Choose a reason for hiding this comment

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

Can you estimate how often that will hit the cache?
Since the local-manager is a child from the global one caching would not save SWT-resources but mainly only another map.

@vogella
Copy link
Contributor Author

vogella commented Oct 13, 2023

I think caching can be applied in a different commit if desired. This one had the intention to provide a simpler way to create a new LocalResourceManager which I think it does.

@vogella vogella merged commit 5f47dac into master Oct 13, 2023
* @param owner control whose disposal will trigger cleanup of everything in the
* registry.
*/
static LocalResourceManager managerFor(Control owner) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this package private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this package private?

Thanks for the catch. Will be fixed with
#1218

@vogella vogella deleted the jfaceresources branch October 18, 2023 13:27
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.

4 participants