Bug in 'swap header / source'

[Jenna]

I looked into the code yesterday and see why it not asks to create the missing header-/source-file.

Creating (or more exactly asking for creating) the missing file in the same directoy as the original file is not a big problem, any other solution (like chosing the directory) would need a little (not much) more coding effort.

[Jenna]

Should work in svn r5865 again.

The opposite file is created in the same directory as the file we tried to switch from.
The behaviour should now be the same as before.

[ironhead]

we can have a look at this function in editormanager.cpp

Code

bool EditorManager::SwapActiveHeaderSource()
{
    cbEditor* ed = GetBuiltinEditor(GetActiveEditor());
    if (!ed)
        return false;

    FileType ft = FileTypeOf(ed->GetFilename());
    if (ft != ftHeader && ft != ftSource)
        return false;

    // because ft == ftHeader || ftSource, the extension has at least 1 character
    bool extStartsWithCapital = wxIsupper(wxFileName(ed->GetFilename()).GetExt()[0]);

    // create a list of search dirs
    wxArrayString dirs;
    cbProject* project = 0;

    // if the file in question belongs to a different open project,
    // then use that project instead.
    // this fixes locating the file's pair in a workspace when the active file
    // does not belong to the active project.
    ProjectFile* opf = ed->GetProjectFile();
    if (opf)
        project = opf->GetParentProject();

    // if we didn't get a valid project, try the active one
    if (!project)
        project = Manager::Get()->GetProjectManager()->GetActiveProject();

    if (project)
    {
        // get project's include dirs
        dirs = project->GetIncludeDirs();

        if (opf)
        {
            wxString const &activeName = opf->file.GetName();

            // first try to find the file among the opened files
            for (int i = 0; i < GetEditorsCount(); ++i)
            {
                cbEditor* edit = GetBuiltinEditor(GetEditor(i));
                if (!edit)
                    continue;

                ProjectFile* pf = edit->GetProjectFile();
                if (!pf)
                    continue;

                if (pf->file.GetName() == activeName)
                {
                    wxFileName const & fname = pf->file;
                    FileType ft_other = FileTypeOf(fname.GetFullName());
                    if (   (    ((ft == ftHeader) && (ft_other == ftSource))
                             || ((ft == ftSource) && (ft_other == ftHeader)) )
                        && (wxIsupper(fname.GetExt()[0]) == extStartsWithCapital) )
                    {
                        if (fname.FileExists())
                        {
                            cbEditor* newEd = Open(fname.GetFullPath());
                            if (newEd!=0L) // we found and were able to open it
                                return true; // --> RETURN
                        }
                    }
                }
            }

            // second try to find in the project files - at the same time
            // build the directory list for further searching if not
            // successful now
            for (int i = 0; i < project->GetFilesCount(); ++i)
            {
                ProjectFile* pf = project->GetFile(i);
                if (!pf)
                    continue;

                wxString dir = pf->file.GetPath(wxPATH_GET_VOLUME);
                if (dirs.Index(dir) == wxNOT_FOUND)
                    dirs.Add(dir);

                if (pf->file.GetName() == activeName)
                {
                    wxFileName const & fname = pf->file;
                    FileType ft_other = FileTypeOf(fname.GetFullName());
                    if (   (    ((ft == ftHeader) && (ft_other == ftSource))
                             || ((ft == ftSource) && (ft_other == ftHeader)) )
                        && (wxIsupper(fname.GetExt()[0]) == extStartsWithCapital) )
                    {
                        if (fname.FileExists())
                        {
                            cbEditor* newEd = Open(fname.GetFullPath());
                            if (newEd!=0L) // we found and were able to open it
                                return true; // --> RETURN
                        }
                    }
                }
            }
        }
        else // no opf
        {
            // build the directory list for further searching if opf not available
            for (int i = 0; i < project->GetFilesCount(); ++i)
            {
                ProjectFile* pf = project->GetFile(i);
                if (!pf)
                    continue;

                wxString dir = pf->file.GetPath(wxPATH_GET_VOLUME);
                if (dirs.Index(dir) == wxNOT_FOUND)
                    dirs.Add(dir);
            }
        }

        // if not found, continue building the list of directories for further searching

        // get targets include dirs
        for (int i = 0; i < project->GetBuildTargetsCount(); ++i)
        {
            ProjectBuildTarget* target = project->GetBuildTarget(i);
            if (target)
            {
                for (unsigned int ti = 0; ti < target->GetIncludeDirs().GetCount(); ++ti)
                {
                    // TODO (Morten#5#): target include dirs might override project include dirs, take append/prepend option into account
                    wxString dir = target->GetIncludeDirs()[ti];
                    if (dirs.Index(dir) == wxNOT_FOUND)
                        dirs.Add(dir);
                }
            }
        }
    } // project

    wxFileName fname;
    wxFileName fn(ed->GetFilename());
    dirs.Insert(fn.GetPath(wxPATH_GET_VOLUME), 0); // add file's dir

    for (unsigned int i = 0; i < dirs.GetCount(); ++i)
    {
        ProjectManager *pm = Manager::Get()->GetProjectManager();
        if ( !pm )
            break;

        wxString dir = dirs[i]; // might contain macros -> replace them
        Manager::Get()->GetMacrosManager()->ReplaceMacros(dir);

        fname.Assign(dir + wxFileName::GetPathSeparator() + fn.GetFullName());
//        Manager::Get()->GetLogManager()->DebugLog(F(_T("Looking for '%s', dir='%s'."), fname.GetFullPath().c_str(), dir.c_str()));
        if (!fname.IsAbsolute() && project)
        {
            fname.Normalize(wxPATH_NORM_ALL & ~wxPATH_NORM_CASE, project->GetBasePath());
//            Manager::Get()->GetLogManager()->DebugLog(F(_T("Normalizing dir to '%s'."), fname.GetFullPath().c_str()));
        }

        wxString HeaderSource = pm->GetHeaderSource(fname);
        if (!HeaderSource.IsEmpty())
        {
            fname.SetFullName(HeaderSource);
//            Manager::Get()->GetLogManager()->DebugLog(F(_T("Located '%s'."), fname.GetFullPath().c_str()));
            break;
        }
    }

    if (fname.FileExists())
    {
        //Manager::Get()->GetLogManager()->DebugLog("ed=%s, pair=%s", ed->GetFilename().c_str(), pair.c_str());
        cbEditor* newEd = Open(fname.GetFullPath());
        if (newEd!=0L) // we found and were able to open it
            return true; // --> RETURN;
    }

    // We couldn't find the file, maybe it does not exist. Ask the user if we
    // should create it:
    if (cbMessageBox(_("The file seems not to exist. Do you want to create it?"),
                _("Error"), wxICON_QUESTION | wxYES_NO) == wxID_YES)
    {
        cbProject* project = Manager::Get()->GetProjectManager()->GetActiveProject();
        if (project)
            wxSetWorkingDirectory(project->GetBasePath());

        // Create a suggestion for the new file name:
        if (ft == ftHeader)
            fn.SetExt(FileFilters::CPP_EXT);
        else if (ft == ftSource)
            fn.SetExt(FileFilters::H_EXT);
        // else? Well, if the filename is not changed we could possibly
        // overwrite an existing file with our suggestion.

        cbEditor* newEd = New(fn.GetFullPath());
        if (project)
        {
            if (cbMessageBox(_("Do you want to add this new file in the active project?"),
                        _("Add file to project"),
                        wxYES_NO | wxICON_QUESTION) == wxID_YES)
            {
                wxArrayInt targets;
                if (Manager::Get()->GetProjectManager()->AddFileToProject(newEd->GetFilename(), project, targets) != 0)
                {
                    ProjectFile* pf = project->GetFileByFilename(newEd->GetFilename(), false);
                    newEd->SetProjectFile(pf);
                    Manager::Get()->GetProjectManager()->RebuildTree();
                }
            }
        }

        // verify that the open files are still in sync
        // the new file might have overwritten an existing one)
        Manager::Get()->GetEditorManager()->CheckForExternallyModifiedFiles();
    }

    return false;
}

Having a quick look at the code, I think the issue I'm encountering is due to the fact that the directory is not being taken in to account, both the first (searching open files) and second (searching the project) seem to rely on filename only (i.e. not the relative path to the file).

Going back to my original example:

root
-- subdir1
    Config.cpp
    Config.h
-- subdir2
    Config.cpp
    Config.h
-- subdir3
    Config.cpp
    Config.h

If I open Config.cpp in either subdir1 or subdir2 with no other files open and hit 'swap...' Config.h in subdir1 is opened, I assume because the second search (the one through the project) is matching on 'Config.' without paying attention to the directory in which it resides.

Similarly, if I have Config.h from subdir1 open and Config.cpp from subdir3 open, if I hit 'swap..' on Config.h from subdir1, it switches to Config.cpp from subdir3 because it's matching on 'Config' (based on the first search which uses the open files).

I think for my issue the relative path must be taken into consideration.

[ironhead]

Based on my analysis of the issue in my previous post, I've submitted a patch:

http://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2841&group_id=5358

That includes the file path as criteria for the match.  This addresses the issues that I've encountered.

Cheers!

[ollydbg]

Based on my analysis of the issue in my previous post, I've submitted a patch:

http://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2841&group_id=5358

That includes the file path as criteria for the match.  This addresses the issues that I've encountered.

Cheers!

I have applied in my working copy, and works fine!! Thanks!!!

[Zini]

Haven't tried it myself, but from what I see you are limiting the search to one directory per file. Wouldn't that break the feature entirely for source layouts, where h and cpp files are separated into different directories?

[ironhead]

Haven't tried it myself, but from what I see you are limiting the search to one directory per file. Wouldn't that break the feature entirely for source layouts, where h and cpp files are separated into different directories?

Yes, the patch I supplied would not correct this issue, but the original implementation would only work if you only had one occurrence of each file.  Let's say you had:

root
--- subdir1
    --- src
         Config.cpp
    --- headers
         Config.h
--- subdir2
    --- src
         Config.cpp
    --- headers
         Config.h

In the original implementation, if you opened Config.h in subdir2 and hit 'Swap...' you would get Config.cpp in subdir1/src I suspect.

To correct this, I believe you would need to implement what Tim S. suggested and search the include path for the project / build target.  However, even this implementation I suspect will have issues (i.e. I don't think it would work well for my particular issue).

I'm open to suggestions on how to more generically address this issue.

[Zini]

Well, you could make a list of all candidate files and then apply some heuristics to find the most likely candidate. For a start you could look for identical path elements (subdir1 in your example).

But that still doesn't handle the case, where the corresponding file does not exist yet. At this point you only have two options:

1. Offer the user a new dialogue, in which he can specify the right directory. Additionally you might want to use the heuristics mentioned above to find the most likely candidate.

Example:

include
-- A
---- foo.h
---- bar.h
-- B
---- foo.h
src
-- A
---- foo.cpp
-- B
---- foo.cpp

From the include/A/foo.h src/A/foo.cpp pair a smart algorithm may deduce, that the newly created cpp file for bar.h goes into src/A/bar.cpp

2. Enhance the Code::Blocks project file format with some information about the source layout. This information could be used to improve other functionality too. For example it could help to automatically pick the right directory for files newly created from the File -> New dialgoue.

Both options sound like a lot of work unfortunately.

[ironhead]

Looking at the code again, it actually already searches the include path if it doesn't find it in the list of currently open editors or in the project files (i.e. in the same directory):

Code

        // get targets include dirs
        for (int i = 0; i < project->GetBuildTargetsCount(); ++i)
        {
            ProjectBuildTarget* target = project->GetBuildTarget(i);
            if (target)
            {
                for (unsigned int ti = 0; ti < target->GetIncludeDirs().GetCount(); ++ti)
                {
                    // TODO (Morten#5#): target include dirs might override project include dirs, take append/prepend option into account
                    wxString dir = target->GetIncludeDirs()[ti];
                    if (dirs.Index(dir) == wxNOT_FOUND)
                        dirs.Add(dir);
                }
            }
        }

With my patch applied, would it be possible for you to validate if my patch actually breaks the scenario you proposed?

As for the scenario where the file doesn't exist, I wouldn't suggest any automation in terms of guessing where the file should be created.  I would leave it up to the developer and simply provide a file dialogue to create the file.

[Zini]

Looking at the code again, it actually already searches the include path if it doesn't find it in the list of currently open editors or in the project files (i.e. in the same directory):

That is a part of the problem. Why the treat open files differently from non-open files? If it only works if the file isn't open, the algorithm is already broken.

With my patch applied, would it be possible for you to validate if my patch actually breaks the scenario you proposed?

Guess it will be less work, if you try it out. Please note that I have uploaded a test project, that demonstrates the problem (a few posts above).


Anyway, I couldn't sleep, so I wrote a new algorithm. Hope you can understand my python-pseudo-code. No guarantees about the correctness though (despite not being able to get to sleep I am awful sleepy already):

Code

listCandidates (file):
    e = extension (file)
    if e in header_extension_list:
        extension_list = source_extension_list
    elseif e in source_extension_list:
        extension_list = header_extension_list
    else
        throw error (unknown extension)

    l = leaf_without_extension (file)
        
    for f in project: # iterate over all files in the project
        if leaf_without_extension (f)==l and extension (f) in extension_list:
            add f to list
            
    return list

findCandidate (file):
    list = listCandidates (file)
    n = len (file) # len (file) is the number of path elements
    
    if n>1:
        for f in list if len (f)==n:
            for i in range (0, n-2):
                if f[i]==file[i]:
                    return f # success
    else
        for f in list if len (f)==1:
            return f # success (top level directory)

    # maybe add other methods to determine the file in case the above fails

    return null # no file found; either does match no known pattern or it does not exist
    
swap (file):
    c = findCandidate (file):
    
    if c!=null:
        open c
        return
        
    # file does not exist
    parent = file[0:n-2] # one level up (the directory where the file is located)
    for f in parent if f!=file: # iterating over the files in the directory
        c = findCandidate (f)
        if c!=null:
            create corresponding file in directory c[0:n-2] and open it
            return
            
    # give up; either ask the user for a directory or report that the file can't be created

[ironhead]

Looking at the code again, it actually already searches the include path if it doesn't find it in the list of currently open editors or in the project files (i.e. in the same directory):

That is a part of the problem. Why the treat open files differently from non-open files? If it only works if the file isn't open, the algorithm is already broken.

Just to clarify, the patch I provided works with both open and non-open files in the same manor.

With my patch applied, would it be possible for you to validate if my patch actually breaks the scenario you proposed?

Guess it will be less work, if you try it out. Please note that I have uploaded a test project, that demonstrates the problem (a few posts above).

Hrm, the patch doesn't handle the include/A with src/A correctly, so there is definitely room for improvement. 

Anyway, I couldn't sleep, so I wrote a new algorithm. Hope you can understand my python-pseudo-code. No guarantees about the correctness though (despite not being able to get to sleep I am awful sleepy already):

Code

listCandidates (file):
    e = extension (file)
    if e in header_extension_list:
        extension_list = source_extension_list
    elseif e in source_extension_list:
        extension_list = header_extension_list
    else
        throw error (unknown extension)

    l = leaf_without_extension (file)
        
    for f in project: # iterate over all files in the project
        if leaf_without_extension (f)==l and extension (f) in extension_list:
            add f to list
            
    return list

findCandidate (file):
    list = listCandidates (file)
    n = len (file) # len (file) is the number of path elements
    
    if n>1:
        for f in list if len (f)==n:
            for i in range (0, n-2):
                if f[i]==file[i]:
                    return f # success
    else
        for f in list if len (f)==1:
            return f # success (top level directory)

    # maybe add other methods to determine the file in case the above fails

    return null # no file found; either does match no known pattern or it does not exist
    
swap (file):
    c = findCandidate (file):
    
    if c!=null:
        open c
        return
        
    # file does not exist
    parent = file[0:n-2] # one level up (the directory where the file is located)
    for f in parent if f!=file: # iterating over the files in the directory
        c = findCandidate (f)
        if c!=null:
            create corresponding file in directory c[0:n-2] and open it
            return
            
    # give up; either ask the user for a directory or report that the file can't be created

I'm afraid my python knowledge is non-existent, but I'll take a closer look at this tomorrow once I've had a chance to catch up on some sleep myself.  Thank you for the pseudo code!

Cheers!

[ironhead]

Putting some more thought into the 'include/A/Unit.h' / 'src/A/Unit.cpp' I'm not sure there is a sure fire solution.  For the swapping to the header, you could use the project / build target's include paths and add the relevant relative path (i.e. the 'A'), but the for the other direction, how would C::B implicitly know that the source files for 'include/A' are in 'src/A'?

[Zini]

Because of the 'A'?

That might not be 100% correct all the time, but you would need some rather exotic layout for the test to fail.

[ironhead]

Even the approach of using the immediate parent directory isn't full proof either.

Lets say you had:
<root>
--- <A>
     --- <src>
          Config.cpp
     --- <include>
          Config.h

Again, the include path would determine where Config.h is relative to Config.cpp, but how would C::B know that for Config.h, Config.cpp is in <src>?

At this point I really don't see a better way of handling things then what I've already proposed in my patch.  I fully admit that it does not handle all cases, but I don't know of an algorithm that would, and I believe the patch makes it better than what is there today.

If anyone else is willing to take a stab at implementing a better algorithm, I'd be happy to help out in any way I can.

[Zini]

Still haven't got around to test your patch (sorry, so busy atm. its insane!), but IIRC it stops the feature from working when h and cpp files sit in different directories. That would break the feature completely for me (and most likely a lot of other people).

At this point I really don't see a better way of handling things then what I've already proposed in my patch.  I fully admit that it does not handle all cases, but I don't know of an algorithm that would, and I believe the patch makes it better than what is there today.

I agree that a solutation that handles all cases isn't possible without storing information about the source layout in the project file. However the situation you mentioned is handled perfectly by my algorithm. Did you have to time to take a look at it?