Menu
▾
▴
Re: [ jEdit-devel ] [ jEdit-commits ] SF.net SVN: jedit:[17492] jEdit/trunk
|
From: Dale A. <da...@gr...> - 2010-03-18 20:56:22
|
On Thu, Mar 18, 2010 at 11:30 AM, Kazutoshi Satoda <k_s...@f2...>wrote: > dal...@us... wrote: > >> Log Message: >> ----------- >> Adjusted the ModeProvider so it is a little smarter in picking the mode >> for a file. >> > (snip) > > This change seems have come from the exact motivation of the patch > #2822686. > https://sourceforge.net/support/tracker.php?aid=2822686 > > Please update the status of the patch. The comment from Matthieu, which > concerns about performance seems also relevant for your change. Thanks for the pointer, I wasn't aware of the patch. Profiling showed negligible performance impact with my change. The patch does not take into account that user modes take priority over global modes. I'll update it and close it out. > > Modified: jEdit/trunk/doc/CHANGES.txt >> > (snip) > > +- Fixed ModeCatalogHandler so if an edit mode in the user-specific >> catalog has >> + the same name as an edit mode in the system catalog, the version in the >> + user-specific catalog will override the system default. This is what >> the >> + documentation for writing an edit mode has stated is true for some >> time, now >> + it is. (Dale Anson) >> > > Since CHANGES.txt is for users, it's not so good idea to start with a > class name (implementation detail) here. It's better to write an entry > to be easy to understand for users. > Okay, I'll keep that in mind in the future. Perhaps some sort of guidelines should be posted somewhere? Looking through the changes.txt file, I see several references to specific class names and comments that are more geared toward developers than end users. I've seen other projects do both a changes.txt file that is more technical, and a release notes file that is more for end users. Maybe we need both? If so, the release notes could easily be constructed from the highlights of the changes.txt file and would only need to be done just before the actual release rather than being continually maintained. > > Also, this looks like a bug fix. Would you like to submit a merge > request for 4.3.x? But, because this revision also includes the > following change (not a bug fix), this will need a separate branch. Sure, I can submit a merge request. Sorry about combining both updates in changes.txt at the same time. I missed committing it with the first commit, so it got included with my second commit. > > > +- Adjusted the ModeProvider so it is a little smarter in picking the mode >> for a >> + file. It is possible for a file to match more than one mode. >> Previously, the >> + first mode that would accept the file was selected as the mode for the >> file. >> + This change checks a little more, so a mode that matches both the >> filename >> + glob and the first line glob will be selected before a mode that >> matches only >> + the filename glob. If no mode matches both filename glob and first >> line glob, >> + then a mode that matches the filename glob will be selected before a >> mode that >> + only matches the first line glob. This is particularly useful for xml >> files, >> + so that Ant build files can be differentiated from jEdit actions.xml >> files and >> + from regular xml files. (Dale Anson) >> > > Same as above. And this looks too long. I think the following is > enough: > Gave better priorities within modes which matches with a file at the > same time, as the following order. > > - both the filename glob and the first line glob > - only the filename glob > - the first line glob > > This goes back to who is the audience for changes.txt versus release notes. > > Modified: jEdit/trunk/org/gjt/sp/jedit/Mode.java >> =================================================================== >> > (snip) > >> + //{{{ equals() method >> >> + /** >> + * @return<code>true</code> if this mode has the same name as >> another mode. >> + * Only the name of the mode matters, this allows user created >> modes to >> + * be treated the same as global modes regardless of the file name >> glob >> + * or the first line glob. >> + */ >> + public boolean equals(Object other) >> > > This looks a responsibility of class ModeProvider and should better be > solved in ModeProvider itself. > What do you mean here? The comment describes the implementation of the equals method. I'm not sure what ModeProvider has to do with it here? > > Additionally, both contains() and remove() over ArrayList based on > equality needs O(N) time for N modes, which means whole set up needs > O(N**2) time (call addMode() N times). This can likely result in a > noticeable performance regression for hundreds of modes. > > If you want to find a mode by its name, the appropriate container would > be Map<String, Mode>, which can search a mode with O(logN) time. Yes, I suppose that would be slightly better for performance, but as I mentioned above, profiling showed a negligible performance impact. However, with a map, the ordering becomes an issue and order is important. I'll take another look at it. (BTW, there are 180 modes in the global catalog. I have about 15 in my user catalog. I doubt either grows that much more.) > > > Modified: jEdit/trunk/org/gjt/sp/jedit/syntax/ModeProvider.java >> > (snip) > >> + if (acceptable.size()> 1) { >> >> + // most acceptable is a mode that matches both the >> + // and the first line glob >> > Missing "filename glob" before "and the ..."? > > > public void addMode(Mode mode) >> { >> + // Modes are loaded from the global catalog first, then >> from any >> + // user catalogs. If a user mode is added with the same >> name as >> > > Only the caller knows the order of addition. Assuming the order on > callee side is bad. > Well, and now anyone reading through this code knows the order of addition. This comment explains why the 'remove' is being done. The order isn't assumed, it's known. The 'remove' doesn't make sense unless the order is known. Notice that the getModeForFile method also makes this assumption. > > Instead, I recommend to specify the behavior of addMode() on addition > of equivalent mode, and put the comment about priority on caller side. > I agree, both sides should be documented. > > -- > k_satoda > |