UI: Simple ncurses window with ls-like output #11

p4vook · 2020-12-07T08:20:39Z

This PR also adds the abstract classes which are intended to be a base classes for futher text windows and windows' contents (buffers)

Basic ls in ncurses window - Require C++17 because of std::filesystem usage - Draw sorted listing of cwd in main window Signed-off-by: Pavel Kalugin <paul.kalug@gmail.com>

leha-bot

Seems OK, but we should fix some parts which I mentioned

CMakeLists.txt

include/buffer.hpp

leha-bot · 2020-12-13T22:25:10Z

src/CMakeLists.txt


 target_include_directories(rang PUBLIC ${CURSES_INCLUDE_DIRS})
-target_link_libraries(rang PUBLIC ${CURSES_LIBRARIES})


I think that we should make a CMake module which will export the Curses imported target and use it (like
target_link_libraries(rang PUBLIC Curses::Curses). This approach is more clear and we could avoid the cumbersome adding of include_directories and libraries if we declare it as INTERFACE in our imported target: target_include_directories(Curses::Curses INTERFACE ${CURSES_INCLUDE_DIRS}).

src/console_io/include/window.hpp

src/rang.cpp

Signed-off-by: Pavel Kalugin <paul.kalug@gmail.com>

Signed-off-by: Pavel Kalugin <pavel@pavelthebest.me>

leha-bot

Nice job, it looks OK for me, but it is required some changes, refactors and git history rewrites.

leha-bot · 2021-01-11T09:29:30Z

include/buffer.hpp

+        virtual void update() = 0;
+};
+
+class dummy : buffer


This class does not contain any logic, therefore it remains abstract, so we couldn't use it as "dummy" class.
Seems it needs to implement the stub update() method.

leha-bot · 2021-01-11T09:32:48Z

src/CMakeLists.txt

 target_include_directories(rang PUBLIC ${CURSES_INCLUDE_DIRS})
-target_link_libraries(rang PUBLIC ${CURSES_LIBRARIES})
+target_link_libraries(rang PUBLIC ${CURSES_LIBRARIES} ${Boost_LIBRARIES} console_io)


I also prefer the FindBoost's exported targets here: Boost::system Boost::thread.

leha-bot · 2021-01-11T09:35:19Z

src/console_io/include/console_io.hpp

+#include "base.hpp"
+#include "window.hpp"
+
+#undef timeout


I suggest to add comment why this #undef directive is here or drop it, if it doesn't use.

leha-bot · 2021-01-11T09:42:49Z

src/console_io/src/window.cpp

+{
+        win_ptr = newwin(size_y, size_x, offset_y, offset_x);
+        if (win_ptr == nullptr) {
+                throw 1; // TODO errors


I suggest to use some type with exception information (maybe named like window_error, or some similar name) derived from std::exception or std::runtime_error (or more semantic standard exception type). This exception type should contain an actual error info , implements the what() method and possibly another information (std::error_code???).

leha-bot · 2021-01-11T09:44:13Z

src/evloop.cpp

+#include <iostream>
+#include <ncurses.h>
+
+#undef timeout


It also should be documented, why this undef is here.

leha-bot · 2021-01-11T09:45:44Z

src/evloop.cpp

+using namespace rang;
+
+event_loop::event_loop()
+    : signals(io_context, SIGINT, SIGTERM), // handle SIGINT and SIGTERM signals


As signals setup is a global operation, it should be mentioned in programmer's documentation to avoid some excessive case then we add a library (e.g. Google Breakpad for stacktraces and error reports) which could also set these signals' handlers.

leha-bot · 2021-01-11T09:46:57Z

src/rwindow.cpp

+void window::scroll_viewport(int shift)
+{
+        if (offset_y + shift >= tied_buf.contents.size() || offset_y + shift < 0) {
+                throw 1; // todo errors


Same commend as in prev. "throw 1;" line.

leha-bot · 2021-01-11T09:48:00Z

src/rang.cpp

+        int ch;
+        main.refresh();
+        rang::event_loop loop;
+        std::function<void(int)> reaction_func = [&](int ch) {


We should move this impl detail later to main window class (or some similar).

Yes, I think that there should be some "Cursor" class which manages what window is currently active and some "Reactor" class, which combines window's keybindings as a single handle function.

berkus · 2021-04-08T18:27:29Z

src/rang.cpp

-        endwin();                      /* End curses mode		  */
+        console_io::ncurses root;
+        cbreak();              /* Line buffering disabled, Pass on
+                                *everty thing to me */


leha-bot

Looks good, but some headers and classes seems to looks like the old POSIX names when the function identifiers were short and without some vovels. I suggest to change some names, but OK with current names.

p4vook and others added 4 commits December 7, 2020 11:16

component: UI

0c5bfaf

Basic ls in ncurses window - Require C++17 because of std::filesystem usage - Draw sorted listing of cwd in main window Signed-off-by: Pavel Kalugin <paul.kalug@gmail.com>

Apply formatting changes

feab7cd

fixup #1

27167fc

fixup #2

b4a7b03

p4vook marked this pull request as draft December 7, 2020 08:31

p4vook added 2 commits December 7, 2020 11:39

fixup #3

13fbf3e

fixup #7

821a8bd

p4vook force-pushed the ncurses-ls branch from 2db0c05 to 821a8bd Compare December 7, 2020 09:38

p4vook added 2 commits December 7, 2020 12:43

fixup #8

d91556c

fixup #9

ce45cba

p4vook requested a review from leha-bot December 7, 2020 09:47

p4vook and others added 6 commits December 7, 2020 14:32

work

524826a

Apply formatting changes

2cde7b1

even more work

ab403cb

move everything to namespaces. control destruction order

3a243de

new window abstraction

629d678

scrolling. exceptions

01fcb87

leha-bot requested changes Dec 13, 2020

View reviewed changes

leha-bot self-assigned this Dec 14, 2020

leha-bot changed the title ~~component: UI~~ UI: Simple ncurses window with ls-like output Dec 14, 2020

p4vook and others added 6 commits December 16, 2020 12:00

virtual destructor for buffer

c93400d

Signed-off-by: Pavel Kalugin <paul.kalug@gmail.com>

target-wide C++ standard

3042533

Signed-off-by: Pavel Kalugin <paul.kalug@gmail.com>

remove obsolete color initialization

20b23df

Signed-off-by: Pavel Kalugin <pavel@pavelthebest.me>

forbid modifying of size_x and size_y

69b5ef7

Signed-off-by: Pavel Kalugin <pavel@pavelthebest.me>

Apply formatting changes

e5f148c

event loop

8c23f55

Signed-off-by: Pavel Kalugin <pavel@pavelthebest.me>

p4vook force-pushed the ncurses-ls branch from ff154f4 to 8c23f55 Compare December 31, 2020 12:20

p4vook added 3 commits December 31, 2020 15:36

fix github actions

8afbfc9

Signed-off-by: Pavel Kalugin <pavel@pavelthebest.me>

fixup

3094bdf

fixup #2

a8ffa77

fix boost required version

e612460

p4vook requested a review from leha-bot December 31, 2020 12:47

leha-bot requested changes Jan 11, 2021

View reviewed changes

berkus reviewed Apr 8, 2021

View reviewed changes

leha-bot approved these changes Apr 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Simple ncurses window with ls-like output #11

UI: Simple ncurses window with ls-like output #11

p4vook commented Dec 7, 2020 •

edited by leha-bot

Loading

leha-bot left a comment

leha-bot Dec 13, 2020

leha-bot left a comment

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

leha-bot Jan 11, 2021

p4vook Jan 11, 2021 •

edited

Loading

berkus Apr 8, 2021

leha-bot left a comment


		target_include_directories(rang PUBLIC ${CURSES_INCLUDE_DIRS})
		target_link_libraries(rang PUBLIC ${CURSES_LIBRARIES})

UI: Simple ncurses window with ls-like output #11

Are you sure you want to change the base?

UI: Simple ncurses window with ls-like output #11

Conversation

p4vook commented Dec 7, 2020 • edited by leha-bot Loading

leha-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leha-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p4vook Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leha-bot left a comment

Choose a reason for hiding this comment

p4vook commented Dec 7, 2020 •

edited by leha-bot

Loading

p4vook Jan 11, 2021 •

edited

Loading