Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#444 simplify SearchDialog.save

closed-accepted
None
4
2012-08-02
2012-07-27
Eric Le Lay
No

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.

Discussion

  • Eric Le Lay
    Eric Le Lay
    2012-07-27

    patch against r21954

     
  • Could you explain why the code is unnecessary? It is not obvious to me.

     
  • Eric Le Lay
    Eric Le Lay
    2012-07-27

    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)

     
  • 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.

     
    • assigned_to: nobody --> jarekczek
    • status: open --> closed-rejected
     
  • Eric Le Lay
    Eric Le Lay
    2012-07-27

    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.

     
  • If you like, please resubmit. The EB message seems to be redundant indeed.

     
    • status: closed-rejected --> pending-rejected
     
    • status: pending-rejected --> closed-accepted
     
  • Applied agreed parts as r21970.