Skip to content
This repository

Undo not working #86

Closed
nhajratw opened this Issue June 22, 2012 · 26 comments

8 participants

Nayan Hajratwala keforbes umphy tnicolson Adelar da Silva Queiróz eustachione ahairyass Brad
Nayan Hajratwala

neither 'u' or ':undo' appear to be working.

I'm using the latest unstable.

keforbes
Owner
Nayan Hajratwala

so, undo seems to be working in fine in Java files, but not in XML files pretty consistently.

umphy
umphy commented June 26, 2012

Hi guys, I've managed to reproduce this elusive issue :) What I did to reproduce this was:

  1. open up a java file to edit
  2. refactor > rename the file via package explorer

After the refactor, then the undo stops working. At first I thought it might be an unnamed clipboard bug, but it's not. It also happens with the default clipboard.

keforbes
Owner

Well done. That definitely reproduces the issue and throws a stack trace:

!MESSAGE could not set mark
!STACK 0
org.eclipse.jface.text.BadPositionCategoryException
    at org.eclipse.jface.text.AbstractDocument.addPosition(AbstractDocument.java:362)
    at org.eclipse.core.internal.filebuffers.SynchronizableDocument.addPosition(SynchronizableDocument.java:236)
    at net.sourceforge.vrapper.eclipse.platform.EclipseCursorAndSelection.setMark(EclipseCursorAndSelection.java:249)
    at net.sourceforge.vrapper.vim.modes.CommandBasedMode.executeCommand(CommandBasedMode.java:230)
    at net.sourceforge.vrapper.vim.modes.CommandBasedMode.handleKey(CommandBasedMode.java:254)

Unfortunately, it doesn't help me solve the issue. That method is throwing a stack trace but we catch it and log it. It doesn't look like it should be leaving Vrapper in a bad state. I'll keep playing with it though. At least this gives me a place to focus my efforts. Thanks!

keforbes
Owner

I'm not sure if this is the only way to reproduce the issue. That bothers me because I might be able to fix this specific problem but it wouldn't be a general solution. I'm still no closer to figuring out what's actually going on here.

This specific issue is that refactoring a filename creates and opens a new file for editing without firing any of the partOpened or partActivated events. This means Vrapper is never told to re-create the listeners it had set on the old filename's editor. So I might be able to find a way to forcibly re-apply the listeners during a refactor operation but that solution definitely wouldn't be accessed in any other scenario. I doubt nhajratw was renaming each of his XML files and that's what led him to believe undo was broken for all XML files. There must be some other scenario that exhibits the same behavior.

By the way, I tried avoiding the stack trace and it didn't fix the undo operation. So the stack trace has nothing to do with the root problem. The stack trace was basically telling me that the listener was never defined, which means everything was already in a bad state before this method was invoked.

umphy
umphy commented June 26, 2012
Nayan Hajratwala

it doesn't seem to be related to renaming for me...

keforbes
Owner

I have a theory. I hit the issue mostly on XML files that start up in some sort of Design view or other structured view. If I have to change tabs from "Design" to "Source" then I typically see the problem. For example, opening up a pom.xml file for Maven doesn't start me out in the XML view immediately so I have to move to the source view. I consistently hit this issue in pom files. I wonder if we fail to setup listeners when the initial view of a file is not a text view.

nhajratw, does this make sense given the behavior you're seeing? Or does your XML editor jump straight into an XML view?

keforbes
Owner

nhajratw, could you try something for me? I have a potential fix but I'm not sure if it's worth checking in yet. This handles the scenario for renaming a file but I'm not sure if it will fix your XML issue. Basically, I re-apply all listeners any time an editor gains focus. I also remove all listeners any time an editor loses focus. This means the listeners will be re-applied when you click into an editor regardless of its name. However, this also means I constantly remove and re-apply listeners as you click through tabs, which may not be ideal.

If your issue is related to losing listeners, I think this should fix your problem. If your issue is not related to listeners, this will do nothing for you and I'll back out the change because I don't want to take the performance hit. That's why I haven't checked in the code yet, I don't want to constantly mess with listeners if it isn't needed.

This code is beyond unstable, it's really-unstable. So I've created a new update site which you can use to pull down the changes and it won't affect anyone else using unstable. Please add this update site to your list and perform an update. Let me know how it goes.

http://vrapper.sourceforge.net/update-site/really-unstable

Nayan Hajratwala

I'll try this sometime this week -- fyi, it's not a problem with xml editor views... Thanks!

tnicolson

I've also found that the search (/) function stops working at the same time as undo. As you type, seemingly random and different (but the correct number of) characters are highlighted until you type enough characters where the "match" fails.

I'm using vrapper within the C++ editor, but I haven't been able to identify the cause.

keforbes
Owner

Quick update on my progress for this annoying defect. I can consistently reproduce it with a pom.xml file for Maven. I don't know if it's the same root cause as when it breaks in a Java file but I haven't found a way to reliably reproduce the issue in Java so I've been sticking to the pom.xml file.

When a file is first opened, Vrapper creates an instance of EclipseHistoryService with an IUndoManager as one of the arguments. When the argument is passed into the constructor, the IUndoManager instance has a fTextViewer and fUndoManager defined. When I perform a change to the pom.xml then hit 'u' to undo, that instance of IUndoManager in EclipseHistoryService now has both fTextViewer and fUndoManager set to null. So the undo call tries to call IUndoManager's undo() method, but since fUndoManager is null, the operation has no effect. I have no idea how or why those arguments suddenly became null.

One more difference. When I open a Java file, the IUndoManager is implemented by a TextViewerUndoManager instance. When I open a pom.xml file, the IUndoManager is implemented by a StructuredTextViewerUndoManager instance. I have no idea if that's relevant.

That's the extent of my knowledge so far. I'm still no closer to finding a fix.

tnicolson

It would appear that the breakage of undo and search that I'm experiencing within the C++ editor are actually related to folding. If I disable folding, I never see this problem.

keforbes
Owner

Well that's interesting. And unexpected. What happens if you enable folding but expand all folds? Do you still see the issue? Or does folding actually have to be disabled to workaround the problem?

tnicolson

I never really use folding, but the default configuration collapses the file header. I enabled folding again but made sure everything was expanded by default. In this configuration I encountered the problem again.

Sadly I have now encountered the issue with folding disabled (possibly after refactoring), but it definitely appears more stable when folding is disabled (it seemed to be happening to every buffer within half an hour of opening). I'll continue experimenting...

keforbes
Owner
Adelar da Silva Queiróz

Is it possible to do a relation between vrapper undo and Eclipse undo? May be a solution I think.

keforbes
Owner

See my long comment from 6 months ago that starts with "Quick update on my progress". Vrapper is already calling Eclipse's UndoManager directly. The problem is that the instance we were using somehow gets its private properties to turn null so calling undo has no effect. I don't know when or why Eclipse sets some of its private variables to null after I initialize it.

Adelar da Silva Queiróz

I will try to figure out something about how to solve this problem.

keforbes
Owner

I would absolutely love it if you could figure this one out. Good luck! Let me know if I can help.

eustachione

I'm also affected by this issue. Hope you can find a fix.

ahairyass

I have problems with this all the time... for me it seems manifest as if the vrapper undo is using a different stack than the eclipse undo. So I often get myself in the situation where doing a vrapper undo undoes more than one edit.

Here is a recent stack trace from my error logs. I hope it helps.

eclipse.buildId=M20130204-1200
java.version=1.6.0_26
java.vendor=Oracle Corporation
BootLoader constants: OS=linux, ARCH=x86_64, WS=gtk, NL=en_US
Framework arguments:  -product org.eclipse.epp.package.jee.product
Command-line arguments:  -os linux -ws gtk -arch x86_64 -product org.eclipse.epp.package.jee.product

!ENTRY org.eclipse.text 4 2 2013-05-08 21:08:11.736
!MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.text".
!STACK 0
java.lang.IllegalArgumentException: Index out of bounds
    at org.eclipse.swt.SWT.error(SWT.java:4342)
    at org.eclipse.swt.SWT.error(SWT.java:4276)
    at org.eclipse.swt.SWT.error(SWT.java:4247)
    at org.eclipse.swt.graphics.TextLayout._getOffset(TextLayout.java:1201)
    at org.eclipse.swt.graphics.TextLayout.getPreviousOffset(TextLayout.java:1398)
    at org.eclipse.swt.custom.StyledText.getPointAtOffset(StyledText.java:5428)
    at org.eclipse.swt.custom.StyledText.getLocationAtOffset(StyledText.java:4331)
    at net.sourceforge.vrapper.eclipse.platform.EclipseCursorAndSelection$StickyColumnUpdater.caretMoved(EclipseCursorAndSelection.java:240)
    at org.eclipse.swt.custom.StyledTextListener.handleEvent(StyledTextListener.java:96)
    at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
    at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:1079)
    at org.eclipse.swt.custom.StyledText.setCaretOffset(StyledText.java:8489)
    at org.eclipse.swt.custom.StyledText.handleTextChanging(StyledText.java:6259)
    at org.eclipse.swt.custom.StyledText$6.textChanging(StyledText.java:5607)
    at org.eclipse.jface.text.DefaultDocumentAdapter.fireTextChanging(DefaultDocumentAdapter.java:392)
    at org.eclipse.jface.text.DefaultDocumentAdapter.documentAboutToBeChanged(DefaultDocumentAdapter.java:310)
    at org.eclipse.jface.text.AbstractDocument.fireDocumentAboutToBeChanged(AbstractDocument.java:656)
    at org.eclipse.jface.text.projection.ProjectionDocument.delayedFireDocumentAboutToBeChanged(ProjectionDocument.java:769)
    at org.eclipse.jface.text.projection.ProjectionDocument.masterDocumentAboutToBeChanged(ProjectionDocument.java:727)
    at org.eclipse.jface.text.projection.ProjectionDocumentManager.fireDocumentEvent(ProjectionDocumentManager.java:121)
    at org.eclipse.jface.text.projection.ProjectionDocumentManager.documentAboutToBeChanged(ProjectionDocumentManager.java:139)
    at org.eclipse.jface.text.AbstractDocument.fireDocumentAboutToBeChanged(AbstractDocument.java:665)
    at org.eclipse.jface.text.AbstractDocument.replace(AbstractDocument.java:1184)
    at org.eclipse.core.internal.filebuffers.SynchronizableDocument.replace(SynchronizableDocument.java:194)
    at org.eclipse.text.undo.DocumentUndoManager$UndoableTextChange.undoTextChange(DocumentUndoManager.java:144)
    at org.eclipse.text.undo.DocumentUndoManager$UndoableTextChange.undo(DocumentUndoManager.java:265)
    at org.eclipse.core.commands.operations.DefaultOperationHistory.doUndo(DefaultOperationHistory.java:417)
    at org.eclipse.core.commands.operations.DefaultOperationHistory.undo(DefaultOperationHistory.java:1282)
    at org.eclipse.text.undo.DocumentUndoManager.undo(DocumentUndoManager.java:861)
    at org.eclipse.jface.text.TextViewerUndoManager.undo(TextViewerUndoManager.java:387)
    at net.sourceforge.vrapper.eclipse.platform.EclipseHistoryService.undo(EclipseHistoryService.java:71)
    at net.sourceforge.vrapper.vim.commands.UndoCommand.execute(UndoCommand.java:13)
    at net.sourceforge.vrapper.vim.modes.CommandBasedMode.executeCommand(CommandBasedMode.java:224)
    at net.sourceforge.vrapper.vim.modes.CommandBasedMode.handleKey(CommandBasedMode.java:258)
    at net.sourceforge.vrapper.vim.DefaultEditorAdaptor.handleKey0(DefaultEditorAdaptor.java:457)
    at net.sourceforge.vrapper.vim.DefaultEditorAdaptor.handleKeyOffRecord(DefaultEditorAdaptor.java:286)
    at net.sourceforge.vrapper.vim.DefaultEditorAdaptor.handleKey(DefaultEditorAdaptor.java:281)
    at net.sourceforge.vrapper.eclipse.interceptor.VimInputInterceptorFactory$VimInputInterceptor.verifyKey(VimInputInterceptorFactory.java:118)
    at org.eclipse.jface.text.TextViewer$VerifyKeyListenersManager.verifyKey(TextViewer.java:489)
    at org.eclipse.swt.custom.StyledTextListener.handleEvent(StyledTextListener.java:66)
    at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
    at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:1079)
    at org.eclipse.swt.custom.StyledText.handleKeyDown(StyledText.java:5932)
    at org.eclipse.swt.custom.StyledText$7.handleEvent(StyledText.java:5629)
    at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
    at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1300)
    at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1286)
    at org.eclipse.swt.widgets.Widget.sendIMKeyEvent(Widget.java:1362)
    at org.eclipse.swt.widgets.Control.gtk_commit(Control.java:2873)
    at org.eclipse.swt.widgets.Canvas.gtk_commit(Canvas.java:160)
    at org.eclipse.swt.widgets.Widget.windowProc(Widget.java:1763)
    at org.eclipse.swt.widgets.Control.windowProc(Control.java:5116)
    at org.eclipse.swt.widgets.Display.windowProc(Display.java:4377)
    at org.eclipse.swt.internal.gtk.OS._gtk_im_context_filter_keypress(Native Method)
    at org.eclipse.swt.internal.gtk.OS.gtk_im_context_filter_keypress(OS.java:7805)
    at org.eclipse.swt.widgets.Control.filterKey(Control.java:2282)
    at org.eclipse.swt.widgets.Control.gtk_key_press_event(Control.java:3043)
    at org.eclipse.swt.widgets.Composite.gtk_key_press_event(Composite.java:741)
    at org.eclipse.swt.widgets.Widget.windowProc(Widget.java:1773)
    at org.eclipse.swt.widgets.Control.windowProc(Control.java:5116)
    at org.eclipse.swt.widgets.Display.windowProc(Display.java:4377)
    at org.eclipse.swt.internal.gtk.OS._gtk_main_do_event(Native Method)
    at org.eclipse.swt.internal.gtk.OS.gtk_main_do_event(OS.java:8317)
    at org.eclipse.swt.widgets.Display.eventProc(Display.java:1193)
    at org.eclipse.swt.internal.gtk.OS._g_main_context_iteration(Native Method)
    at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3184)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$9.run(PartRenderingEngine.java:1053)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
    at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1086)
    at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:86)
    at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:588)
    at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
    at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:543)
    at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
    at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:130)
    at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
    at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:353)
    at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:180)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:629)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:585)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1439)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1414)
ahairyass

Oops... I should have mentioned that stack was generated using Juno SR2 and vrapper v0.31.20130421

keforbes keforbes closed this in d515c9e May 10, 2013
keforbes
Owner

Thanks for the stack trace. I'm not sure if it's the same issue everyone else was seeing but it was simple enough to avoid. After the undo happened we were trying to update the cursor position to the stickyColumn. In your stack trace that was throwing an IllegalArgumentException, I'm assuming because the desired column no longer existed after the undo. So, I now catch IllegalArgumentException and fallback to the first column of the line. This might not be the ideal behavior but hopefully the operation will complete now.

Note that I wasn't able to reproduce this stack trace so I'll need you to install the latest unstable build and test it out. I should have the new build (0.31.20130510) up soon.

UPDATE: Done. Unstable update site is updated. Let me know if this didn't work and I'll re-open the issue.

ahairyass

Thanks! Just installed the fix. I'll let you know if the problem recurs next week as I'm using this at work daily.

Brad

Just updated after finding this issue and it no longer undoes giant chunks, just small pieces that can be redone with my U. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.