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

IllegalArgumentException: Group not found: diffLabel #1295

Closed
stephan-herrmann opened this issue Mar 26, 2024 · 14 comments · Fixed by #1319
Closed

IllegalArgumentException: Group not found: diffLabel #1295

stephan-herrmann opened this issue Mar 26, 2024 · 14 comments · Fixed by #1319
Labels
bug Something isn't working

Comments

@stephan-herrmann
Copy link
Contributor

I've been getting this IllegalArgumentException for a little while now. Today I found a corresponding stacktrace in the log.

This time I was in the "Encapsulate field... " refactoring, had inspected the preview of a particular class and then pressed back. This scenario reliable reproduces the bug for me.

There have been various other situations for the same problem, too.

eclipse.buildId=4.31.0.I20240229-0520
java.version=17.0.6
java.vendor=Eclipse Adoptium
BootLoader constants: OS=linux, ARCH=x86_64, WS=gtk, NL=en_US
Command-line arguments:  -os linux -ws gtk -arch x86_64 -data /home/eclipse/jdt-master/ws
java.lang.IllegalArgumentException: Group not found: diffLabel
	at org.eclipse.jface.action.ContributionManager.addToGroup(ContributionManager.java:130)
	at org.eclipse.jface.action.ContributionManager.appendToGroup(ContributionManager.java:140)
	at org.eclipse.compare.contentmergeviewer.TextMergeViewer.updateToolbarLabel(TextMergeViewer.java:5432)
	at org.eclipse.compare.contentmergeviewer.TextMergeViewer.update(TextMergeViewer.java:5414)
	at org.eclipse.compare.contentmergeviewer.TextMergeViewer.updateContent(TextMergeViewer.java:3063)
	at org.eclipse.compare.contentmergeviewer.ContentMergeViewer.internalRefresh(ContentMergeViewer.java:796)
	at org.eclipse.compare.contentmergeviewer.ContentMergeViewer.inputChanged(ContentMergeViewer.java:704)
	at org.eclipse.jface.viewers.ContentViewer.setInput(ContentViewer.java:282)
	at org.eclipse.jdt.internal.ui.compare.JavaMergeViewer.setInput(JavaMergeViewer.java:156)
	at org.eclipse.compare.CompareViewerSwitchingPane.setInput(CompareViewerSwitchingPane.java:268)
	at org.eclipse.ltk.internal.ui.refactoring.TextEditChangePreviewViewer.setInput(TextEditChangePreviewViewer.java:249)
	at org.eclipse.ltk.internal.ui.refactoring.TextEditChangePreviewViewer.setInput(TextEditChangePreviewViewer.java:208)
	at org.eclipse.ltk.internal.ui.refactoring.TextEditGroupNode.feedInput(TextEditGroupNode.java:84)
	at org.eclipse.ltk.internal.ui.refactoring.PreviewWizardPage.showPreview(PreviewWizardPage.java:635)
	at org.eclipse.ltk.internal.ui.refactoring.PreviewWizardPage.lambda$0(PreviewWizardPage.java:606)
	at org.eclipse.jface.viewers.Viewer$1.run(Viewer.java:151)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:174)
	at org.eclipse.jface.viewers.Viewer.fireSelectionChanged(Viewer.java:148)
	at org.eclipse.jface.viewers.StructuredViewer.updateSelection(StructuredViewer.java:2132)
	at org.eclipse.jface.viewers.ColumnViewer.updateSelection(ColumnViewer.java:1055)
	at org.eclipse.jface.viewers.StructuredViewer.setSelection(StructuredViewer.java:1664)
	at org.eclipse.jface.viewers.TreeViewer.setSelection(TreeViewer.java:1091)
	at org.eclipse.jface.viewers.Viewer.setSelection(Viewer.java:375)
	at org.eclipse.ltk.internal.ui.refactoring.ChangeElementTreeViewer.handleInvalidSelection(ChangeElementTreeViewer.java:113)
	at org.eclipse.jface.viewers.StructuredViewer.preservingSelection(StructuredViewer.java:1405)
	at org.eclipse.jface.viewers.TreeViewer.preservingSelection(TreeViewer.java:367)
	at org.eclipse.jface.viewers.StructuredViewer.preservingSelection(StructuredViewer.java:1353)
	at org.eclipse.jface.viewers.CheckboxTreeViewer.preservingSelection(CheckboxTreeViewer.java:407)
	at org.eclipse.jface.viewers.AbstractTreeViewer.inputChanged(AbstractTreeViewer.java:1589)
	at org.eclipse.ltk.internal.ui.refactoring.ChangeElementTreeViewer.inputChanged(ChangeElementTreeViewer.java:122)
	at org.eclipse.jface.viewers.ContentViewer.setInput(ContentViewer.java:282)
	at org.eclipse.jface.viewers.StructuredViewer.setInput(StructuredViewer.java:1636)
	at org.eclipse.ltk.internal.ui.refactoring.PreviewWizardPage.setTreeViewerInput(PreviewWizardPage.java:576)
	at org.eclipse.ltk.internal.ui.refactoring.PreviewWizardPage.setChange(PreviewWizardPage.java:329)
	at org.eclipse.ltk.ui.refactoring.RefactoringWizard.internalSetChange(RefactoringWizard.java:776)
	at org.eclipse.ltk.ui.refactoring.RefactoringWizard.computeUserInputSuccessorPage(RefactoringWizard.java:518)
	at org.eclipse.ltk.ui.refactoring.UserInputWizardPage.computeSuccessorPage(UserInputWizardPage.java:77)
	at org.eclipse.ltk.ui.refactoring.UserInputWizardPage.getNextPage(UserInputWizardPage.java:113)
	at org.eclipse.ltk.internal.ui.refactoring.RefactoringWizardDialog2.nextOrPreviewPressed(RefactoringWizardDialog2.java:494)
	at org.eclipse.ltk.internal.ui.refactoring.RefactoringWizardDialog2$1.widgetSelected(RefactoringWizardDialog2.java:695)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:252)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1529)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:5065)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4517)
	at org.eclipse.jface.window.Window.runEventLoop(Window.java:823)
	at org.eclipse.jface.window.Window.open(Window.java:799)
	at org.eclipse.ltk.ui.refactoring.RefactoringWizardOpenOperation.lambda$0(RefactoringWizardOpenOperation.java:190)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67)
	at org.eclipse.ltk.ui.refactoring.RefactoringWizardOpenOperation.run(RefactoringWizardOpenOperation.java:209)
	at org.eclipse.ltk.ui.refactoring.RefactoringWizardOpenOperation.run(RefactoringWizardOpenOperation.java:126)
	at org.eclipse.jdt.internal.ui.refactoring.actions.RefactoringStarter.activate(RefactoringStarter.java:41)
	at org.eclipse.jdt.internal.corext.refactoring.RefactoringExecutionStarter.startSelfEncapsulateRefactoring(RefactoringExecutionStarter.java:481)
	at org.eclipse.jdt.ui.actions.SelfEncapsulateFieldAction.run(SelfEncapsulateFieldAction.java:172)
	at org.eclipse.jdt.ui.actions.SelfEncapsulateFieldAction.run(SelfEncapsulateFieldAction.java:128)
	at org.eclipse.jdt.ui.actions.SelectionDispatchAction.dispatchRun(SelectionDispatchAction.java:278)
	at org.eclipse.jdt.ui.actions.SelectionDispatchAction.run(SelectionDispatchAction.java:252)
	at org.eclipse.jface.action.Action.runWithEvent(Action.java:474)
	at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:581)
	at org.eclipse.jface.action.ActionContributionItem.lambda$4(ActionContributionItem.java:415)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1529)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:5065)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4517)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1042)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:152)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:639)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:651)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:588)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1459)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1432)
@stephan-herrmann stephan-herrmann added the bug Something isn't working label Mar 26, 2024
@BeckerWdf
Copy link
Contributor

This seems to be a regression of #731
@lathapatil can you pls provide a fix for that?

@stephan-herrmann
Copy link
Contributor Author

Simpler way to reproduce: Team > Apply Patch using a patch with many conflicting changes.

  • Select one conflict (double click)
  • Compare editor appears
  • Select Exclude from the context menu of the selected change
  • Compare editor disappears, error dialog pops up.

This situation is more severe, because the compare editor is broken henceforth, and selecting other changes does not bring it back!

As I was about to lose an hour's work resolving many conflicts in a monster patch, I had to be inventive, and luckily switching from "Java Source Compare" to "Text Compare" gave me a working compare editor, from where I could also go back to "Java Source Compare".

@lathapatil
Copy link
Contributor

This seems to be a regression of eclipse-platform/eclipse.platform#731 @lathapatil can you pls provide a fix for that?

I am debugging this case. Currently I can understand that all contributions are empty for toolbar manager during this call !

@lathapatil
Copy link
Contributor

lathapatil commented Apr 5, 2024

The issue seems to be in setInput method of CompareViewerSwitchingPane.java where during this call a proper viewer for the given input is not found and a NullViewer is assigned . Toolbar is cleared out in the NullViewer constructor.

I still need to check on fixing part of this issue ! Any suggestions are welcome

@lathapatil
Copy link
Contributor

The file that needs fix is TextMergeViewer.java which is part of eclipse.platform
can anyone move this issue to eclipse-platform repository?

@merks merks transferred this issue from eclipse-platform/eclipse.platform.ui Apr 9, 2024
@iloveeclipse
Copy link
Member

Toolbar is cleared out in the NullViewer constructor.

I still need to check on fixing part of this issue ! Any suggestions are welcome

What about this:

diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java
index aab04a6..4ec4cf0 100644
--- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java
+++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java
@@ -546,6 +546,4 @@
 				// If the element falls within a diff, highlight that diff
-				if (diffPos.offset + diffPos.length >= p.offset && diff.getKind() != RangeDifference.NOCHANGE)
-					return diff;
 				// Otherwise, highlight the first diff after the elements position
-				if (diffPos.offset >= p.offset)
+				if ((diffPos.offset + diffPos.length >= p.offset && diff.getKind() != RangeDifference.NOCHANGE) || (diffPos.offset >= p.offset))
 					return diff;
@@ -2061,10 +2059,3 @@
 			fAncestorCanvas = new Canvas(composite, SWT.DOUBLE_BUFFERED);
-			fAncestorCanvas.addPaintListener(new PaintListener() {
-
-				@Override
-				public void paintControl(PaintEvent e) {
-					paintSides(e.gc, fAncestor, fAncestorCanvas, false);
-
-				}
-			});
+			fAncestorCanvas.addPaintListener(e -> paintSides(e.gc, fAncestor, fAncestorCanvas, false));
 			fAncestorCanvas.addMouseListener(
@@ -2097,10 +2088,3 @@
 			fLeftCanvas = new Canvas(composite, SWT.DOUBLE_BUFFERED);
-			fLeftCanvas.addPaintListener(new PaintListener() {
-
-				@Override
-				public void paintControl(PaintEvent e) {
-					paintSides(e.gc, fLeft, fLeftCanvas, false);
-
-				}
-			});
+			fLeftCanvas.addPaintListener(e -> paintSides(e.gc, fLeft, fLeftCanvas, false));
 			fLeftCanvas.addMouseListener(
@@ -2160,10 +2144,3 @@
 			fRightCanvas = new Canvas(composite, SWT.DOUBLE_BUFFERED);
-			fRightCanvas.addPaintListener(new PaintListener() {
-
-				@Override
-				public void paintControl(PaintEvent e) {
-					paintSides(e.gc, fRight, fRightCanvas, fSynchronizedScrolling);
-
-				}
-			});
+			fRightCanvas.addPaintListener(e -> paintSides(e.gc, fRight, fRightCanvas, fSynchronizedScrolling));
 			fRightCanvas.addMouseListener(
@@ -2193,10 +2170,6 @@
 		fBirdsEyeCanvas = new Canvas(composite, SWT.DOUBLE_BUFFERED);
-		fBirdsEyeCanvas.addPaintListener(new PaintListener() {
+		fBirdsEyeCanvas.addPaintListener(e -> {
+			updateVScrollBar(); // Update scroll bar here as initially viewport height is wrong
+			paintBirdsEyeView((Canvas) e.widget, e.gc);
 
-			@Override
-			public void paintControl(PaintEvent e) {
-				updateVScrollBar(); // Update scroll bar here as initially viewport height is wrong
-				paintBirdsEyeView((Canvas) e.widget, e.gc);
-
-			}
 		});
@@ -2279,6 +2252,3 @@
 				Diff diff = (Diff) iterator.next();
-				if (diff.isDeleted())
-					continue;
-
-				if (fShowCurrentOnly2 && !isCurrentDiff(diff))
+				if (diff.isDeleted() || (fShowCurrentOnly2 && !isCurrentDiff(diff)))
 					continue;
@@ -2323,6 +2293,3 @@
 				Diff diff = (Diff) iterator.next();
-				if (diff.isDeleted())
-					continue;
-
-				if (fShowCurrentOnly2 && !isCurrentDiff(diff))
+				if (diff.isDeleted() || (fShowCurrentOnly2 && !isCurrentDiff(diff)))
 					continue;
@@ -2482,10 +2449,3 @@
 
-			canvas.addPaintListener(new PaintListener() {
-
-				@Override
-				public void paintControl(PaintEvent e) {
-					paintCenter((Canvas) e.widget, e.gc);
-
-				}
-			});
+			canvas.addPaintListener(e -> paintCenter((Canvas) e.widget, e.gc));
 			new HoverResizer(canvas, HORIZONTAL);
@@ -3135,6 +3095,3 @@
 
-		if (!fHighlightRanges)
-			return false;
-
-		if (diff == null || diff.isToken())
+		if (!fHighlightRanges || diff == null || diff.isToken())
 			return false;
@@ -4004,7 +3961,3 @@
 	private void disposeCompareFilterActions(boolean updateActionBars) {
-		Iterator<ChangeCompareFilterPropertyAction> compareFilterActionsIterator = fCompareFilterActions
-				.iterator();
-		while (compareFilterActionsIterator.hasNext()) {
-			ChangeCompareFilterPropertyAction compareFilterAction = compareFilterActionsIterator
-					.next();
+		for (ChangeCompareFilterPropertyAction compareFilterAction : fCompareFilterActions) {
 			fLeft.removeTextAction(compareFilterAction);
@@ -4138,6 +4091,3 @@
 
-		if (fLeft == null || fRight == null) {
-			return;
-		}
-		if (fLeft.getSourceViewer().getDocument() == null || fRight.getSourceViewer().getDocument() == null) {
+		if (fLeft == null || fRight == null || fLeft.getSourceViewer().getDocument() == null || fRight.getSourceViewer().getDocument() == null) {
 			return;
@@ -4315,6 +4265,3 @@
 				Diff diff = (Diff) iterator.next();
-				if (diff.isDeleted())
-					continue;
-
-				if (fShowCurrentOnly2 && !isCurrentDiff(diff))
+				if (diff.isDeleted() || (fShowCurrentOnly2 && !isCurrentDiff(diff)))
 					continue;
@@ -4459,6 +4406,3 @@
 				Diff diff = iterator.next();
-				if (diff.isDeleted())
-					continue;
-
-				if (fShowCurrentOnly2 && !isCurrentDiff(diff))
+				if (diff.isDeleted() || (fShowCurrentOnly2 && !isCurrentDiff(diff)))
 					continue;
@@ -4502,5 +4446,3 @@
 
-		if (! fHighlightRanges)
-			return;
-		if (!fMerger.hasChanges())
+		if (! fHighlightRanges || !fMerger.hasChanges())
 			return;
@@ -4525,6 +4467,3 @@
 			Diff diff = (Diff) iterator.next();
-			if (diff.isDeleted())
-				continue;
-
-			if (fShowCurrentOnly && !isCurrentDiff(diff))
+			if (diff.isDeleted() || (fShowCurrentOnly && !isCurrentDiff(diff)))
 				continue;
@@ -5419,14 +5358,21 @@
 
+			boolean updateNeeded = false;
 			if (tbm.find(DIFF_COUNT_ID) != null) {
 				tbm.replaceItem(DIFF_COUNT_ID, labelContributionItem);
+				updateNeeded = true;
 			} else {
-				tbm.appendToGroup("diffLabel", labelContributionItem); //$NON-NLS-1$
-			}
-			fComposite.getDisplay().asyncExec(() -> {
-				// relayout in next tick
-				ToolBar control = tbm.getControl();
-				if (control != null && !control.isDisposed()) {
-					tbm.update(true);
+				if (tbm.find("diffLabel") != null) { //$NON-NLS-1$
+					tbm.appendToGroup("diffLabel", labelContributionItem); //$NON-NLS-1$
+					updateNeeded = true;
 				}
-			});
+			}
+			if (updateNeeded) {
+				fComposite.getDisplay().asyncExec(() -> {
+					// relayout in next tick
+					ToolBar control = tbm.getControl();
+					if (control != null && !control.isDisposed()) {
+						tbm.update(true);
+					}
+				});
+			}
 		}
@@ -5496,5 +5442,3 @@
 			return false;
-		if (diff == fCurrentDiff)
-			return true;
-		if (fCurrentDiff != null && fCurrentDiff.getParent() == diff)
+		if ((diff == fCurrentDiff) || (fCurrentDiff != null && fCurrentDiff.getParent() == diff))
 			return true;

Unfortunately steps to reproduce seem to be non trivial, if that would be easier way I could test if that patch works or not.

@iloveeclipse
Copy link
Member

Arrgh, this commit 5c071d3 added lot of save actions that make it impossible to produce a clean patch. @HeikoKlare : I just wanted to change few lines but extra actions make it impossible to keep only related changes. Can we please undo this and do not perform so much changes automatically?

@lathapatil : here is what I really wanted to propose:

diff --git a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java
index aab04a6..a884a8f 100644
--- a/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java
+++ b/team/bundles/org.eclipse.compare/compare/org/eclipse/compare/contentmergeviewer/TextMergeViewer.java
@@ -5419,14 +5419,21 @@
 
+			boolean updateNeeded = false;
 			if (tbm.find(DIFF_COUNT_ID) != null) {
 				tbm.replaceItem(DIFF_COUNT_ID, labelContributionItem);
+				updateNeeded = true;
 			} else {
-				tbm.appendToGroup("diffLabel", labelContributionItem); //$NON-NLS-1$
-			}
-			fComposite.getDisplay().asyncExec(() -> {
-				// relayout in next tick
-				ToolBar control = tbm.getControl();
-				if (control != null && !control.isDisposed()) {
-					tbm.update(true);
+				if (tbm.find("diffLabel") != null) { //$NON-NLS-1$
+					tbm.appendToGroup("diffLabel", labelContributionItem); //$NON-NLS-1$
+					updateNeeded = true;
 				}
-			});
+			}
+			if (updateNeeded) {
+				fComposite.getDisplay().asyncExec(() -> {
+					// relayout in next tick
+					ToolBar control = tbm.getControl();
+					if (control != null && !control.isDisposed()) {
+						tbm.update(true);
+					}
+				});
+			}
 		}

@HeikoKlare
Copy link
Contributor

I just wanted to change few lines but extra actions make it impossible to keep only related changes. Can we please undo this and do not perform so much changes automatically?

The unintended changes only seem to be related to combining if clauses (just like in #1282) and replacing anymous classes with lambdas. So if this is a recurring problem (which it seems to be), we should probably turn off specific autosave action(s). I would not be in favor of completely reverting the commit completely, as it streamlined all the different, inconsistent autosave configurations across the platform projects (and basically only applied the JDT default settings). This streamlining would completely be lost when reverting the commit, while a follow-up fix would be relatively easy.
I propose the following:

  • Disable the autosave action related to combining if clauses.
  • Keep the save action for replacing anonymous classes with lambdas.

Of course, the latter may produce changes in existing code, but they should be far more local than the if-clause changes (and they are reasonable in almost every case). Note that you can also easily revert the auto save actions by performing an undo and re-save after doing the save that applied the auto save actions.

What do you think, @iloveeclipse?

I will at least provide a PR disabling the if-clause-related save action soon.

@iloveeclipse
Copy link
Member

The unintended changes only seem to be related to combining if clauses (just like in #1282) and replacing anymous classes with lambdas. So if this is a recurring problem (which it seems to be), we should probably turn off specific autosave action(s)

Yes, please. If anyone wants to have a cleanup change, one can run whatever cleanups wanted. However it is impossible to create a single line change without searching & reverting related settings on the project.

@iloveeclipse
Copy link
Member

Another possibility would be to run all the wanted cleanups before enabling actions by default, so if something is changed, it is only in the new code.

@HeikoKlare
Copy link
Contributor

Another possibility would be to run all the wanted cleanups before enabling actions by default, so if something is changed, it is only in the new code.

Yes, that has also been discussed in #1001 and #1282. I just did not put any urgency on it as I have not noticed many PRs suffering from save actions applying unexpected changes.

If anyone wants to have a cleanup change, one can run whatever cleanups wanted. However it is impossible to create a single line change without searching & reverting related settings on the project.

Note that is is not about overall cleanups but about save actions. So it is basically about rules to follow when contributing new code. An essential problem is that save actions cannot apply to changed code but always apply to a whole file. I will now go through the rules and either apply an according cleanup making all code conform to the save action rule or remove the save action rule again, so that save actions on existing code will always be a no-op.

@lathapatil
Copy link
Contributor

@lathapatil : here is what I really wanted to propose:

Below empty and null checks fix the current issue . If the toolbar is empty there is no need to show the differences as well.
How does the proposed code helps to fix this issue ?

image

@iloveeclipse
Copy link
Member

How does the proposed code helps to fix this issue ?

Not sure whom you ask? If your patch fixes the problem (it seem to), please set a PR.

Please note, one should avoid the extra async call below (that updates toolbar) if no changes were made (like in my patch proposal)

@lathapatil
Copy link
Contributor

Not sure whom you ask? If your patch fixes the problem (it seem to), please set a PR.

@iloveeclipse Soon I will set a PR for the fix .

Please note, one should avoid the extra async call below (that updates toolbar) if no changes were made (like in my patch proposal)

Apologies, I didn't understand the code before because I read it as a whole instead of viewing it as a diff. I'll adjust my approach to prevent making async calls if no changes are made to the toolbar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants