-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add support for bundle URL types in macOS launcher in product files #533
Conversation
...p2.publisher.eclipse/src/org/eclipse/equinox/internal/p2/publisher/eclipse/BrandingIron.java
Outdated
Show resolved
Hide resolved
|
||
import org.eclipse.equinox.p2.publisher.eclipse.IMacOsBundleUrlType; | ||
|
||
public class MacOsBundleUrlType implements IMacOsBundleUrlType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a Java Record here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought about that, but I opted for consistency with the other interfaces / minimal implementations in the package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also in favor of using records. Sorry I just read this remark after assembling my other commit and suggested it there already.
In order to be consistent with other code in this plugin I suggest to use records wherever suitable.
In PDE I recently submitted a change where I even moved the record from a separate file into the only caller of the constructor, because having an (effectively) one-line in a separate file does not make sense to me.
...uinox.p2.publisher.eclipse/src/org/eclipse/equinox/p2/publisher/eclipse/IBrandingAdvice.java
Outdated
Show resolved
Hide resolved
...uinox.p2.publisher.eclipse/src/org/eclipse/equinox/p2/publisher/eclipse/IBrandingAdvice.java
Outdated
Show resolved
Hide resolved
...lisher.eclipse/src/org/eclipse/equinox/internal/p2/publisher/eclipse/IProductDescriptor.java
Show resolved
Hide resolved
...g.eclipse.equinox.p2.tests/testData/ProductActionTest/productWithMacOsBundleUrlTypes.product
Outdated
Show resolved
Hide resolved
a9ccf9b
to
e95cd21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sratz also for your patience on this one.
This also looks fine to me, but I couldn't test it in action. I assume you are confident that this is the right general way. At least to me it looks sane.
I have pushed a few clean-ups/some streamlining to your branch. Here also the remark from PDE: I don't think it is necessary to be consistent with old styles, especially for internals (I'm actually in favor of the opposite: modernize the code at every occasion).
If you are fine with the additions, I'll squash and submit them tomorrow.
3f3d1e2
to
739b14e
Compare
Thanks for the review!
Thanks for taking the time to improve the code. Looks good!
We also ran an end-to-end test on this part of the functionality:
--> The resulting macOS package contains the correct So this can also be merged 👍 (*) Tycho needs a small adjustment as well: It wraps diff --git a/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java b/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java
index 734d5b4dc..2d7a5137c 100644
--- a/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java
+++ b/tycho-core/src/main/java/org/eclipse/tycho/p2/tools/publisher/ExpandedProduct.java
@@ -28,6 +28,7 @@ import org.eclipse.equinox.internal.p2.publisher.eclipse.ProductContentType;
import org.eclipse.equinox.p2.metadata.IInstallableUnit;
import org.eclipse.equinox.p2.metadata.IVersionedId;
import org.eclipse.equinox.p2.repository.IRepositoryReference;
+import org.eclipse.equinox.p2.publisher.eclipse.IMacOsBundleUrlType;
import org.eclipse.tycho.ArtifactType;
import org.eclipse.tycho.Interpolator;
import org.eclipse.tycho.core.VersioningHelper;
@@ -291,4 +292,9 @@ class ExpandedProduct implements IProductDescriptor {
return defaults.getVM(os);
}
+ @Override
+ public List<IMacOsBundleUrlType> getMacOsBundleUrlTypes() {
+ return defaults.getMacOsBundleUrlTypes();
+ }
+
} But we will take care of the Tycho part as soon as Tycho consumes Eclipse 2024-09 P2. |
Adds CFBundleURLTypes entries to the Info.plist dictionary. Contributes to eclipse-platform/eclipse.platform.ui#1901.
739b14e
to
32e5deb
Compare
Great! Then, lets have this one in. I just squashed the commits and will submit it once the build passed again. |
After eclipse-tycho#4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
After eclipse-tycho#4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
…om 1.6.0 to 1.6.200 This is a precondition to use the new API provided with eclipse-equinox/p2#533 and to properly support link handler registrations in macOS product launchers. More details: eclipse-platform/eclipse.platform.ui#1901
After eclipse-tycho#4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
After #4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
…om 1.6.0 to 1.6.200 This is a precondition to use the new API provided with eclipse-equinox/p2#533 and to properly support link handler registrations in macOS product launchers. More details: eclipse-platform/eclipse.platform.ui#1901
After eclipse-tycho#4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
This is a precondition to use the new API provided with eclipse-equinox/p2#533 and to properly support link handler registrations in macOS product launchers. More details: eclipse-platform/eclipse.platform.ui#1901
After eclipse-tycho#4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
This is a precondition to use the new API provided with eclipse-equinox/p2#533 and to properly support link handler registrations in macOS product launchers. More details: eclipse-platform/eclipse.platform.ui#1901
After eclipse-tycho#4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
This is a precondition to use the new API provided with eclipse-equinox/p2#533 and to properly support link handler registrations in macOS product launchers. More details: eclipse-platform/eclipse.platform.ui#1901
After #4246, we can now consume the new org.eclipse.equinox.internal.p2.publisher.eclipse.IProductDescriptor.getMacOsBundleUrlTypes() API in the Tycho wrapper org.eclipse.tycho.p2.tools.publisher.ExpandedProduct. This is necessary to properly support link handler registrations in product's launchers. See eclipse-platform/eclipse.platform.ui#1901 eclipse-equinox/p2#533 for more information.
Adds CFBundleURLTypes entries to the Info.plist dictionary.
Contributes to
eclipse-platform/eclipse.platform.ui#1901.