11
\$\begingroup\$

Cat

Hello everyone! I am working on this TUI framework which aims to abstract the ncurses library. I was wondering how understandable it is, and what I can improve! Any feedback is welcome!!

My framework aims to be easily extendible, while also being able to do what you want to do quickly so you can focus on your task. As much behavior as possible should be redefinable by the user, to suit as many usecases as possible, though I cannot deny that the main focus lies on simple-ish interfaces, and not draw expensive projects such as games.
The source code and more examples can be found here (github).

The following code allows you to open a window using a and remove then with the r key. These windows contain their number, and are ordered horizontally. Pressing q will exit the program. You can move the cursor from window to window using the arrow keys.
The framework a lot of features, but showing them all would only clutter the example, so I left it simple-ish.

#include <cat/cat.hpp>

/* displays usage of the `cat::lib::HorizontalContainer` */

int main() {
        // creating our container
    cat::lib::HorizontalContainer* container;
    
        // initializing with max hight/width
    cat::async(cat::Priority::NORMAL, [&]() {
        container = cat::new_container<cat::lib::HorizontalContainer>({0,0},{cat::terminal::get_x(), cat::terminal::get_y()});
    });
        
        // define behaviour when applying 
        // changes to the container
    cat::async(cat::Priority::NORMAL, [&]() {
        container->on_apply([](cat::Window* win) {
            win->box();
            win->draw_buffer({1,1});
            win->redraw();
        });
    });
        
        // set a few key events
    cat::async(cat::Priority::HIGH, [&]() {
        cat::set_keymap('q', []() {
                // emit to the framework to shut down
            cat::signals::emit(
                    cat::signals::quit_signal
                );
        });
            
        cat::set_keymap({
                // create a new window 
            {'a', [&]() {
                cat::Window* win = container->add(cat::new_window());
                win->set_buffer();
                    // let the window display it's number
                win->get_buffer()->set(std::to_string(container->entries()));
                    
                    // if we are focused, take the cursor
                win->on_focus([](cat::Window* win, bool mode) {
                    if(mode) {
                        win->cursor({1,1});
                        win->redraw();
                    }
                });
                    
                    // apply changes
                container->apply();
                    
                    // if we are the first window, take focus
                if(container->entries() == 1)
                    win->focus();
            }},
                // remove last added window
            {'r', [&]() {
                    // remove only if there are windows left
                if(container->entries() != 0)
                    container->pop(container->back());

                container->apply();
            }},
                // refresh
            {'e', [&]() {
                container->apply();
            }}
        });
            
            // make it posible to switch between windows 
            // using the arrow keys
        cat::set_keymap(KEY_RIGHT, [&]() {
            for(size_t i = 0; i < container->entries(); ++i) {
                if(container->at(i)->focused()) {
                    if(i+1 == container->entries()) break;
                        
                        // move the focus one to the left
                    container->at(i)->lose_focus();
                    container->at(i+1)->focus();
                    break;
                }
            }
        });
        cat::set_keymap(KEY_LEFT, [&]() {
            for(size_t i = 0; i < container->entries(); ++i) {
                if(container->at(i)->focused()) {
                    if(i == 0) break;
                        
                        // move the focus one to the right
                    container->at(i)->lose_focus();
                    container->at(i-1)->focus();
                    break;
                }
            }
        });
    });
    
        // apply some settings
    cat::cycle({ .tick_sleep = 10, .color = false, .stuck_cursor = true });
}

Some notable files I want to attach:

window.hpp, my Window class

#ifndef CAT_WINDOW_HPP
#define CAT_WINDOW_HPP

#include <ncurses.h>
#include <string>
#include <vector>
#include <set>

#include "vector.hpp"
#include "input.hpp"
#include "id.hpp"
#include "buffer.hpp"
#include "drawcalls.hpp"
#include "effects.hpp"
#include "positional.hpp"
#include "cursor.hpp"

namespace cat {

extern void redraw();
 
class Window : public Positional {
public:
    using redraw_event = std::function<void(Window*)>;
    using focus_event = std::function<void(Window*,bool)>;
protected:
    WINDOW* ncurses_window = nullptr;

    bool redraw_request = false;
    std::vector<Window*> syncs;

    bool is_focused = false;
    bool is_hidden = false;

    using key_checker_type = std::function<bool(const key&)>;
    using key_handler_type = std::function<void(const key&)>;
    keymap win_keymap;
    key_handler_type key_handler;
    key_checker_type key_check;

    id_type id;

    Buffer* buffer = nullptr;

    std::vector<redraw_event> redraw_events;
    std::vector<focus_event> focus_events;
public:

    Window() = delete;
    Window(const Vector2& position, const Vector2& resolution)
    : Positional(position, resolution) {
        ncurses_window = newwin(resolution.y,  resolution.x, position.y, position.x);
        id = new_id();
    }

    virtual ~Window() {
        if(ncurses_window)
            delwin(ncurses_window);
        if(buffer)
            delete buffer;
    }
   
    virtual const id_type& get_id();

    virtual Window& clear();
    virtual Window& redraw();
    virtual Window& move(const Vector2& position) override;
    virtual Window& resize(const Vector2& resolution) override;
    virtual Window& box(int chtype = 0);
    virtual Window& background(const effect::color& color);
    virtual Window& cursor(const Vector2& position, bool absolute = false);
    virtual Window duplicate();

    virtual Window& adopt(Window* win);
    virtual Window& sync();
    virtual Window& trigger_syncs();

    virtual Window& hide(bool status = true);
    virtual Window& unhide();
    virtual bool hidden();

    virtual Window& focus(bool status = true);
    virtual Window& lose_focus();
    virtual bool focused();

    template<typename TBuffer = Buffer>
        requires ( std::is_base_of_v<Buffer,TBuffer> )
    inline Window& set_buffer(Renderer renderer = Renderer::null, Composer composer = Composer::null) {
        if(buffer)
            delete buffer;
        buffer = new TBuffer(this);
        buffer->composer() &= composer;
        buffer->renderer() &= renderer;
        return *this;
    }

    template<typename TBuffer = Buffer>
        requires ( std::is_base_of_v<Buffer,TBuffer> )
    inline TBuffer* get_buffer() {
        return dynamic_cast<TBuffer*>(buffer);
    }
 
    virtual Window& set_key(const key& skey, const key_event& event);
    virtual Window& unset_key(const key& skey);
    virtual Window& set_key_handler(const key_checker_type& check, const key_handler_type& handler); 
    virtual const key_handler_type& get_key_handler();
    virtual const key_checker_type& get_key_check();
    virtual bool do_key_check(const key& skey);
    virtual bool has_key(const key& skey);
    virtual key_event& get_key_event(const key& skey);

    template<typename ...Tfargs>
    inline Window& draw_at(const Vector2& position, const CatString& fmstr, const Tfargs&... fargs) {
        cat::draw(position, ncurses_window, fmstr, fargs...);
        return *this;
    }

    template<typename ...Tfargs>
    inline Window& draw(const CatString& fmstr, const Tfargs&... fargs) {
        cat::draw(ncurses_window, fmstr, fargs...);
        return *this;
    }

    virtual Window& draw_buffer(const Vector2& offset = {0,0});

    virtual Window& on_redraw(const redraw_event& event);
    virtual Window& on_focus(const focus_event& event);

    template<typename TWindow>
        requires ( std::is_base_of_v<Window,TWindow> )
    inline bool viable_as() const {
        return !!dynamic_cast<const TWindow*>(this);
    }
    
    friend void redraw();
    friend void move_cursor(Window*, const Vector2&);
};

}

#endif

buffer.hpp, designed to represent the content of a window

#ifndef CAT_BUFFER_HPP
#define CAT_BUFFER_HPP

#include <string>
#include <functional>

#include "string.hpp"

namespace cat {

class Buffer;
using image_type = CatString;
using snippet_type = CatString;

template<typename Tcomp>
inline Tcomp compose(const Tcomp& r0) {
    return r0;
}

template<typename Tcomp, typename... TcompP>
inline Tcomp compose(const Tcomp& r0, const Tcomp& r1, const TcompP& ...tail) {
    return
        [=](const typename Tcomp::param_type& image, Buffer* buffer)
            ->typename Tcomp::return_type 
        { return r0(compose<Tcomp>(r1,tail...)(image, buffer), buffer); };
}


template<typename Treturn_type, typename Tparam_type>
struct Processor {
    using return_type = Treturn_type;
    using param_type = Tparam_type;
    using fn_type = std::function<return_type(const param_type&, Buffer*)>;

    fn_type fn;

    return_type operator()(const param_type& data, Buffer* buffer) { return fn(data, buffer); }
    return_type operator()(const param_type& data, Buffer* buffer) const { return fn(data, buffer); }


    template<typename Fn>
        requires ( !std::is_same_v<std::remove_cvref_t<Fn>,Processor> )
    Processor(Fn&& fn): fn(std::forward<Fn>(fn)) { }
    
    Processor(const fn_type& fn): fn(fn) { }
    Processor(fn_type&& fn): fn(fn) { }
    Processor() { }
    Processor& operator=(const Processor&) = default;
    
    Processor operator&(const Processor& processor) {
        return compose(*this, processor);
    }
    Processor& operator&=(const Processor& processor) {
        return *this = operator&(processor);
    }

    template<typename Fn>
        requires ( !std::is_same_v<std::remove_cvref_t<Fn>,Processor> )
    Processor& operator=(Fn&& fn) { 
        this->fn = std::forward<Fn>(fn);
        return *this;
    }

    Processor& operator=(const fn_type& fn) {
        this->fn = fn;
        return *this;
    }

    const static Processor null; 
}; 

template<typename T, typename U>
inline const Processor<T,U> Processor<T,U>::null = [](const Processor::param_type& param, Buffer*) { return (Processor::return_type)param; };

using Renderer = Processor<image_type,image_type>;
using Composer = Processor<image_type,snippet_type>;

class Window;

class Buffer {
protected:
    std::string raw;
    Window* assoc_window = nullptr;

    Composer i_composer = [](const Composer::param_type& data, Buffer*) { return (Composer::return_type)data; };
    Renderer i_renderer = [](const Renderer::param_type& data, Buffer*) { return (Renderer::return_type)data; };

    virtual snippet_type generate_snippet(); 
public:

    Buffer() = delete;
    Buffer(Window* win): assoc_window(win) { }

    virtual Window* window();

    virtual Buffer& set(const std::string& raw);
    virtual Buffer& append(const std::string& raw);
    virtual std::string& get_raw();

    virtual Buffer& clear();

    virtual Composer& composer();
    virtual Renderer& renderer();

    virtual image_type display();

    virtual ~Buffer() { }

    friend class Window;
};

} // namespace cat

#endif

I also attached a picture of what i looks like in my terminal, don't get confused by the green color, normally it's white text (I have some settings and stuff)

Thank you!

Cat_Example

\$\endgroup\$
7
  • 4
    \$\begingroup\$ Welcome to the site! This doesn’t seem to be a complete program that compiles. Could you please also post at least the necessary sections of the header file? \$\endgroup\$
    – Davislor
    Commented Sep 3, 2023 at 18:55
  • \$\begingroup\$ Heya @Davislor! Sorry but the whole framework is around 2300 lines, <cat/cat.hpp> only checks for C++20 and includes the rest of the headers. You can view them on the github page, but I was more aiming for design and best practice feedback, sorry I am new to this side, and the help-pages were not really helpful. On it's own, if you have the library installed, it is indeed a complete program, but I get what you mean, sorry. qwq \$\endgroup\$
    – Labricecat
    Commented Sep 3, 2023 at 19:09
  • 4
    \$\begingroup\$ We can’t review code that you don’t post here. If there’s a small, relatively self-contained, section you want to ask about, that would be great. \$\endgroup\$
    – Davislor
    Commented Sep 3, 2023 at 19:17
  • 4
    \$\begingroup\$ There's some happy medium between what's here now, and 40+ files. Try choosing modules that are relatively easy to review in isolation (if they aren't easy to review in isolation, that's a smell of its own). \$\endgroup\$
    – Reinderien
    Commented Sep 3, 2023 at 19:32
  • 3
    \$\begingroup\$ Alright. I will pick out some more important/essential header files, and add them to my initial post. Thanks for the information. As I said, I am new to this page, but I try my best. \$\endgroup\$
    – Labricecat
    Commented Sep 3, 2023 at 19:35

2 Answers 2

7
\$\begingroup\$

You appear to be asking for a review the part of the code you posted, not the library itself. (ETA: I wrote this before you posted any of the library code.) I’m not sure how useful that will be, but a few things do jump out at me.

Be Careful of [&] Lambda Captures

In this instance, you are implicitly capturing a local variable, container, by reference, and then passing the closure as a callback. This works for main, because all local variables declared inside main have static storage, but if you tried this from any other function, and the closure could escape the function, you would either need to declare the local variable static, capture a global, or capture by copy (if that would work; it seemingly would not in this example).

List Your Captured Variables

It’s good practice to list your captures explicitly, such as [&container]. This makes it much more obvious, to both the maintainer and the compiler, when you do introduce a dangling reference, or a dependency that should not be there.

Consider Alternatives to Lambdas

Lambdas are great for writing short functions inline. For longer functions with multiple nested blocks, or for callbacks that you will need to register more than once, you might prefer a named helper function.

You should also, if the code doesn’t already, accept a closure of a function and its arguments, or an object and one of its member functions, with std::bind.

Consider a Familiar Interface

I don’t know whether it’s currently possible to look up or replace a keymapping, but conceptually, this is very similar to a std::map or std::unordered_map whose key is a literal key on the keyboard and whose values are std::function objects. You might therefore want to implement similar operations with similar names, to reduce the learning curve.

\$\endgroup\$
3
  • \$\begingroup\$ Heya! Thanks for the feedback! You cannot look up keymapping, but you can simply override it. Under the hood it is indeed using an std::map! Everywhere I pass a lambda it takes in an std::function, I don't particularly know the difference to std::bind, but passing functions should not be a problem. Probably I should look into it! Per se, you use async() or process() inside of your main and call cycle(), hence leave the rest to the framework. I did not see a problem with using [&], but I suppose because they're pointers, [=] should do fine too! Thanks for the feedback! \$\endgroup\$
    – Labricecat
    Commented Sep 3, 2023 at 19:46
  • 1
    \$\begingroup\$ @Labricecat It appears that [=] would not work here, because the closure modifies the data structure by reference. You would not want to copy the entire thing, even if it did work. \$\endgroup\$
    – Davislor
    Commented Sep 3, 2023 at 23:01
  • \$\begingroup\$ Oh I did not see the first async() call, you are right. \$\endgroup\$
    – Labricecat
    Commented Sep 4, 2023 at 8:11
6
\$\begingroup\$

Unfortunately you've only shown some of the header files, but no concrete implementation of your TUI library, so I cannot meaningfully review that. However, we can look at your intended use of the TUI.

Usage

Your example main() function looks quite complex, and I'd rather write simpler code than that. Some issues I see are:

  • Overuse of lambdas (see Davislor's answer).
  • Everything is a pointer. It would be much nicer if things were passed by value or reference. Why was it necessary to use cat::new_container<> instead of just writing the following?
    cat::lib::horizontalContainer container({0, 0}, cat::terminal::get_size());
    
    Also, who is going to delete container? It looks like a memory leak to me.
  • Async. What is this doing here? It looks very unsafe as well: if these things are really executed asynchronously, what guarantees that container will be initialized before the other async calls use it? Especially since the third async call apparently has a higher priority than the first two.
  • Missing iterator interface. The for-loops are all very C-like, instead I would want to be able to write:
    cat::set_keymap(KEY_LEFT, [&container]() {
         std::ranges::adjacent_find(container, [](auto& left, auto& right) {
             if (right.focused()) {
                 right.lose_focus();
                 left.focus();
                 return true;
             }
             return false;
         });
    });
    
    Note how there are no arrows here, and no need to count anything.
  • Unnecessarily different from the standard library. It would be much nicer if your containers use the same interface as the STL's containers do. Instead of container->pop(container->back()), I'd rather write container->pop_back(). It's one less difference I have to learn and remember. Also see Davislor's remarks about familiar interfaces.
  • Still quite low-level. You say you want a simple interface so "you can focus on your task", but I see that you have to deal with lots of low-level things like calling redraw() and apply(). Why do I have to manually call lose_focus() if I am going to focus() another window?

Naming things

While I see that naming is done in a rather consistent way, some names are badly chosen. For example:

  • set_keymap(): it doesn't set the whole map. Maybe set_key_callback() or on_key() would be better names.
  • box(): first, prefer using verbs for functions, and only use nouns for variables or types. So if I were to guess at what this function might do, perhaps it should be called draw_box()? What does that parameter do?
  • background(), cursor(): similar, make them verbs.
  • draw_at(): this is meant to print formatted text, so perhaps print_at() would be better.
  • cycle(): the comment above it says it "apply some settings". But then I would expect the function to be called apply_settings() or something similar. If it does more than that, it's hard to tell from the function name.
\$\endgroup\$
7
  • \$\begingroup\$ Thanks for the feedback! a.) It does not memory leak as you don't own the instance, it's stored globally and deleted properly. b.) async's are guaranteed to execute in the order they got called, the last one has priority, but only sets some key events. c.) yeah I should add more STL-like functionality d.) redraw() / apply() are for optimization reasons, thogh redraw() only marks the window to be redrawn next frame, so calling it multiple times does nothing. e.) box() is the same as ncurses box() and draws a .. box f.) cycle() starts the framework loop, there is in fact a apply_settings() \$\endgroup\$
    – Labricecat
    Commented Sep 4, 2023 at 10:41
  • \$\begingroup\$ Sorry the character limit got me. What I want to add is, that I indeed should use more verbs, though I think box() is a verb? And I thought it was consistent for people that know ncurses. Thanks again for the feedback! And I am sorry I could not include that much source code! What would you suggest as alternative to the lambdas though? \$\endgroup\$
    – Labricecat
    Commented Sep 4, 2023 at 10:42
  • 1
    \$\begingroup\$ a) This is fine, but I'd still be a bit weary using raw pointers for this. b) Still, why use async in the first place? It doesn't seem necessary here. d,e) It is unclear what the goal is of your library. Do you want to make a simple C++ wrapper for ncurses? In that case, sure it's fine to copy its interface where possible. But if you want to create your own, simpe-to-use TUI on top of ncurses, I would not copy all the idiosyncracies of ncurses. I also wouldn't want to learn two libraries (ncurses and yours) just to be able to use one TUI library. \$\endgroup\$
    – G. Sliepen
    Commented Sep 4, 2023 at 11:19
  • \$\begingroup\$ As for the lambdas: I would either just use regular functions, but if you need to capture some variable, you can still use a lambda, but keep its body small. For example, write cat::set_keymap('a', [&container]() { add_window(container); }), and create a regular function add_window() that takes a pointer to the container as a parameter. This helps keep the code readable. \$\endgroup\$
    – G. Sliepen
    Commented Sep 4, 2023 at 11:23
  • \$\begingroup\$ I think I should explain async() and what the goal of my framework is. async() takes in a priority and a function. This function gets pushed onto a queue and next frame this queue gets worked down and freed. (As opposed to a process() do the same thing but stay). The general idea of this framework is not to wrap ncurses. You define your behavious using events, and execute code once the framework starts. After that you leave it to the framework, all you as user should do is call async()s/process()s and cycle() afterwards. \$\endgroup\$
    – Labricecat
    Commented Sep 4, 2023 at 11:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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