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

Target platform icon for "Load TP" job #1034

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

Conversation

Bananeweizen
Copy link
Contributor

Register the typical target platform icon for the job family of the "Loading target platform" job. That leads to the icon being shown next to the progress bar of the progress view, similar to how the build has its own icon and all egit, m2e etc. jobs have their own icon.

grafik

Copy link

github-actions bot commented Dec 31, 2023

Test Results

  4 files   -    273    4 suites   - 273   6m 10s ⏱️ - 44m 50s
166 tests  -  3 330  160 ✅  -  3 300   6 💤  - 30  0 ❌ ±0 
332 runs   - 10 287  320 ✅  - 10 197  12 💤  - 90  0 ❌ ±0 

Results for commit 21e6da9. ± Comparison against base commit 8fbf299.

This pull request removes 3330 tests.
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_importPackage
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeFragments
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeFragmentsProvidingPackages
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeNonTestFragments
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_includeOptional
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requireBundle
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requireBundle2
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requireDifferentVersions
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.DependencyManagerTest ‑ testFindRequirementsClosure_requiredCapability
AllPDETests org.eclipse.pde.core.tests.internal.AllPDECoreTests org.eclipse.pde.core.tests.internal.WorkspaceModelManagerTest ‑ testBundleRootHandling_bundleRootChangedFromDefaultToOthersAndReverse
…

♻️ This comment has been updated with latest results.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks, in general a good idea.
Just a few remarks about how to implement this.

/**
* @since 3.17
*/
public static Object getFamily() {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding this new API I would prefer to move JOB_FAMILITY_ID as public constant to an internal class, e.g. ICoreConstants, and references it that here and int PDEPlugin.

@@ -42,6 +43,7 @@
import org.eclipse.ui.internal.views.log.ILogFileProvider;
import org.eclipse.ui.internal.views.log.LogFilesManager;
import org.eclipse.ui.plugin.AbstractUIPlugin;
import org.eclipse.ui.progress.IProgressService;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why there is org.eclipse.ui.progress.IProgressService and org.eclipse.e4.ui.progress.IProgressService which looks very similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

One is the "old" eclipse.ui and the other the "new" rcp E4 ... but I think the E4 one is not really completed and would need a declarative way (e4xmi) to add icons for job groups.

Comment on lines +222 to +230
protected void registerProgressIcon() {
if (!PlatformUI.isWorkbenchRunning()) {
return;
}
IProgressService service = PlatformUI.getWorkbench().getProgressService();
if (service == null) {
return;
}
service.registerIconForFamily(PDEPluginImages.DESC_TARGET_DEFINITION, LoadTargetDefinitionJob.getFamily());
Copy link
Member

Choose a reason for hiding this comment

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

Can't this be simply done in a static initializer in LoadTargetDefinitionJob?
This would also solve the problem where to place the job-family constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The job lives in pde.core. The registration for the icon happens with the workbench progress service, which looks like a UI dependent class to me. I'm not sure if that service can be retrieved in the plain pde.core bundle without disturbing the workbench lifecycle. In egit we therefore even put that code into a service reacting to the application has started lifecycle.
Do you know better?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think given the current structure, this is fine, an alternative might be to use an addon in the e4xmi but that really seems a bit overhead here...

Copy link
Member

Choose a reason for hiding this comment

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

Do you know better?

You are right and no, I don't know better in this case.

Register the typical target platform icon for the job family of the
"Loading target platform" job. That leads to the icon being shown next
to the progress bar of the progress view, similar to how the build has
its own icon and all egit, m2e etc. jobs have their own icon.
@laeubi laeubi force-pushed the target_platform_progress_icon branch from aaaf0f6 to 21e6da9 Compare January 16, 2024 05:44
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Since we can't add the icon in the Job itself please create constant for the job-family instead of adding a new API method getFamily().
Besides that this change is fine.

Comment on lines +222 to +230
protected void registerProgressIcon() {
if (!PlatformUI.isWorkbenchRunning()) {
return;
}
IProgressService service = PlatformUI.getWorkbench().getProgressService();
if (service == null) {
return;
}
service.registerIconForFamily(PDEPluginImages.DESC_TARGET_DEFINITION, LoadTargetDefinitionJob.getFamily());
Copy link
Member

Choose a reason for hiding this comment

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

Do you know better?

You are right and no, I don't know better in this case.

@laeubi
Copy link
Contributor

laeubi commented Apr 29, 2024

@Bananeweizen can you apply the suggestion from @HannesWell so we can merge this?

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