5
\$\begingroup\$

So I've been working on learning c++ and have created a file browser for the popular "Dear ImGui" framework (Immediate mode GUI). I'm looking for any feedback on its layout / code correctness. (It works well enough but maybe I'm doing something dumb?)

One of the things I'm specifically wondering, is it ok/proper to have static helper functions in the .cpp file if they are not member functions? (should they be member functions and should they even be static?)

.h

#pragma once

#include <vector>
#include <string>
#include <experimental/filesystem>
#include <filesystem>

// With Visual Studio compiler, filesystem is still "experimental"
namespace fs = std::experimental::filesystem;

namespace imgui_ext {

    const struct file {
        std::string alias;
        fs::path path;
    };

    class file_browser_modal final {

        static const int modal_flags;

        const char* m_title;

        bool m_oldVisibility;

        int m_selection;

        fs::path m_currentPath;
        bool m_currentPathIsDir;

        std::vector<file> m_filesInScope;

    public:

        file_browser_modal(const char* title);

        const bool render(const bool isVisible, std::string& outPath);

    };

};

.cpp

#include "file_browser_modal.h"

#include <limits>
#include <imgui/imgui.h>

using namespace imgui_ext;

static void get_files_in_path(const fs::path& path, std::vector<file>& files) {
    files.clear();

    if (path.has_parent_path()) {
        files.push_back({
            "..",
            path.parent_path()
        });
    }

    for (const fs::directory_entry& dirEntry : fs::directory_iterator(path)) {
        const fs::path& dirPath = dirEntry.path();
        files.push_back({
            dirPath.filename().string(),
            dirPath
        });
    }
}

static const int clamp_size_t_to_int(const size_t data) {
    static const int max_int = std::numeric_limits<int>::max();
    return static_cast<int>(data > max_int ? max_int : data);
}

static bool vector_file_items_getter(void* data, int idx, const char** out_text) {
    const std::vector<file>* v = reinterpret_cast<std::vector<file>*>(data);
    const int elementCount = clamp_size_t_to_int(v->size());
    if (idx < 0 || idx >= elementCount) return false;
    *out_text = v->at(idx).alias.data();
    return true;
}

inline static constexpr int modal_flags =
    ImGuiWindowFlags_NoResize |
    ImGuiWindowFlags_NoCollapse |
    ImGuiWindowFlags_NoScrollbar |
    ImGuiWindowFlags_AlwaysAutoResize;

file_browser_modal::file_browser_modal(const char* title) :
    m_title(title),
    m_oldVisibility(false),
    m_selection(0),
    m_currentPath(fs::current_path()),
    m_currentPathIsDir(true) {

}

// Will return true if file selected.
const bool file_browser_modal::render(const bool isVisible, std::string& outPath) {
    bool result = false;

    if (m_oldVisibility != isVisible) {
        m_oldVisibility  = isVisible;
        //Visiblity has changed.

        if (isVisible) {
            //Only run when the visibility state changes to visible.

            //Reset the path to the initial path.
            m_currentPath = fs::current_path();
            m_currentPathIsDir = true;

            //Update paths based on current path
            get_files_in_path(m_currentPath, m_filesInScope);

            //Make the modal visible.
            ImGui::OpenPopup(m_title);
        }

    }

    bool isOpen = true;
    if (ImGui::BeginPopupModal(m_title, &isOpen, modal_flags)) {

        if (ImGui::ListBox("##", &m_selection, vector_file_items_getter, &m_filesInScope, m_filesInScope.size(), 10)) {

            //Update current path to the selected list item.
            m_currentPath = m_filesInScope[m_selection].path;
            m_currentPathIsDir = fs::is_directory(m_currentPath);

            //If the selection is a directory, repopulate the list with the contents of that directory.
            if (m_currentPathIsDir) {
                get_files_in_path(m_currentPath, m_filesInScope);
            }

        }

        //Auto resize text wrap to popup width.
        ImGui::PushItemWidth(-1);
        ImGui::TextWrapped(m_currentPath.string().data());
        ImGui::PopItemWidth();

        ImGui::Spacing();
        ImGui::SameLine(ImGui::GetWindowWidth() - 60);

        // Make the "Select" button look / act disabled if the current selection is a directory.
        if (m_currentPathIsDir) {

            static const ImVec4 disabledColor = { 0.3f, 0.3f, 0.3f, 1.0f };

            ImGui::PushStyleColor(ImGuiCol_Button, disabledColor);
            ImGui::PushStyleColor(ImGuiCol_ButtonActive, disabledColor);
            ImGui::PushStyleColor(ImGuiCol_ButtonHovered, disabledColor);

            ImGui::Button("Select");

            ImGui::PopStyleColor();
            ImGui::PopStyleColor();
            ImGui::PopStyleColor();

        } else {

            if (ImGui::Button("Select")) {
                ImGui::CloseCurrentPopup();

                outPath = m_currentPath.string();
                result = true;
            }

        }

        ImGui::EndPopup();

    }

    return result;
}

use case:

static imgui_ext::file_browser_modal fileBrowser("Import");

// Had to use this awkward approach to get the menu item to open the pop-up modal.
bool isImportClicked = false;
if (ImGui::BeginMainMenuBar()) {
    if (ImGui::BeginMenu("File")) {
        if (ImGui::MenuItem("Import")) {
            isImportClicked = true;
        }
        ImGui::EndMenu();
    }
    ImGui::EndMainMenuBar();
}


std::string path;
if (fileBrowser.render(isImportClicked, path)) {
    // The "path" string will hold a valid file path here.
}
\$\endgroup\$
8
\$\begingroup\$

One of the things I'm specifically wondering, is it ok/proper to have static helper functions in the .cpp file if they are not member functions? (should they be member functions and should they even be static?)

Yes, it is perfectly good. In fact, if something can be written using only the public interface, it should be written as a non-member.

Yes, functions used only in one CPP file should be static. In general, use an anonymous namespace for anything (including templates) used only in the one file.

Header

#include <experimental/filesystem>
#include <filesystem>

Both? Shouldn’t the regular one not exist if the experimental one does?

// With Visual Studio compiler, filesystem is still "experimental"
namespace fs = std::experimental::filesystem;

BTW, the experimental/filesystem has some differences from the standard. You can download the preview of the new version for free. I believe filesystem is conforming now, but have not verified that yet.

Yes! Very good. You did not make the common mistake of spilling the entire namespace into global space, and you abstracted where the filesystem is coming from. This can go inside the namespace, though. You are leaking this to users of the header file.

namespace imgui_ext {

Very good, putting your stuff in a namespace.

const struct file {
    std::string alias;
    fs::path path;
};

Didn’t that give you a compiler warning? Are you paying attention to any warnings and eliminating them at level 3? There is no variable being declared so the const doesn’t do anything here. Did you mean something else?

class file_browser_modal final {

There are no virtual functions declared, so final is unnecessary and misleading. (C++ is not Java)

static const int modal_flags;

I think this line is left over from an earlier edit. You are using a non-member variable of the same name in the CPP file instead. It is neither given any value nor defined anywhere.

const char* m_title;

What are you doing with a char*? I don’t know if it’s correct of advised yet, having only read to this point. But assuming you do indeed want a pointer to a primitive string info owned elsewhere, use a sgl::zstring (⧺SL.str.3)or std::string_view.

At this point, I wonder if the file struct can be nested inside file_browser_modal. No big deal if it’s easier to use this way, as it’s still inside imgui_ext.

rule of 5 ?

Now, you have a char* member, so I wonder how you copy, move, or delete the object? You did not define any special member functions.

file_browser_modal(const char* title);

The constructor ought to take a string_span or zstring as mentioned for the data member.

render

const bool render(const bool isVisible, std::string& outPath);

You are rare in over using const! The const on the return value doesn’t do anything. Did you mean to make this a const member instead?

The const on the isVisible parameter does not mean anything to the caller. You should leave it out here. You can add it in the definition where the body is; this does not change the signature.

I worry about outPath. Good that I can see it is meant to be an “out” parameter from the naming! But don’t make “out” parameters (⧺F.20.

I’ll return to this when I see what it does.

CPP file

regarding static

Putting static on a single function has been re-welcomed as just fine, in C++17. But you have several in a bunch, so use an anonymous namespace instead (which is still preferred).

static void get_files_in_path(const fs::path& path, std::vector<file>& files) {
    files.clear();

Don’t use “out” parameters to return information.

If returning files, the NRVO will be just as efficient (if not more) than what you wrote.

static std::vector<file> get_files_in_path(const fs::path& path) 
{
    std::vector<file> files;
      ⋮
    return files;
}

use auto (almost everywhere)

for (const fs::directory_entry& dirEntry : fs::directory_iterator(path)) {

It is very easy to not get the variable type exactly right, and end up copying each element when you meant to be referencing it, with no compiler warnings. So, it is especially important to use auto in a for loop.

static const int clamp_size_t_to_int(const size_t data) {
    static const int max_int = std::numeric_limits<int>::max();
    return static_cast<int>(data > max_int ? max_int : data);
}

Again, very good for thinking about narrowing issues.
It is said that “constexpr is the new static const”. So use it for max here.

BTW, just what will the promotion rules do if size_t is the same size as int (e.g. 4 bytes)? You should get a compiler warning here, so I suspect this is a lesson for you: as mentioned earlier, pay attention and eliminate warnings at level 3 (on MSVC).

static bool vector_file_items_getter(void* data, int idx, const char** out_text) {

The usual: don’t use an “out” parameter. I assume the void* is due to this being data kept by the GUI framework? Making this nicer is more advanced than you need right now. But use auto in the next line; you are naming the type twice.

I’m guessing the use of a pointer rather than the std::string or somesuch is another artefact of the GUI framework. It would be good if functions that need such “bad C++ due to API” are clearly labeled as such.

inline static constexpr int modal_flags =

inline or static? One or the other. constexpr implies static which is what you want here. (inline if used in a header, to get the linker to merge all the copies.)

file_browser_modal::file_browser_modal(const char* title) :

Good that you use an initialization list!
You are, however, using ye olde syntax. Use curlys, not parens, for initialization.

Since you only have the one constructor, “all” of them use the same values for most of the members. Put them in the class definition as direct initializers on the members, instead.

    ⋮
bool m_oldVisibility = false;
int m_selection = 0;
    ⋮

render

Same lessons explained already. Now how do you make a return value that can be a string or nothing? std::optional<std::string>.

This function is too long.
The body of “visibility state changes to visible” can go into a helper function. The body of the next if statement is huge; it can probably go into its own function which can then be split up into others.

Every time you have a comment header explaining what the next block of code does, you are “naming” that block of code and indicating it has a purpose distinct/different from other such blocks. A block of code with a cohesive single purpose and a name— what’s that normally called?

Closing remarks

Your code is a lot better than most of the code posted here. There are a lot of things about how-to that you already know. Your fluency in the language indicates that you are new to it, but you are learning from more up-to-date material compared with a lot of newcomers’ code, so good show there! Is there a book or online resource you recommend for this?

\$\endgroup\$
  • 1
    \$\begingroup\$ I would just like to say, Thank you again for taking the time to go through my work and provide high quality, and much appreciated feedback! You mentioned checking the warnings, which I have been. Visual Studio is set to "Level3 (/W3)" for C/C++ Warning level. That said I've not seen any of the warnings you mentioned I should be seeing based on the code above. As for your closing remark, no, everything I've learned so far in regards to c++ has just been googling around each time I hit a snag (plus these code reviews). \$\endgroup\$ – Hex May 16 '18 at 22:28
2
\$\begingroup\$

Strictly from the point of view of using dear imgui, you can get rid of the awkward vector_file_items_getter() by displaying the listbox yourself.

If you look at the source code for ImGui::ListBox() you'll find it is essentially doing:

if (!ImGui::ListBoxHeader(...))
    return false;
bool value_changed = false;
for each item (...)
    value_changed |= ImGui::Selectable(...);
ImGui::ListBoxFooter();
return value_changed;

This is all public API stuff. From looking at this pattern you'll see that you can pass in your own strings/items data to Selectable() and manage your selection however you want (so you can use a size_t if you prefer that, handle multi/custom selection patterns, and not have to fit your data through that awkward callback etc.). ListBox() is a short-cut that makes sense to use with a lambda and possibly capturing but it's so much more flexible to use the custom one.


(PS: Code review websites are both great and also can be very misleading. Many productive and talented programmers would have vastly different suggestions and wouldn't agree to the wall of text above. I'm not going to get into details I just want to point out that by the nature of those communities, they represent only a narrow fraction of the possible programming wisdom one could receive and share. Stay curious and open-minded to other sources of programming wisdom.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.