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.

Bug 28212 - The AbstractFileSystem.Info.lock is NOT called for read-only files. (was: ALL: Auto editing feature does not work.)
Summary: The AbstractFileSystem.Info.lock is NOT called for read-only files. (was: ALL...
Status: VERIFIED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Text (show other bugs)
Version: 3.x
Hardware: PC Windows ME/2000
: P2 blocker (vote)
Assignee: Peter Zavadsky
URL:
Keywords: API
Depends on:
Blocks:
 
Reported: 2002-10-23 11:45 UTC by Jiri Kovalsky
Modified: 2008-12-22 17:38 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Kovalsky 2002-10-23 11:45:29 UTC
Development build #200210230100 of NetBeans 4.0
Windows 2000 with FCS build #21 of JDK 1.4.1

Description:
============
I can't tell since which build this occurs but
it's fact that auto-editing feature does not work.
No question pops up when trying to alter a
read-only file.

Steps to reproduce:
===================
1. Invoke "Versioning|Mount Version
Control|Generic VCS".
2. Select "Empty (Windows NT/2000)" profile, push
"Next >" and then "Edit Commands..." buttons.
3. Expand "Empty" folder and select "Lock" command.
4. Change its "Name" property to "EDIT", push "OK"
and "Finish" buttons.
5. Invoke "Customize" on the filesystem, switch to
"Advanced" tab, set "Really ?" as "Message" and
push "Close"
6. Create new file under the filesystem and turn
on its read-only attribute externally.
7. Open that file into editor and push Enter.
8. No "Really ?" question pops up.
Comment 1 Martin Entlicher 2002-10-23 14:49:26 UTC
This is a regression. I'm going to explore why it stopped working...
Comment 2 Martin Entlicher 2002-10-23 15:12:55 UTC
Probably some "optimalization" was made??
Comment 3 Peter Zavadsky 2002-10-24 12:51:29 UTC
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.
Comment 4 Martin Entlicher 2002-10-24 13:37:44 UTC
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.
Comment 5 Jiri Kovalsky 2002-10-24 13:44:20 UTC
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 ...
Comment 6 Peter Zavadsky 2002-10-25 10:09:18 UTC
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.
Comment 7 Martin Entlicher 2002-10-25 11:23:56 UTC
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 ??)).
Comment 8 Martin Entlicher 2002-10-25 11:25:16 UTC
Reassigning to Yarda to make the architectural decission.
Comment 9 Jaroslav Tulach 2002-10-29 14:39:56 UTC
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.
Comment 10 Martin Entlicher 2002-10-29 15:12:39 UTC
> 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?
Comment 11 Jaroslav Tulach 2002-10-31 12:06:13 UTC
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.
Comment 12 Jaroslav Tulach 2002-10-31 12:07:07 UTC
Peter please make the change in CES.
Comment 13 Peter Zavadsky 2002-11-01 09:31:11 UTC
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.

Comment 14 Jaroslav Tulach 2002-11-01 09:36:16 UTC
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.
Comment 15 Peter Zavadsky 2002-11-01 10:25:34 UTC
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.


Comment 16 Jaroslav Tulach 2002-11-01 13:21:49 UTC
"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?
Comment 17 Peter Zavadsky 2002-11-01 14:38:03 UTC
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.

Comment 18 Peter Zavadsky 2002-11-04 13:03:52 UTC
OK, even I personaly don't agree with such a contract, I'm going to
put it in.
Comment 19 Peter Zavadsky 2002-11-04 13:12:02 UTC
Fixed in [trunk]

Now the takeLock method is called from Env.markModified like it was
before.

openide/../text/DataEditorSupport.java 1.21
Comment 20 Jesse Glick 2002-11-04 14:26:52 UTC
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.
Comment 21 Peter Zavadsky 2002-11-05 10:17:28 UTC
For sure, such javadoc should help. I'll put it in.
Comment 22 Peter Zavadsky 2002-11-05 12:41:46 UTC
I applied the Jesse's suggestion:

openide/../filesystems/FileObject.java 1.68
          /text/DataEditorSupport.java 1.22
Comment 23 Jiri Kovalsky 2003-07-11 13:50:50 UTC
Cool, works fine in RC1 build #200307092351 of NetBeans 3.5.1.