WorkbookEditorsHandler: Fix NPE in case name is null #1286

sratz · 2023-11-07T11:31:15Z

(Regression was introduced in 03277d3)

github-actions · 2023-11-07T11:58:50Z

Test Results

    899 files -       1     899 suites - 1 42m 13s ⏱️ - 9m 39s
  7 468 tests ±      0   7 314 ✔️ +      2 153 💤 ±    0 1 ❌ - 2
21 977 runs - 1 576 21 587 ✔️ - 1 454 389 💤 - 120 1 ❌ - 2

For more details on these failures, see this check.

Results for commit b5aad0e. ± Comparison against base commit 4c5fccc.

♻️ This comment has been updated with latest results.

laeubi · 2023-11-07T12:28:21Z

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/WorkbookEditorsHandler.java

@@ -162,10 +164,10 @@ private List<EditorReference> getParts(WorkbenchPage page) {
 	 */
 	private Map<EditorReference, String> generateColumnLabelTexts(List<EditorReference> editorReferences) {
 		Map<EditorReference, String> editorReferenceLabelTexts = new HashMap<>(editorReferences.size());
-		Map<String, List<EditorReference>> collisionsMap = editorReferences.stream()
-				.collect(Collectors.groupingBy(r -> r.getName()));
+		Map<Optional<String>, List<EditorReference>> collisionsMap = editorReferences.stream()


maybe one should filter out null names altogether?

Then they would disappear from Ctrl-E?

I guess keeping them is the safest approach for now and just fix the regression introduced by 03277d3 .

What do you think about using Objects.requireNonNullOrElse(r.getName(), "N/A"); if the only problem is that name is null (whatever that the should mean to the user).

I think I have a better solution:

The actual intended text to be used in Ctrl-E (if unique), is ref.getTitle(),

eclipse.platform.ui/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/FilteredTableBaseHandler.java

Line 821 in 5b62380

return ref.getTitle();

which is already null-safe (returns "" in that case):

eclipse.platform.ui/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/EditorReference.java

Lines 227 to 240 in 5b62380

@Override

public String getTitle() {

String label = Util.safeString(getModel().getLocalizedLabel());

if (label.isEmpty()) {

if (input == null) {

if (descriptor != null) {

return descriptor.getLabel();

}

} else {

return Util.safeString(input.getName());

}

}

return label;

}

But in 03277d3 I have inadvertently used getName() to populate the collisionMap.

It should simply also use getTitle().

sratz · 2023-11-08T10:17:34Z

Any objections to merge like this? (I don't know why the build is failing, but it's definitely unrelated).

laeubi

Looks good

iloveeclipse

We can't merge, API tools are broken. @laeubi : please take a look.

See https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/PR-1286/4/console

09:45:32.437 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.4:verify (verify) on project org.eclipse.ui.workbench: There are API errors:
09:45:32.437 [ERROR] Eclipse UI/org/eclipse/ui/AbstractSourceProvider.java:31 The type org.eclipse.ui.AbstractSourceProvider has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/ActiveShellExpression.java:32 The type org.eclipse.ui.ActiveShellExpression has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/BasicWorkingSetElementAdapter.java:58 The type org.eclipse.ui.BasicWorkingSetElementAdapter has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/ExtensionFactory.java:46 The type org.eclipse.ui.ExtensionFactory has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IActionBars.java:66 The type org.eclipse.ui.IActionBars has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IActionBars2.java:24 The type org.eclipse.ui.IActionBars2 has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IActionDelegate.java:53 The type org.eclipse.ui.IActionDelegate has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IActionDelegate2.java:42 The type org.eclipse.ui.IActionDelegate2 has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IActionDelegateWithEvent.java:34 The type org.eclipse.ui.IActionDelegateWithEvent has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IActionFilter.java:52 The type org.eclipse.ui.IActionFilter has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IAggregateWorkingSet.java:30 The type org.eclipse.ui.IAggregateWorkingSet has been removed from org.eclipse.ui.workbench_3.131.0
09:45:32.438 [ERROR] Eclipse UI/org/eclipse/ui/IContainmentAdapter.java:22 The type 

...

jukzi · 2023-11-08T10:58:36Z

We can't merge, API tools are broken. @laeubi : please take a look.

see eclipse-equinox/p2#382 (comment)
currently it helps to add the default 'output.. = bin/' to build.properties.

jukzi · 2023-11-08T11:00:52Z

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/WorkbookEditorsHandler.java

@@ -163,7 +163,7 @@ private List<EditorReference> getParts(WorkbenchPage page) {
 	private Map<EditorReference, String> generateColumnLabelTexts(List<EditorReference> editorReferences) {
 		Map<EditorReference, String> editorReferenceLabelTexts = new HashMap<>(editorReferences.size());
 		Map<String, List<EditorReference>> collisionsMap = editorReferences.stream()
-				.collect(Collectors.groupingBy(r -> r.getName()));
+				.collect(Collectors.groupingBy(r -> r.getTitle()));


How about replacing null with empty string?

getTitle() already does that and does not return null

that can return descriptor.getLabel() that can return configurationElement.getAttribute(IWorkbenchRegistryConstants.ATT_NAME); that can return null

True, I have added another Util.safeString() guard around it.

iloveeclipse · 2023-11-09T10:25:07Z

see eclipse-equinox/p2#382 (comment) currently it helps to add the default 'output.. = bin/' to build.properties.

The problem is still there. Please fix that (even if it is not introduced by you).
M3 is currently in the process to be declared. No merges are expected before that.
Once M3 is officially declared, we could try to merge (assuming 1) is solved and build is green), but we will need a positive vote from a project lead to do that.

jukzi · 2023-11-09T10:44:36Z

The problem is still there. Please fix that (even if it is not introduced by you).

it's tychos problem eclipse-tycho/tycho#3019

Regression was introduced in 03277d3, where getTitle() was accidentally changed to getName(). Revert that back to getTitle(). getTitle() is already null-safe in most cases, but to be 100% safe, guard it using Util.safeString() again. Fixes eclipse-platform#1275.

sratz · 2023-11-14T09:55:20Z

@iloveeclipse: Any objections to merge for RC1?

iloveeclipse

LGTM

laeubi · 2023-11-14T10:00:18Z

@vogella @merks we need a PL/PMC approval for RC1 I think its worth to be considered.
@iloveeclipse build is green now except flaky VirtualTableViewerTest what I think is not affected by this change.

merks · 2023-11-14T10:03:01Z

PMC +1

I do generally trust that folks will use their discretion to fix things that really ought to be fixed and to avoid changing things that are risk.

laeubi reviewed Nov 7, 2023

View reviewed changes

sratz force-pushed the WorkbookEditorsHandler-npe branch 3 times, most recently from 67cfb45 to a88b209 Compare November 8, 2023 09:34

BeckerWdf approved these changes Nov 8, 2023

View reviewed changes

laeubi approved these changes Nov 8, 2023

View reviewed changes

iloveeclipse requested changes Nov 8, 2023

View reviewed changes

iloveeclipse mentioned this pull request Nov 8, 2023

Remove blank Javadoc eclipse-platform/eclipse.platform#841

Merged

jukzi reviewed Nov 8, 2023

View reviewed changes

jukzi mentioned this pull request Nov 8, 2023

ApiAnalysisMojo fails with API error if build.properties has missing 'output.. = bin/' entry eclipse-tycho/tycho#3019

Closed

sratz force-pushed the WorkbookEditorsHandler-npe branch from a88b209 to 47ccb55 Compare November 9, 2023 09:29

jukzi approved these changes Nov 9, 2023

View reviewed changes

iloveeclipse added this to the 4.30 RC1 milestone Nov 9, 2023

sratz added 2 commits November 14, 2023 09:02

Add output.. to build.properties to work around Tycho problem

b5aad0e

sratz force-pushed the WorkbookEditorsHandler-npe branch from 47ccb55 to b5aad0e Compare November 14, 2023 09:02

laeubi requested a review from iloveeclipse November 14, 2023 09:58

iloveeclipse approved these changes Nov 14, 2023

View reviewed changes

iloveeclipse merged commit b72dc9b into eclipse-platform:master Nov 14, 2023
13 of 16 checks passed

sratz deleted the WorkbookEditorsHandler-npe branch November 14, 2023 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WorkbookEditorsHandler: Fix NPE in case name is null #1286

WorkbookEditorsHandler: Fix NPE in case name is null #1286

sratz commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

laeubi Nov 7, 2023

sratz Nov 7, 2023

laeubi Nov 7, 2023

sratz Nov 7, 2023 •

edited

Loading

sratz commented Nov 8, 2023

laeubi left a comment

iloveeclipse left a comment •

edited

Loading

jukzi commented Nov 8, 2023

jukzi Nov 8, 2023

sratz Nov 8, 2023

jukzi Nov 8, 2023

sratz Nov 9, 2023

iloveeclipse commented Nov 9, 2023

jukzi commented Nov 9, 2023

sratz commented Nov 14, 2023

iloveeclipse left a comment

laeubi commented Nov 14, 2023

merks commented Nov 14, 2023

	@Override
	public String getTitle() {
	String label = Util.safeString(getModel().getLocalizedLabel());
	if (label.isEmpty()) {
	if (input == null) {
	if (descriptor != null) {
	return descriptor.getLabel();
	}
	} else {
	return Util.safeString(input.getName());
	}
	}
	return label;
	}

WorkbookEditorsHandler: Fix NPE in case name is null #1286

WorkbookEditorsHandler: Fix NPE in case name is null #1286

Conversation

sratz commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 • edited Loading

Test Results

laeubi Nov 7, 2023

Choose a reason for hiding this comment

sratz Nov 7, 2023

Choose a reason for hiding this comment

laeubi Nov 7, 2023

Choose a reason for hiding this comment

sratz Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

sratz commented Nov 8, 2023

laeubi left a comment

Choose a reason for hiding this comment

iloveeclipse left a comment • edited Loading

Choose a reason for hiding this comment

jukzi commented Nov 8, 2023

jukzi Nov 8, 2023

Choose a reason for hiding this comment

sratz Nov 8, 2023

Choose a reason for hiding this comment

jukzi Nov 8, 2023

Choose a reason for hiding this comment

sratz Nov 9, 2023

Choose a reason for hiding this comment

iloveeclipse commented Nov 9, 2023

jukzi commented Nov 9, 2023

sratz commented Nov 14, 2023

iloveeclipse left a comment

Choose a reason for hiding this comment

laeubi commented Nov 14, 2023

merks commented Nov 14, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

sratz Nov 7, 2023 •

edited

Loading

iloveeclipse left a comment •

edited

Loading