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

Using DI for creating the ShowViewDialog #187

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

Conversation

vogella
Copy link
Contributor

@vogella vogella commented Jul 5, 2022

Instead of getting the parameters in our handler for calling the
constructor of ShowViewDialog we can also use DI do to this, the
resulting code is slightly shorter. No big win but a tiny move for using
more DI in internal Eclipse code.

MWindow window = workbenchWindow.getService(MWindow.class);

final ShowViewDialog dialog = new ShowViewDialog(shell, app, window, modelService, partService, ctx);
private static void openOther(IWorkbenchWindow workbenchWindow, IEclipseContext ctx, EPartService partService) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not only IEclipseContext ctx be required here then?

@laeubi
Copy link
Contributor

laeubi commented Jul 5, 2022

This is really useful and you can also use this to inject OSGi Services into UI components without using static access then!

I wonder if it would make sense to even replace ContextInjectionFactory.make(..., IEclipseContext) by an E4 service that could be injected instead (DependecyInjector?) that one could use for such purpose as an alternative to the IEclipseContext, I find this passing of IEclipseContext around and using static ContextInjectionFactory defeats the purspose of DI, also it would be usefull to have then a Method that takes a VarArg of additional parameters to pass as the additional context, then client code do not need to bind to ContextInjectionFactory/IEclipseContext/EclipseContextFactory

@vogella
Copy link
Contributor Author

vogella commented Jul 5, 2022

This is really useful and you can also use this to inject OSGi Services into UI components without using static access then!

Glad you like it. Would be great if this API could be used more in Eclipse to reduce code.

I wonder if it would make sense to even replace ContextInjectionFactory.make(..., IEclipseContext) by an E4 service that could be injected instead (DependecyInjector?) that one could use for such purpose as an alternative to the IEclipseContext, I find this passing of IEclipseContext around and using static ContextInjectionFactory defeats the purspose of DI, also it would be usefull to have then a Method that takes a VarArg of additional parameters to pass as the additional context, then client code do not need to bind to ContextInjectionFactory/IEclipseContext/EclipseContextFactory

I don't mind, getting the fitting IEclipseContext might be a challenge. Please open a new ticket / PR for this idea if you plan to work on this.

@akurtakov
Copy link
Member

I've seen some activity about more E4 lately and wondered whether you guys are interested in finishing this one?

HannesWell pushed a commit to HannesWell/eclipse.platform.ui that referenced this pull request Jul 1, 2023
- add filtering to FileTreeContentProvider.elementsChanged() method so
  extraneous line elements are already filtered out of lineMatches
  set
- fixes eclipse-platform#187
Instead of getting the parameters in our handler for calling the
constructor of ShowViewDialog we can also use DI do to this, the
resulting code is slightly shorter. No big win but a tiny move for using
more DI in internal Eclipse code.
Copy link
Contributor

Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit 27b9bd4.

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.

None yet

3 participants