This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | The AbstractFileSystem.Info.lock is NOT called for read-only files. (was: ALL: Auto editing feature does not work.) | ||
---|---|---|---|
Product: | platform | Reporter: | Jiri Kovalsky <jkovalsky> |
Component: | Text | Assignee: | Peter Zavadsky <pzavadsky> |
Status: | VERIFIED FIXED | ||
Severity: | blocker | CC: | jglick, jtulach, mentlicher |
Priority: | P2 | Keywords: | API |
Version: | 3.x | ||
Hardware: | PC | ||
OS: | Windows ME/2000 | ||
Issue Type: | DEFECT | Exception Reporter: |
Description
Jiri Kovalsky
2002-10-23 11:45:29 UTC
This is a regression. I'm going to explore why it stopped working... Probably some "optimalization" was made?? Sorry but I need to know what is the problem with text package. I tried it with Empty Unix profile. No "Really?" message popped out, that's true. But there were also reverted the typed chars whats the editors job. Please specify what is wrong. One more question. Why this really strange thing is marked as P2? I would mark it P5. I had big problems even repeat the steps. I guess no user could do something like that -> changing property name from LOCK to EDIT, thus activating some components in tab. It's P2, because it's a regression in functionality. The problem is, that when the user writes anything to the editor, AbstractFileSystem.Info.lock() should be called and it's IOException should be taken into account. This does not work anymore for read-only files. Here is the stack trace, that is called for RW files: at org.netbeans.modules.vcscore.VcsFileSystem.lock(VcsFileSystem.java:3777) at org.openide.filesystems.AbstractFileObject.lock(AbstractFileObject.java:210) at org.openide.loaders.MultiDataObject$Entry.takeLock(MultiDataObject.java:935) at org.openide.text.EditorSupport$EntryEnv.takeLock(EditorSupport.java:820) at org.openide.text.DataEditorSupport$Env.markModified(DataEditorSupport.java:403) at org.openide.text.CloneableEditorSupport.notifyModified(CloneableEditorSupport.java:925) at org.openide.text.EditorSupport$Del.superNotifyModified(EditorSupport.java:630) at org.openide.text.EditorSupport.notifyModified(EditorSupport.java:445) at org.openide.text.EditorSupport$Del.notifyModified(EditorSupport.java:618) at org.openide.text.CloneableEditorSupport$Listener.insertUpdate(CloneableEditorSupport.java:1512) at javax.swing.text.AbstractDocument.fireInsertUpdate(AbstractDocument.java:180) at org.netbeans.editor.BaseDocument.fireInsertUpdate(BaseDocument.java:902) at org.netbeans.editor.BaseDocument.insertString(BaseDocument.java:571) at org.netbeans.editor.BaseKit$DefaultKeyTypedAction.actionPerformed(BaseKit.java:783) at org.netbeans.editor.ext.ExtKit$ExtDefaultKeyTypedAction.actionPerformed(ExtKit.java:786) at org.netbeans.editor.BaseAction.actionPerformed(BaseAction.java:137) at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1384) at javax.swing.JComponent.processKeyBinding(JComponent.java:2078) at javax.swing.JComponent.processKeyBindings(JComponent.java:2104) at javax.swing.JComponent.processKeyEvent(JComponent.java:2050) at javax.swing.JEditorPane.processKeyEvent(JEditorPane.java:1159) at java.awt.Component.processEvent(Component.java:3553) at java.awt.Container.processEvent(Container.java:1164) at java.awt.Component.dispatchEventImpl(Component.java:2593) at java.awt.Container.dispatchEventImpl(Container.java:1213) at java.awt.Component.dispatchEvent(Component.java:2497) at java.awt.LightweightDispatcher.processKeyEvent(Container.java:2155) at java.awt.LightweightDispatcher.dispatchEvent(Container.java:2135) at java.awt.Container.dispatchEventImpl(Container.java:1200) at java.awt.Window.dispatchEventImpl(Window.java:914) at java.awt.Component.dispatchEvent(Component.java:2497) at java.awt.EventQueue.dispatchEvent(EventQueue.java:339) at java.awt.EventDispatchThread.pumpOneEventForHierarchy(EventDispatchThread.java:131) at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:98) at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93) at java.awt.EventDispatchThread.run(EventDispatchThread.java:85) The problem is in org.openide.text.DataEditorSupport$Env.markModified(). You've made the change on Jul 15. And I would like to explain why the procedure is so long. It is pretty easy to reproduce for VSS users but I am afraid most of people don't have this tool installed. Therefore I provided universal procedure ... Summary: There is no guarranty that editor support will take lock on read-only file. In fact it was wrong and it is not necessary. The file is not going to be changed so why to take the lock? The change fixed bigger problems than is the yours. Anyway your feature relied on such a flow. And that's incorrect. You cannot request on the impls to keep any obscure thing alive. There is no way I could be aware about it. If you need some functionality, you have to request some new API if needed. O.K., then I need a new API for this purpose ;-) I understand, that you can know, that if the file is read-only, it can not be modified. But you can not be sure, that if file is RW it can be modified. Thus you need to run lock() anyway. You use 2 methods for one purpose. Also when you do not call lock(), you effectively removed a possibility to revert the file from read-only state to read-write. Thus I need either: 1) specify in the openide docs, that FileObject.lock() MUST be called whenever there is an attempt to modify a file and it's IOException (or UserQuestionException) taken into account, OR 2) create a new API (something like FileObject.canModify(), that would return true/false (and probably also throw UserQuestionException ??)). Reassigning to Yarda to make the architectural decission. If the file can be modified, then it is not read only. Let the VCSFS not return true from isReadOnly if one can get output stream and write to the file. > If the file can be modified, then it is not read only. It *is* read-only. But it can be changed and *then* modified. > Let the VCSFS not return true from isReadOnly if one can get output > stream and write to the file. Do you mean, that I should lie???? You can not write to the file while it's read-only. The point is, that at some point the file can be (but does not have to be) changed from read-only mode to read-write. I don't think, that I should return isReadOnly() == false when the file *is* read-only. This can cause problems somewhere else. Some better idea? Two comments: The javadoc of isReadOnly describes that "the method tests whether a file object can be written to". So regardless of the actual state of the file on the disk, if it can be written (for example in some automatic mode), it is not read only. I have improved the javadoc http://www.netbeans.org/source/browse/openide/src/org/openide/filesystems/FileObject.java.diff?r1=1.64&r2=1.65 but please notice the line before my changes to say the same, so there is no change in the meaning of the method. So please update your filesystem to return false when in automatic mode. But for the non-automatic case (which throws UserQuestionException), we need the DataEditorSupport to really try to ask for the lock. I suggest it to ask everytime user does first modification to the document, but not report any exception if the file was read only. Peter please make the change in CES. I will resist a bit. Such a solution is just an ugly hack. Isn't it better to try to think some clean API solution? So we could avoid this kind of regressions next time. I believe the correct impl of markModified should never try to take the lock of file. It is enough for CES to know whether the file is read-only or not and behave accordingly, and take the lock, just at the moment it is going to save the changes. I don't like the method should be restricted this way. Such hacks makes the class fragile, hard to maintain, and it leads to problems like this one. Do you think it is the correct way? I don't. One more time passing to Yarda. UserQuestionException is the API that was designed to allow cooperation between (versioning) filesystems and editor. As far as I know it used to work fine, even was not covered by test. That is why, we did not catch the regression that was done in the CES. I agree that should not happen again. So I am asking the owner of this package, to rollback the regression, write a test to check proper behaviour of UserQuestionException API and then try to solve rollbacked commit in different way. One more time passing this to Yarda, to get his answer. I'm not interested in how to fix it since I can fix it easily, the way, there will be called takeLock in markModified in all cases.(I won't rollback fix of #24462 which was much worser bug). But I want to point out the "design technique" I don't agree with. Is that supposed to be an API contract between openapi and vcs that support have to always call takeLock in its markModified method and handle specially UserCancelException if thrown? I claim that that is not a design, just a hack. How could somebody looking at the code to know (in this case me) that the method call takeLock is "holy" one and has some other meaning in markModified than just taking the lock? This kind of "design" leads to unmaintainable, ugly and fragile code. I suppose if our goal is NB to be rock and solid, it can't contain these kind of hacks. "How could somebody looking at the code to know that the method call" One could not, because the contract is not well documented nor tested. That is a lesson we should learn from. Fix the documentation and next time be more careful. Is there anything else I can do for you? Well, it seems it doesn't have a sense. Of course you could do something, and that is design a clear API. I'm talking about that this case is bad design, just a hack. Contract saying that "markModified method has to always call method takeLock because when the UserCancelException is thrown from that one then it is done this and that" is a clear nonsense. Do you really want to have such sentences in javadoc? The method markModified doesn't need any lock, so why it should be restricted that way it tries to take it? Because there could be somewhere and somewhen some vcs FS? I agree with you just in that it is a lesson. But I think we are not learning the right way. OK, even I personaly don't agree with such a contract, I'm going to put it in. Fixed in [trunk] Now the takeLock method is called from Env.markModified like it was before. openide/../text/DataEditorSupport.java 1.21 I tend to agree with Peter here, something is still wrong. At a minimum, the Javadoc for FileObject.isReadOnly is inadequate. It says that if it returns false, then getOutputStream etc. should succeed. Fine. But it says nothing about when it returns true, implying (but not saying) that in such cases getOutputStream etc. should fail. In this case they probably will, but there is the further fact that takeLock will throw UserQuestionException and permit the file to be made writable. Therefore I don't think the newly reverted behavior of DES.Env.markModified is wrong, but it is depending on a part of the API which is not yet documented! (As Martin said, more or less.) Suggest that FO.isRO Javadoc contain some additional information about UQE and lock(). For example: ---%<--- It is a good idea to call this method before attempting to perform any operation on the FileObject that might throw an IOException simply because it is read-only. If isReadOnly returns true, the operation may be skipped, or the user notified that it cannot be done. <em>However</em> it is often desirable for the user to be able to continue the operation in case the filesystem supports making a file writable. In this case calling code should: <ol> <li>Call lock() and catch any exception thrown. <li>If no exception is thrown, proceed with the operation. <li>If a UserQuestionException is thrown, call confirmed() on it (asynchronously - do not block any important threads). If true, return to step 1. If false, exit. If an IOException is thrown, notify it and exit. <li>If another IOException is thrown, call isReadOnly(). If true, ignore the exception (it is expected). If false, notify it. In either case, exit. </ol> ---%<--- Does that make more sense? Then Javadoc for DES.Env.markModified would refer to FO.isReadOnly for a fuller explanation of its logic. For sure, such javadoc should help. I'll put it in. I applied the Jesse's suggestion: openide/../filesystems/FileObject.java 1.68 /text/DataEditorSupport.java 1.22 Cool, works fine in RC1 build #200307092351 of NetBeans 3.5.1. |