From: SourceForge.net <no...@so...> - 2012-08-02 20:13:06
|
Patches item #3549670, was opened at 2012-07-26 23:23 Message generated for change (Comment added) made by jarekczek You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=300588&aid=3549670&group_id=588 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None >Status: Closed >Resolution: Accepted Priority: 4 Private: No Submitted By: Eric Le Lay (kerik-sf) Assigned to: Jarek Czekalski (jarekczek) Summary: simplify SearchDialog.save Initial Comment: current code in org.gjt.sp.jedit.search.SearchDialog#save() is overly complex for "Search In Directory" case and produces an unnecessary EB message. Here is a patch to fix that. ---------------------------------------------------------------------- >Comment By: Jarek Czekalski (jarekczek) Date: 2012-08-02 13:13 Message: Applied agreed parts as r21970. ---------------------------------------------------------------------- Comment By: Jarek Czekalski (jarekczek) Date: 2012-07-27 11:54 Message: If you like, please resubmit. The EB message seems to be redundant indeed. ---------------------------------------------------------------------- Comment By: Eric Le Lay (kerik-sf) Date: 2012-07-27 08:16 Message: You are right ! While we are at it, what about removing the 2 fields ? What about the explicit EB message sent. What's its goal ? And I think a comment in save() would be welcome, to prevent somebody from making the same mistake as I did. ---------------------------------------------------------------------- Comment By: Jarek Czekalski (jarekczek) Date: 2012-07-27 07:05 Message: DirectoryFileSet may be subclassed and in such cases it is not allowed to recreate it. That is the reason for which the code was done this way. So we mustn't apply this patch. See http://jedit.git.sourceforge.net/git/gitweb.cgi?p=jedit/ProjectViewer;a=blob;f=projectviewer/action/SearchAction.java;h=f4ed78090fd0578be7523181c44f1afa571d1bf9;hb=HEAD Eric, correct me if I'm wrong. ---------------------------------------------------------------------- Comment By: Jarek Czekalski (jarekczek) Date: 2012-07-27 06:06 Message: Thanks for the explanation. Still it was hard for me to make sure of that, but finally I am :) It was really twisted. The skipBinary and skipHidden members of DirectoryListSet are dummies and could be removed. They should be local variables instead. ---------------------------------------------------------------------- Comment By: Eric Le Lay (kerik-sf) Date: 2012-07-27 04:29 Message: Here is the old code commented. My point is that instead of checking the class and casting the existing fileset, then overriding every field, it's simpler to create a new one using the appropriate constructor. It will be consistent with all the other branches of the condition, where a new fileset is created. Also the DirectoryListSet fields could be made immutable since the setter methods are only called from here (maybe in plugins ?). And about sending a SearchSettingsChanged explicitly , it's redondant with the one sent when calling SearchAndReplace.setSearchFilter (last line of the extract). In the new code, I create a new DirectoryListSet each time (point #1) (line 762) SearchFileSet fileset = SearchAndReplace.getSearchFileSet(); // only used in case of Search directory boolean recurse = searchSubDirectories.isSelected(); if(searchSelection.isSelected()) fileset = new CurrentBufferSet(); else if(searchCurrentBuffer.isSelected()) { fileset = new CurrentBufferSet(); jEdit.setBooleanProperty("search.hypersearch.toggle", hyperSearch.isSelected()); } else if(searchAllBuffers.isSelected()) fileset = new AllBufferSet(filter); else if(searchDirectory.isSelected()) { String directory = this.directory.getText(); this.directory.addCurrentToHistory(); directory = MiscUtilities.constructPath( view.getBuffer().getDirectory(),directory); if((VFSManager.getVFSForPath(directory).getCapabilities() & VFS.LOW_LATENCY_CAP) == 0) { if(cancel) return false; int retVal = GUIUtilities.confirm( this,"remote-dir-search", null,JOptionPane.YES_NO_OPTION, JOptionPane.WARNING_MESSAGE); if(retVal != JOptionPane.YES_OPTION) return false; } if(fileset instanceof DirectoryListSet) // check instanceof { DirectoryListSet dset = (DirectoryListSet)fileset; // cast dset.setDirectory(directory); // reset every field of the object dset.setFileFilter(filter); dset.setRecursive(recurse); EditBus.send(new SearchSettingsChanged(null)); // send an EB message } else fileset = new DirectoryListSet(directory,filter,recurse); // #1: create a new DirectoryListSet } else { // can't happen fileset = null; // here I'd rather throw an exception, since it would be a programming error to be in this else branch } jEdit.setBooleanProperty("search.subdirs.toggle", recurse); jEdit.setBooleanProperty("search.keepDialog.toggle", keepDialog.isSelected()); SearchAndReplace.setSearchFileSet(fileset); // register the fileset (sends an EB message internally) ---------------------------------------------------------------------- Comment By: Jarek Czekalski (jarekczek) Date: 2012-07-26 23:34 Message: Could you explain why the code is unnecessary? It is not obvious to me. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=300588&aid=3549670&group_id=588 |