Menu
▾
▴
#3707 old modes/catalog entries clutter properties
closed-fixed
7
2012-11-26
2012-06-09
No
If I add a custom mode to my .jedit/modes/catalog and later remove it,
there is still an entry for that mode.x.file= in my properties file that never gets cleaned up.
steps to reproduce.
add a custom c.xml file to your .jedit/modes.
add a catalog entry for it
use it
later remove the catalog entry
reload modes
it is still using that c.xml file you got.
how do you know? console beanshell: jEdit.getMode("c").getProperty("file")
now rm .jedit/modes/c.xml
reload modes
then you see a FileNotFound exception and all other garbage.
I think at the very least, when the file is not found it should clear the mode property, but perhaps there is a better way - avoiding saving it to properties entirely, since it is already in the modes/catalog?!?!?
The problem is that those properties should not be saved. A boolean field "initialised" and a method init() has been put in Mode to allow for transient properties to be defined based on the file contents and persistent properties set afterwards in the Options > Editing > Custom settings pane.
But, in ModeCatalogHandler#startElement(), the old mode is reused if possible (line 65) and only if it's not found is a new mode instantiated.
This works fine when loading the system modes: no preexisting mode, so initialised is false when setting mode properties based on the content of the element in catalog. Then init() is called on the mode and later properties set in the option panes are persisted, and override the transient properties.
This works fine as well when loading new user modes: no preexisting mode either.
But what happens when an overriden mode is present in the user catalog: the old mode is reused, and the setProperty() doesn't put values in the transient properties of the mode, but really in jEdit's saved properties.
Now, mode loading (parsing the_mode.xml) happens only on demand, and the file to parse is taken from a (hopefully) transient property "file" of the mode. If an overriden is loaded, mode.the_mode.file is saved as a jEdit property. If it is later on removed, the value of the saved property still overrides the transient value set when reading the system catalog and you get the FileNotFoundException...
So, why reuse modes ?
Maybe because some parts of jEdit keeps a reference to the Modes and wouldn't get the new Mode instance. Think of the buffers, for instance.
Well, ModeCatalogHandler is only called from jEdit#reloadModes() which resets modes of buffers afterward, so it's not a problem.
The only other pb I can think of is mode properties not inherited from system level to user level, but if you override a mode, you have removed these properties from the user mode for a reason, haven't you ?
If it's not a pb, simply creating a new mode each time would have its "initialised" field false when setting properties in ModeCatalogHandler#startElement even for overriden modes and all would be fine.
Cleaning up the environment by unsetting the "file" property on load would be also good. But you would have to call jEdit.unsetProperty("mode.the_mode.file") directly because you want to remove it from jEdit properties but keep it in the mode's transient properties.
In JEdit.java:4465 seems a good candidate to me, since ModeCatalogHandler must be compatible with standaloneTextArea...
Basically the property is redundantly stored in modes/catalog and also in properties.
When we do a mode.getProperty("file") it should somehow fetch it from modes/catalog instead of properties so we don't need to ever store it in the properties file.
Some special case for "file"? It can't be changed from the jEdit options anyway, am I correct?
OK for mode.file, but there are problems also with filenameGlob and firstlineGlob: mode.unsetProperty() is called, which will override user custom settings at startup, basically...
I really think the most straightforward thing would be to not reuse modes.
Do you see anything against this ?
I have nothing against you fixing this issue, kerik-sf. I am not sure how to answer your question though since I have not seen your patch.
oh I see, you fix the problem by not caching modes at all. this is fine with me.
diff against r22487
yes, this is what I mean.
See the attached patch, to be very sure.
committed in r22502.