[ jEdit-devel ] [ jedit-Patches-3549670 ] simplify SearchDialog.save

[SourceForge.net <no...@so...>]

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: Pending
Resolution: Rejected
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-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