Bugzilla – Full Text Bug Listing
| Summary: | [implementation][api] Editor setup and reuse | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Dani Megert <daniel_megert> |
| Component: | Text | Assignee: | Platform-Text-Inbox <platform-text-inbox> |
| Status: | CLOSED WONTFIX | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bob, david_williams, mark.melvin, thatnitind |
| Version: | 3.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | stalebug | ||
|
Description
Dani Megert
*** Bug 70797 has been marked as a duplicate of this bug. *** This comment is copied from 70797. This seems a monkey-see, monkey-do situation. I see your PresentationReconciler and how it sets up an ITextInputListener to listen for document changes and an IDocumentListener to listen for changes to documents. I can do that. One slightly annoying thing is that setDocument is called during createPartControl, so that it is not possible to set a listener to catch the initial documentChanged. I can work around this simply by getting the document after super.createPartControl returns and calling documentChanged directly, but it occurs to me this is yet another fragile hack depending on internal behavior which is not API and might change in any release. To which Daniel Megert replied: "Unfortunately the method which would fit your needs (initializeSourceViewer) for adding the listener is private and not protected." You mean I could override initializeSourceViewer (if it were protected) to sneak in before calling super, which is after the viewer and reconciliation listener are set up but before the document is set? Yes, but even if it were possible, it would be yet another dependence on internal behavior that, I would be quickly reminded were you ever to change it, is not API. The whole order-of-listener thing has proved troublesome in the 3.0 cycle, as so many have been changed around (e.g., for code assist). It is really not possible to write an editor without knowing the order in which text changes are reflected in the various associated data structures and services. Thanks Bob. It's good to add some more context here. I'm beginning to think the current design can't be made to work correctly. E.g., if an editor assigns a partitioner using setDocumentPartitioner(partitioner), it will interfere with any other editor that does the same, e.g., my editor vs. the Ant editor. OTOH, if the partitioner is assigned using setDocumentPartitioner(string, partitioner), it doesn't interfere with other editors, but that partitioner is not used for coloring! (The latter also has the undesirable side effect that multiple partitioners get called for every document change.) It seems from here like these changes were designed without considering the full list of requirements, which include: - Multiple candidate editors for given extension. - Candidates can be added/removed dynamically by user. - Partitioning (scanning) is performance-critical. - Active editor must control coloring. etc. I must admit, I (too?) don't even understand the rationale or use case the "setup participants". Seems to only invite conflicts. I've opened other bugs on the ant editor being "over defined" for every xml file (but haven't checked their status lately). But even if that wasnt' the case, still don't see how two xml editors could ever be on the same system, and both be "participating" in any meaningful way? During 3.0 we introduced file buffers. Using JDT terminology, you can say that file buffers implement the concept of working copies. I.e. a file that is being changed. As file buffers are independent from UI, it does matter whether the changing object is an editor, some action, or what so ever. For text files, the file buffers content is given in form of a document and annotation model. Partitioning is a concept defined by documents. There is well defined relation between document partitioning and position updating. Position updaters can assume that at the point in time they are requested to update positions in reaction to a document change, the document partitioning is up to date. With 3.0, document support multiple partitionings at the same time. Text editors use one particular partitioning to configure their behavior. This partitioning is defined by SourceViewerConfiguration.getConfiguredDocumentPartitioning. As before, in 3.0 you can have a situation in which the same file is open in two different editors, say the Java editor and the Text editor. Before 3.0 the changes made in the text editor and those in the Java editor were decoupled from each other. The conflict had to be resolved manually by the user. In 3.0 both editors work with document providers that in the end rely on file buffers. I.e. in both editors the same document is shown. However, both editors rely on different partitionings. This concrete case is one instance of simultaneous changes. Think of actions that work on file buffers and don't want to be aware of whether any text editor is open on that file buffer or not. Each of these actions might assume the same document partitioning as some text editor are a completely different one. This is why we came up with what is there right now. As pointed out, the critical point is performance. In the current version, the partitioning once installed is always active. This indeed needs to be improved. The other point is dynamic assignment of editors to file extensions. There is a conceptual flaw. The IDE defines with which types of content it can work. In order to add a new type of content you usually have to install new plug-ins. Now that we have the explicit concept of content types, editors should be assigned to content types. This should be decoupled from the strategy that assigns content types to files. Users should be allowed to configure that strategy. One way among other would be file extensions. Having written all that, this PR is actually about the different issue of different code paths when creating an editor and when changing its input. Thanks for the documentation! Still not entirely clear. For example, javadoc of IDocumentExtension3.setDocumentPartitioner says "The caller of this method is responsible for disconnecting the document's old partitioner from the document..." but no callers seem to. (The only way to disconnect a partitioner is to call setDocumentPartitioner with a null partitioner argument, but this is not documented.) The Ant editor likes to set the document partitioner so well that it does it once in AntDocumentSetupParticipant.setupDocument and again in AntStorageDocumentProvider.setup. Should one assume that both are necessary since StorageDocumentProviders do not necessarily refer to files and therefore are not associated with a file extension, so the setup participant would never be called? Ditto for the Java editor (except for the oddly placed call in JavaAutoIndentStrategy.installJavaStuff, don't know what that's about). WRT getConfiguredDocumentPartitioning: The java editor example for 3.0 doesn't override this. I do override it (now) but it doesn't help. My DamagerRepairer is never called. Further, I don't see the result of getConfiguredDocumentPartitioning() - sets the value of fPartitioning which is returned by getConfiguredDocumentPartitioning() and otherwise not referenced - used in any interesting way during a document change. There are only three references to getConfiguredDocumentPartitioning(), none of which appear to be relevant. Regarding comment #9: My statement "Text editors use one particular partitioning to configure their behavior. This partitioning is defined by SourceViewerConfiguration.getConfiguredDocumentPartitioning." is a bit misleading. The editor uses that partitioning. However, there are editor functions that are implemented by text viewer add-ons. One example is syntax highlighting. Text viewer add-ons work autonomous, once they are installed. I.e. the editor does not have to call them when a content change happens. Thus, the internal structure of the add-ons are opaque for the text viewer and also the editor. Prior to 3.0, the add-ons made use of document partitioning. In 3.0, we added extension interfaces for text viewer add-ons that reflect their ability to work with multiple document partitionings by proving the getDocumentPartitioning method. Our default implementations of the add-ons implement the extension interface and additionally provide a setDocumentPartitioning method. For syntax highlighting the extension interface is IPresentationReconcilerExtension and the default implementation is PresentationReconciler. When creating the PresentationReconciler you have to specified the partitioning the reconciler should work with. Regarding comment #8: Disconnecting refers to the IDocumentPartitioner.disconnect method. Connecting refers to the IDocumentPartitioner.connect method. In the Platform and in the JDT, the ones I can talk about, we don't call disconnect as we (unsafely) assume that no partitioner for the given partitioning is installed. Now, that everybody can install document setup participants this may no longer be true but would indicate a configuration issue. The Javadoc refers more to situations such as you temporarily remove a document partitioner while an expensive operation is running. Deferred to 3.2. 3.0 tries to support a compatibility option where editors using the 2.x API would continue to work, but in fact it has created a situation where if there are two editors using the 2.x API and both edit the same extension, one or both of them stop working. This has turned out to be the most frequently reported user problem. Yet, one is reluctant to convert to the 3.0 API because a) there is little or no documentation as to what that is and b) the listener order is different than in the compatibility mode. The technical term for this is "a mess". >if there
>are two editors using the 2.x API and both edit the same extension, one or both
>of them stop working. This has turned out to be the most frequently reported
>user problem.
Couldn't fint the bug regarding this. Which one is it?
I'll file a bug report if that will make you happy. Otherwise, you could talk to the Ant editor people, who adopted the new APIs after they found that using the old APIs either broke them or any other *.xml editor. It's bug 96917. (Sorry, I don't know how to make that a link.) You just did :-) Ah. It's magic. Something new every day. ;-} Resetting priority to P3. Will be reassessed for the next release. This bug describes two different problems: the one reported regarding the code (path) that handles editor setup and reuse and the one that has been introduced during the discussion of this bug. For this second problem I filed bug 101846. It's been a long time since I last visited these issues (with code, at least) and I discover I've forgotten some key facts. - Exactly what interface is it that, if you implement it, changes the order in which listeners are called (in particular, has the side effect of calling one's IContentAssistProcessor before the event that triggered it is reflected in the document)? - I seem to recall there was more than one kind of document configuration (to handle the "new" JavaFileEditorInput)? Deja vu is killing me. I have the following in plugin.xml: <extension point="org.eclipse.core.filebuffers.documentSetup" id="com.objfac.xmleditor.XMLDocumentSetupParticipant" name="%xmlDocumentSetupParticipant.name" > <participant extensions="xml,tld,xsl,xslt,xhtml,html,htm,rng,php,xsd,dtd,mod" class="com.objfac.xmleditor.extensions.XMLDocumentSetupParticipant"/> </extension> I open a *.xml file. XMLDocumentSetupParticipant.setup() is never called. What other incantations do I need to get this to work? I see. The document setup hook is only called sometimes, even for matching extensions sometimes not, and plugins need to compensate for that by calling it themselves whenever they find out it has not been. Moreover, it may now be called multiple times for the same document, even though re-adding a partitioner is not side effect free. This is slightly worse than last year. Having gotten it to be called and my partitioner used, I now see that auto-triggered content assist no longer works. It appears the trigger characters are examined _after_ the partitions are updated as a result of a keystroke. Since triggers are partition-type dependent, it is only good luck if the same keystroke is a trigger in the new context. I wonder how one works around that? (In reply to comment #24) I was all wet on the second part. It's just a 3.0 API difference. Need to specify the partition type to the assistant. >I see. The document setup hook is only called sometimes, even for matching >extensions sometimes not, and plugins need to compensate for that by calling it >themselves whenever they find out it has not been Can you list those cases? I'd expect this consistently work for workspace files. Of course if you open external files (or from CVS) this will not work because the editor input is not an IFileEditorInput. To make it work a ForwardingDocumentProvider needs to be installed as parent document provider in the constructor: e.g. the PropertiesFileDocumentProvider does this: public PropertiesFileDocumentProvider() { IDocumentProvider provider= new TextFileDocumentProvider(); provider= new ForwardingDocumentProvider(IPropertiesFilePartitions.PROPERTIES_FILE_PARTITIONING, new PropertiesFileDocumentSetupParticipant(), provider); setParentDocumentProvider(provider); } re comment 23: was it an external file? If not, can you give more details? >Moreover, it may now be called multiple times for the same document, - does this happen by our code or by the code that you added? - can you make it go away if you use a forwarding document provider? See PropertiesFileDocumentSetupParticipant for an example of the setup(IDocument). re comment 24: "This is slightly worse than last year." AFAIK we did not change anything in that area. re comment 25: correct. (In reply to comment #26) > Can you list those cases?... > Of course if you open external files (or from CVS) this will not work The Open File... menu item "will not work"? > > >Moreover, it may now be called multiple times for the same document, > > re comment 24: "This is slightly worse than last year." > AFAIK we did not change anything in that area. You added content types and fixed bugs related to not calling setup for file associations the user makes. The net is the same setup provider can appear multiple times in the list you collect, and is called multiple times. >The Open File... menu item "will not work"? I tried to guess your scenario/case: if you only used a TextFileDocumentProvider (or subclass) without setting a forwarding document provider as parent then "Open File..." command would work for files that can be located inside your workspace but not for those outside since the implementation first checks whether the selected file is contained in the workspace (==> IFileEditorInput) or not (JavaFileEditorInput). For the Eclipse SDK editors the "Open File..." command works for external files. >You added content types Platform Text and JDT Text do not define any content type. I guess you refer to Ant and others. >and fixed bugs related to not calling setup for file >associations the user makes. That must have been already before 3.0. I scanned all the file buffer changes since 3.0 and could not find any such change, however still a good catch: while scanning the code I found the bug: we collect the same document participant more than once. We should call each participant just once. I filed bug 104321 for this. I also scanned clients of setup(...) and when I look at the Ant code I see that they call setup(...) on their own at various locations, which is not so good (filed bug 104320). (In reply to comment #28) > I tried to guess your scenario/case: if you only used a TextFileDocumentProvider > (or subclass) without setting a forwarding document provider as parent then > "Open File..." command would work for files that can be located inside your > workspace but not for those outside since the implementation first checks > whether the selected file is contained in the workspace (==> IFileEditorInput) > or not (JavaFileEditorInput). I don't understand. Why isn't that a bug? Also, it is no good recommending that this or that document provider be used, as in Bug 104320; an editor cannot be certain its own document provider will be the one that creates a document. This scheme only works if one has absolute assurance one's setup() will be called in _every_ document one is called to edit. Good on bug 104321, not that the test for duplicate calls is very expensive. For bug 104320, if you read my little recipe you know I have code in my editor that explicitly calls setup(), as well. I'll be happy to remove it when it is no longer needed. ;-} >I don't understand. Why isn't that a bug? Because it's up to the editor implementor to decide what kind of input (external, internal, CVS, DB,...) it supports. Out of the box we support IFile. >I have code in my editor that >explicitly calls setup(), as well. It's not "bad" if you know what you're doing. We've chosen to call a util to do this kind of document setup if need instead of calling IDocumentSetupParticipant.setup(IDocument). Same result but lets it easier separate the manual from framework/automatic document setup. >I'll be happy to remove it when it is no longer needed. ;-} If you use the forwarding document provider you should be able to get rid of this. (In reply to comment #30) > >I don't understand. Why isn't that a bug? > Because it's up to the editor implementor to decide what kind of input > (external, internal, CVS, DB,...) it supports. Out of the box we support IFile. Huh? Eclipse has a File > Open File... command and you decide not to support it? That doesn't make any sense to me. > >I'll be happy to remove it when it is no longer needed. ;-} > If you use the forwarding document provider you should be able to get rid of this. I'll believe that when I believe that my document provider will always be used for any way my editor can be opened in Eclipse. That has not been my experience with external files. I don't have the luxury of not supporting them. Most likely not for 3.2. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the request is still relevant please remove the stalebug whiteboard tag. |