Product file editor: Support for <bundleUrlTypes> on macOS #1326
Conversation
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/product/LauncherInfo.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/product/LauncherInfo.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/product/LauncherInfo.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/product/LauncherInfo.java
Outdated
Show resolved
Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/product/LauncherInfo.java
Outdated
Show resolved
Hide resolved
Test Results 285 files ±0 285 suites ±0 46m 14s ⏱️ - 4m 45s Results for commit ad53e00. ± Comparison against base commit edf6f1b. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
HannesWell
force-pushed
the
linkhandlers
branch
from
163aad6 to
8b7d317
Compare
Support editing of <launcher><macosx><bundleUrlTypes> entries in the product editor. These are translated to CFBundleURLTypes entries in the Info.plist dictionary of the macOS app bundle. Contributes to eclipse-platform/eclipse.platform.ui#1901.
HannesWell
force-pushed
the
linkhandlers
branch
from
8b7d317 to
ad53e00
Compare
There was a problem hiding this comment.
Thanks @sratz for your patience, I now reviewed this change in detail and in general it looks good.
I think LauncherInfo.removeMacBundleUrlTypes() was not completely correct although it would probably not have been completely dysfunctional.
I pushed a fix to your branch together with multiple other minor clean-ups. In references to them: There is a lot of old code in the area of the editors but I see no need to copy the old style in new code. :)
If you are fine with the changes I'll submit this tomorrow.
| public void removeMacBundleUrlTypes(List<IMacBundleUrlType> bundleUrlTypes) { | ||
| IProductObject[] removedUrlSchemes = bundleUrlTypes.stream() // | ||
| .map(IMacBundleUrlType::getScheme).map(fMacBundleUrlTypes::remove) // | ||
| .filter(Objects::nonNull).toArray(IProductObject[]::new); | ||
|
|
||
| if (removedUrlSchemes.length > 0 && isEditable()) { | ||
| fireStructureChanged(removedUrlSchemes, IModelChangedEvent.REMOVE); | ||
| } | ||
| } |
There was a problem hiding this comment.
The original logic in this case seems not to be correct since it is attempted twice to remove the scheme from the fMacBundleUrlTypes map?
Did you intend to send an event for the list of schemes passed and the list of schemes actually removed?
There was a problem hiding this comment.
Thanks for catching this! You are right, this was definitely not intended. I accidentally took Product#removePlugins() as a template, but that of course has different semantics (removing a plugin also removes its configuration).
Thanks a lot for the review and the improvements to the code!
We just tested the PDE code end-to-end again and everything looks good! So this can be merged 👍 |
|
Nice teamwork. |
Awesome, thanks for checking it again. Then lets have this. |
Follow-up on eclipse-pde#1326. Contributes to eclipse-platform/eclipse.platform.ui#1901.
Follow-up on eclipse-pde#1326. Contributes to eclipse-platform/eclipse.platform.ui#1901.
Follow-up on #1326. Contributes to eclipse-platform/eclipse.platform.ui#1901.
Corner case (no <launcher> element present, yet) was missed in eclipse-pde#1326. Fixes eclipse-pde#1443.
Support editing of entries in the product editor.
These are translated to CFBundleURLTypes entries in the Info.plist dictionary of the macOS app bundle.
Contributes to
eclipse-platform/eclipse.platform.ui#1901.