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

Migrate AutomaticUpdateScheduler from IStartup to OSGi EventHandler #400

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

... for the UIEvents.UILifeCycle.APP_STARTUP_COMPLETE event.

This is a less Eclipse specific approach and also prevents the user from disabling the start-up task.
The automatic search for updates can still be disabled.

Based on the work in eclipse-platform/eclipse.platform.ui#1362.

Copy link

github-actions bot commented Dec 9, 2023

Test Results

  375 files  ±0    375 suites  ±0   42m 58s ⏱️ - 2m 22s
1 895 tests ±0  1 892 ✅ ±0  3 💤 ±0  0 ❌ ±0 
6 685 runs  ±0  6 676 ✅ ±0  9 💤 ±0  0 ❌ ±0 

Results for commit 8e319bf. ± Comparison against base commit 775774f.

♻️ This comment has been updated with latest results.

@@ -56,7 +62,7 @@ public class AutomaticUpdateScheduler implements IStartup {
private IUpdateChecker checker;

@Override
public void earlyStartup() {
public void handleEvent(Event event) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we check if automated updates are disabled before starting the job?

@iloveeclipse
Copy link
Member

With "Early startup" one could disable updates job. Now it is always started even if updates are not enabled. I think this is a regression.

@HannesWell HannesWell force-pushed the update-scheduler-as-eventHandler branch from 52b2783 to 613cd77 Compare December 9, 2023 14:17
@HannesWell
Copy link
Member Author

HannesWell commented Dec 9, 2023

With "Early startup" one could disable updates job. Now it is always started even if updates are not enabled. I think this is a regression.

There is a dedicated preference to control the automatic search for updates:
grafik

So yes now the job always runs, but the job still respects the preference so if you have disabled updates, it is basically a noop.

I can also refactor this to only run the job if the preference is enabled, but I'm not sure what's best in regard of IDE start-up speed. Check the preference before running the job or within the job and potentially exit early?
Of course starting a job has an overhead but so does checking a preference.

@HannesWell HannesWell force-pushed the update-scheduler-as-eventHandler branch from 613cd77 to b3215ef Compare January 19, 2024 17:57
@laeubi
Copy link
Member

laeubi commented Jun 16, 2024

@HannesWell @iloveeclipse should we further work on this or should we close this?

@HannesWell HannesWell force-pushed the update-scheduler-as-eventHandler branch 2 times, most recently from bfe3d8e to f5213b9 Compare October 12, 2024 11:27
@HannesWell
Copy link
Member Author

@HannesWell @iloveeclipse should we further work on this or should we close this?

I would still be in favor of having this and was waiting for the response of @iloveeclipse on my previous response:

I can also refactor this to only run the job if the preference is enabled, but I'm not sure what's best in regard of IDE start-up speed. Check the preference before running the job or within the job and potentially exit early?
Of course starting a job has an overhead but so does checking a preference.

@eclipse-equinox-bot
Copy link
Contributor

eclipse-equinox-bot commented Oct 12, 2024

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.equinox.p2.ui.sdk.scheduler/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From bb09f38612e3dd269472533abb01dd5313f69ec0 Mon Sep 17 00:00:00 2001
From: Eclipse Equinox Bot <[email protected]>
Date: Tue, 15 Oct 2024 17:34:06 +0000
Subject: [PATCH] Version bump(s) for 4.34 stream


diff --git a/bundles/org.eclipse.equinox.p2.ui.sdk.scheduler/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.p2.ui.sdk.scheduler/META-INF/MANIFEST.MF
index 011622b8c..106de039f 100644
--- a/bundles/org.eclipse.equinox.p2.ui.sdk.scheduler/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.equinox.p2.ui.sdk.scheduler/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %bundleName
 Bundle-SymbolicName: org.eclipse.equinox.p2.ui.sdk.scheduler;singleton:=true
-Bundle-Version: 1.6.300.qualifier
+Bundle-Version: 1.6.400.qualifier
 Bundle-Activator: org.eclipse.equinox.internal.p2.ui.sdk.scheduler.AutomaticUpdatePlugin
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
-- 
2.46.2

Further information are available in Common Build Issues - Missing version increments.

@HannesWell HannesWell force-pushed the update-scheduler-as-eventHandler branch from a7eee7f to f5213b9 Compare October 12, 2024 13:11
... for the UIEvents.UILifeCycle.APP_STARTUP_COMPLETE event.

This is a less Eclipse specific approach and also prevents the user from
disabling the start-up task.
The automatic search for updates can still be disabled.
@HannesWell HannesWell force-pushed the update-scheduler-as-eventHandler branch from b426237 to 4a67dd8 Compare October 15, 2024 17:31
@iloveeclipse
Copy link
Member

Check the preference before running the job or within the job and potentially exit early?
Of course starting a job has an overhead but so does checking a preference.

I would avoid starting job if possible, because compared with a single preference check extra job adds extra complexity for the overall IDE startup, uses shared resources (worker pool), adds async execution effects.

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