Bug 223912 - Enabling on save actions breaks Refactoring
Enabling on save actions breaks Refactoring
Status: VERIFIED FIXED
Product: editor
Classification: Unclassified
Component: -- Other --
7.3
PC Windows 7
: P1 (vote)
: 7.3
Assigned To: Miloslav Metelka
issues@editor
: 73_HR_FIX
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-17 10:12 UTC by Jiri Prox
Modified: 2013-01-23 09:45 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Proposed patch (957 bytes, patch)
2012-12-18 15:37 UTC, Ralph Ruijs
Details | Diff
Output of loggers when renaming Bean class to BeanRenamed (254.15 KB, text/plain)
2012-12-20 22:57 UTC, Miloslav Metelka
Details
Excerpt from the previous log showing problematic edits only (11.51 KB, text/plain)
2012-12-20 22:58 UTC, Miloslav Metelka
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Prox 2012-12-17 10:12:57 UTC
The code is broken when performing rename class and format on save is enabled

Steps to reproduce:
1) enable Reformat and remove trailing spaces for modified lines 
2) open sample project : http://wiki.netbeans.org/wiki/images/2/2d/Default_TS_60_Refactoring.zip
3) rename class Bean to something else

-> the code of other class is broken (invalid spaces are added in the middle of identifier)


Product Version: NetBeans IDE Dev (Build 201212120001)
Java: 1.7.0_10; Java HotSpot(TM) 64-Bit Server VM 23.6-b04
Runtime: Java(TM) SE Runtime Environment 1.7.0_10-b18
System: Windows 7 version 6.1 running on amd64; Cp1250; en_US (nb)
User directory: C:\Users\jprox\AppData\Roaming\NetBeans\dev
Cache directory: C:\Users\jprox\AppData\Local\NetBeans\Cache\dev

reproducible also on linux
Comment 1 Ralph Ruijs 2012-12-17 11:22:55 UTC
Apart from the reported problem with rename, other refactorings are affected as well. If save actions are enabled, undo is broken for all refactorings.
Comment 2 Miloslav Metelka 2012-12-18 13:57:58 UTC
In today's dev build the provided steps appear to work properly for me on Ubuntu 12.04. Dusan was doing some fixes to java's reformat-on-save recently which could possibly have effect on this.
I've also undone the rename refactoring successfuly.
If there are any more failing usecases please provide the steps. Thanks.
Comment 3 Ralph Ruijs 2012-12-18 14:12:41 UTC
(In reply to comment #2)
> In today's dev build the provided steps appear to work properly for me on
> Ubuntu 12.04. Dusan was doing some fixes to java's reformat-on-save recently
> which could possibly have effect on this.
> I've also undone the rename refactoring successfuly.
> If there are any more failing usecases please provide the steps. Thanks.

Thanks for looking into this. I will rebuild and try again.
Comment 4 Ralph Ruijs 2012-12-18 14:51:04 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > In today's dev build the provided steps appear to work properly for me on
> > Ubuntu 12.04. Dusan was doing some fixes to java's reformat-on-save recently
> > which could possibly have effect on this.
> > I've also undone the rename refactoring successfuly.
> > If there are any more failing usecases please provide the steps. Thanks.
> 
> Thanks for looking into this. I will rebuild and try again.

Jirka's test case still fails for me. And the following steps show the undo problem:

1) enable Reformat and remove trailing spaces for modified lines 
2) open sample project :
http://wiki.netbeans.org/wiki/images/2/2d/Default_TS_60_Refactoring.zip
3) rename method Bean#abc to something else
4) undo


Product Version: NetBeans IDE Dev (Build 20121218-3ad0bc06994c)
Java: 1.7.0_07; OpenJDK 64-Bit Server VM 23.2-b09
Runtime: OpenJDK Runtime Environment 1.7.0_07-b30
System: Linux version 3.5.7-gentoo running on amd64; UTF-8; en_US (nb)
Comment 5 Ralph Ruijs 2012-12-18 15:37:42 UTC
Created attachment 129514 [details]
Proposed patch

Refactoring saves all its changes in a single transaction. I think the onsavetasks should not be run, when the UndoManager is still in progress.
Comment 6 Miloslav Metelka 2012-12-19 11:22:24 UTC
Sorry, but the patch would not fix the problem (inProgress flag is always true for UndoRedoManager, it's only useful for CompoundEdit where it becomes false after calling end() which means that after that the compound edit cannot absorb additional edits and it can only be fully undone or redone).

Regarding reproducing of the problem modification of Bean#abc does not work for me but I can reproduce the problem when renaming Bean to BeanRenamed then everything renames ok except a line

        Bean b=new BeanB();

in BeansD class which renames to

        BeanRe n amed b=new BeanB();    }


I'm almost sure that the garbled name is result of a wrong formatting during save actions since if
1) I open the BeansD after the refactoring
2) Scroll to "BeanRe n amed" line
3) Perform Undo
4) Text changes to "BeanRenamed" first (this corresponds to undoing of the save actions)
5) Dialog asking whether perform Undo of refactoring is shown.
6) When replying Yes the undoing fails saying that some files were modified (Probably undoing of save actions have turned the modified flag of the files - I'll check that and possibly fix that).
I'll improve some debugs in BaseDocument and add more info then.
Comment 7 Miloslav Metelka 2012-12-20 22:54:34 UTC
I've improved logging of inserts/removals in BaseDocument that now show a context around a particular text edit:
http://hg.netbeans.org/jet-main/rev/00e0ff245cc1

The logging can be turned on by 

-J-Dorg.netbeans.editor.BaseDocument.level=FINE
-J-Dnetbeans.debug.editor.document.stack=true

I have also turned on (for loading/saving of the files):

-J-Dorg.openide.text.CloneableEditorSupport.level=FINE

I'll attach an output with loggers turned on and a short excerpt showing problematic edits only.
Comment 8 Miloslav Metelka 2012-12-20 22:57:39 UTC
Created attachment 129600 [details]
Output of loggers when renaming Bean class to BeanRenamed
Comment 9 Miloslav Metelka 2012-12-20 22:58:29 UTC
Created attachment 129601 [details]
Excerpt from the previous log showing problematic edits only
Comment 10 Miloslav Metelka 2012-12-20 23:04:12 UTC
At this point I reassign to Dusan to possibly fix the formatter to not insert the spaces at wrong positions.
 I'm still wondering whether any additional fix would be necessary for refactoring undo part because generally the refactoring undo seems to be working fine in other cases even with reformat on save turned on so I'll wait for Dusan's fix first.
Comment 11 Quality Engineering 2012-12-22 02:28:05 UTC
Integrated into 'main-golden', will be available in build *201212220001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/00e0ff245cc1
User: Miloslav Metelka <mmetelka@netbeans.org>
Log: #223912 - Enabling on save actions breaks Refactoring - improved logging of inserts/removals in BaseDocument.
Comment 12 Dusan Balek 2013-01-02 14:32:06 UTC
Problem with reformatting mentioned by Mila fixed.

http://hg.netbeans.org/jet-main/rev/f3ef8b1714e2

However, since the problem with Undo persists, reassigning back to Mila.
Comment 13 Quality Engineering 2013-01-03 02:39:07 UTC
Integrated into 'main-golden', will be available in build *201301030001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/f3ef8b1714e2
User: Dusan Balek <dbalek@netbeans.org>
Log: Issue #223912 - reformatting part fixed.
Comment 14 Miloslav Metelka 2013-01-08 16:29:26 UTC
Ralph, after Dusan's fix the only problem that I see now is refactoring undo.
If I Ctrl+R "Bean.java" in Projects explorer the Bean gets renamed to BeanRenamed. If I then do Ctrl+Z the Undo Refactoring dialog asks for confirmation. When confirmed it fails with "Following files were modified" with a number of files. In debugger BackupFacility2.checkCheckSum() the doc != null and editor != null but editor.isModified() is false so NbDocument.getEditToBeUndoneOfType() is not called at all (originally I thought that there will some sort of inconsistency regarding this).
So the code continues and it ends up on return in

                    byte[] ts = getMD5(getInputStream(backup.path));
                    if (!Arrays.equals(backup.checkSum, ts)) {
                        LOG.fine("MD5 check failed");
                        return backup.path.toURL().getPath();
                    }

so the MD5 differs. At this point I think it's no longer an editor problem so I reassign back to you for evaluation. Thanks.
Comment 15 Ralph Ruijs 2013-01-09 09:25:55 UTC
(In reply to comment #14)
> So the code continues and it ends up on return in
> 
>                     byte[] ts = getMD5(getInputStream(backup.path));
>                     if (!Arrays.equals(backup.checkSum, ts)) {
>                         LOG.fine("MD5 check failed");
>                         return backup.path.toURL().getPath();
>                     }
> 
> so the MD5 differs. At this point I think it's no longer an editor problem so I
> reassign back to you for evaluation. Thanks.

The MD5 is different, because the document is changed after refactoring has recorded the checksum. The problem is still the same, there is no point in running the onsave actions while refactoring.

I do think this is a problem in editor, because the onsave tasks are run regardless of context and there is no way to stop them from running.
Comment 16 Jan Lahoda 2013-01-11 11:19:22 UTC
This disables the OnSaveTasks while the refactoring is being performed, and even more importantly, while the refactoring is being undone:
http://hg.netbeans.org/jet-main/rev/4130b9ad2186

For the record, I think that the on-save-tasks must not be performed while undoing the refactoring (because if the original source code did not adhere to the on-save-tasks, when undo will undo the changes and then save the file the on-save-tasks would modify it, leading to code that is not the same as was the original code, which I think is wrong). For the refactoring apply part (when the refactoring is being done), I incline to not running the tasks, not only because of the refactoring preview not showing the changes (as Ralph pointed out), but also because it feels strange to (e.g.) reformat the whole project because of a trivial rename. Internally, we probably don't want to guarantee that refactoring commits the changes through Documents.

A problem that remains even after the above patch: when the refactoring is called from the editor, the one editor remains modified after the refactoring undo finishes (which makes some sense, and worked like this in NetBeans 7.2 as well). But, as a consequence, the OnSaveTasks are run when the user saves the file - i.e. when the undo is called from the editor, it is virtually impossible to get to the original state, unless the original state adhered to the OnSaveTasks. When the action is called from e.g. Projects tab, everything is OK.
Comment 17 Jan Lahoda 2013-01-11 17:30:20 UTC
Jirka, could you please verify? Thanks.
Comment 18 Jiri Prox 2013-01-11 22:54:22 UTC
the fix seems to be ok
verified in trunk
Comment 19 Quality Engineering 2013-01-12 02:52:59 UTC
Integrated into 'main-golden', will be available in build *201301120001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/4130b9ad2186
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #223912: disabling OnSaveTasks during refactoring perform and undo
Comment 20 Ralph Ruijs 2013-01-14 10:52:16 UTC
(In reply to comment #16)
> This disables the OnSaveTasks while the refactoring is being performed, and
> even more importantly, while the refactoring is being undone:
> http://hg.netbeans.org/jet-main/rev/4130b9ad2186

The change looks good to me, thanks !
Comment 21 Jan Lahoda 2013-01-15 09:40:28 UTC
release73:
http://hg.netbeans.org/releases/rev/f36da50a8ee4
Comment 22 Quality Engineering 2013-01-16 00:01:07 UTC
Integrated into 'releases', will be available in build *201301152100* or newer. Wait for official and publicly available build.
Changeset: http://hg.netbeans.org/releases/rev/f36da50a8ee4
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #223912: disabling OnSaveTasks during refactoring perform and undo


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo