-
Notifications
You must be signed in to change notification settings - Fork 151
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
Always inject FinishedJobs instance into ProgressServiceImpl #1887 #1888
Always inject FinishedJobs instance into ProgressServiceImpl #1887 #1888
Conversation
Hello @selundqma , thank you for contributing this! Could you please provide a snippet to reproduce the error (even better, a test)? I wonder if removing the Can you describe the behavior more in detail? Are the other fields (always) being injected? Are there some use cases where |
@fedejeanne The field will be filled if someone else requested the creation of the singelton, I think thats the original intend but I can't see it harms as the object is literally always created on all other places. Beside that it might be just that the |
What I understand from this is that it should be possible to request the creation of the @selundqma if you could provide a snippet to reproduce the error maybe we could analyze it and see if something is missing.
If that's the case then the other fields in the class should also be checked for |
The Class is marked with |
If I'm reading correctly between the lines here, this means that the lazy initialization failed and the solution (removing |
Hi @fedejeanne and @laeubi! I guess the person(s) who wrote the code is not available anymore to explain the reason for the use of It is hard for me to provide any code snippets due to company policies. In short we create a job (WorkspaceJob) and schedule it, implement the ProgressMonitor interface and use We also show the Progress View in our application and it is possible to cancel a single job in the Progress View since the view is created with the following method where FinishedJobs is injected without the
The other fields are registered in different ways and I guess that is why it works fine with those two:
An observation: ProgressManager.java, ProgressRegion.java, ClearAllHandler.java and more do not use Since the FinishedJobs class is both |
@selundqma thank you for your input. Can you explain why |
@selundqma thanks for the link, it explains it perfectly. |
Thank you for approving the PR! Just to understand the process here, what happens now? Are we waiting for more reviewers? Maybe you have "internal meetings" to decide what to merge or not? This is my first pull request for this project so I am a bit insecure how it all happens here. |
@selundqma once the freeze period is over, this PR can be merged. I happen to be out of office next week so I'll merge it in June. |
…platform#1887 The "finishedJobs" variable isn´t injected, thus is always null, when used from a progress dialog. Since the @optional field in combination with @Inject only refers to already created instances of FinishedJobs. But, this does not happen in this flow. Fixes eclipse-platform#1887
20c6aac
to
063a2fa
Compare
Please don't merge anything to master until master is opened for 4.33!!! |
The "finishedJobs" variable isn´t injected, thus is always null, when used from a progress dialog. Since the
@Optional
field in combination with@Inject
only refers to already created instances of FinishedJobs. But, this does not happen in this flow.Fixes #1887