[netsniff-ng] Re: [PATCH v2 07/11] ui: Implement UI table for flows printing
On 2016-04-21 at 14:04:05 +0200, Vadim Kochan wrote:
> On Thu, Apr 21, 2016 at 2:53 PM, Tobias Klauser wrote:
> > On 2016-04-17 at 19:31:30 +0200, Vadim Kochan wrote:
> >> Add new module ui.c which is responsible to render
> >> different kinds of UI widgets - tables, etc.
> >>
> >> Implemented generic API for print table-like list of elements.
> >> This table API might be used for print flows in curses or text mode.
> >>
> >> Signed-off-by: Vadim Kochan
> >
> > Looks good in general, a few minor nits below.
> >
> >> ---
> >> ui.c | 142
> >> +++
> >> ui.h | 53 +
> >
> > How about calling the module ui_table, which is a bit more specific?
> > Also all function names already start with "ui_table_".
>
> I thought that ui.c might keep other UI helpers (widgets), so I called
> it just "ui", so if
> there will be some other UI widget, then ui_xxx naming will be still OK.
Hm, ok. I just think UI might be a bit too generic (also given that it
is only used by flowtop). But we can still rename/split later on if need
be...
> >> +static struct ui_col *ui_table_col_get(struct ui_table *tbl, uint32_t id)
> >> +{
> >> + struct ui_col *col;
> >> +
> >> + list_for_each_entry(col, &tbl->cols, entry) {
> >> + if (col->id == id)
> >> + return col;
> >> + }
> >> +
> >> + /* Should not happen in normal case */
> >> + panic("Invalid column id %u\n", id);
> >
> > I'd rather omit the panic() here, return NULL and gracefully handle (if
> > possible) NULL returns in the callers.
> >
>
> Hm, for me it looks like more programmers's issue so may be
> bug_on(...) will be better ?
> But I will still think on it more ...
In my opinion we should never panic() based on a programming error but
only use it e.g. if syscalls fail. bug_on() is certainly better suited
here.
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[netsniff-ng] Re: [PATCH v2 07/11] ui: Implement UI table for flows printing
On Thu, Apr 21, 2016 at 2:53 PM, Tobias Klauser wrote:
> On 2016-04-17 at 19:31:30 +0200, Vadim Kochan wrote:
>> Add new module ui.c which is responsible to render
>> different kinds of UI widgets - tables, etc.
>>
>> Implemented generic API for print table-like list of elements.
>> This table API might be used for print flows in curses or text mode.
>>
>> Signed-off-by: Vadim Kochan
>
> Looks good in general, a few minor nits below.
>
>> ---
>> ui.c | 142
>> +++
>> ui.h | 53 +
>
> How about calling the module ui_table, which is a bit more specific?
> Also all function names already start with "ui_table_".
I thought that ui.c might keep other UI helpers (widgets), so I called
it just "ui", so if
there will be some other UI widget, then ui_xxx naming will be still OK.
>
>> 2 files changed, 195 insertions(+)
>> create mode 100644 ui.c
>> create mode 100644 ui.h
>>
>> diff --git a/ui.c b/ui.c
>> new file mode 100644
>> index 000..d5fe1a7
>> --- /dev/null
>> +++ b/ui.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * netsniff-ng - the packet sniffing beast
>> + * Subject to the GPL, version 2.
>> + */
>> +
>> +#include "ui.h"
>> +#include "xmalloc.h"
>> +
>> +#include
>
> Global headers should come first, i.e.
>
> #include
>
> #include "ui.h"
> #include "xmalloc.h"
>
>> +void ui_table_init(struct ui_table *tbl)
>> +{
>> + memset(tbl, 0, sizeof(*tbl));
>> +
>> + getsyx(tbl->y, tbl->x);
>> +
>> + tbl->rows_y = tbl->y;
>> + tbl->width = COLS;
>> + tbl->col_pad = 1;
>> +
>> + INIT_LIST_HEAD(&tbl->cols);
>> +}
>> +
>> +void ui_table_uninit(struct ui_table *tbl)
>> +{
>> + struct ui_col *col, *tmp;
>> +
>> + list_for_each_entry_safe(col, tmp, &tbl->cols, entry)
>> + xfree(col);
>> +}
>> +
>> +void ui_table_pos_set(struct ui_table *tbl, int y, int x)
>> +{
>> + tbl->y = y;
>> + tbl->x = x;
>> + tbl->rows_y = y;
>> +}
>> +
>> +static struct ui_col *ui_table_col_get(struct ui_table *tbl, uint32_t id)
>> +{
>> + struct ui_col *col;
>> +
>> + list_for_each_entry(col, &tbl->cols, entry) {
>> + if (col->id == id)
>> + return col;
>> + }
>> +
>> + /* Should not happen in normal case */
>> + panic("Invalid column id %u\n", id);
>
> I'd rather omit the panic() here, return NULL and gracefully handle (if
> possible) NULL returns in the callers.
>
Hm, for me it looks like more programmers's issue so may be
bug_on(...) will be better ?
But I will still think on it more ...
>> +
>> +void ui_table_header_print(struct ui_table *tbl)
>> +{
>> + struct ui_col *col;
>> + int max_width = tbl->width;
>> + int width = 0;
>> +
>> + attron(tbl->hdr_color);
>> +
>> + mvprintw(tbl->y, tbl->x, "%-*.*s", max_width - tbl->x, max_width -
>> tbl->x, "");
>> + mvprintw(tbl->y, tbl->x, "");
>> +
>> + list_for_each_entry(col, &tbl->cols, entry) {
>> + __ui_table_row_print(tbl, col, col->name);
>> + width += col->len + tbl->col_pad;
>> + }
>> +
>
> Trailing whitespace (tab) here.
>
>> + mvprintw(tbl->y, width, "%*s", max_width - width, " ");
>> + attroff(tbl->hdr_color);
>> +}
>> diff --git a/ui.h b/ui.h
>> new file mode 100644
>> index 000..02d1da2
>> --- /dev/null
>> +++ b/ui.h
>> @@ -0,0 +1,53 @@
>> +#ifndef UI_H
>> +#define UI_H
>> +
>> +#include "list.h"
>> +
>> +#include
>> +#include
>
> Global headers should come first.
>
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
[netsniff-ng] Re: [PATCH v2 07/11] ui: Implement UI table for flows printing
On 2016-04-17 at 19:31:30 +0200, Vadim Kochan wrote:
> Add new module ui.c which is responsible to render
> different kinds of UI widgets - tables, etc.
>
> Implemented generic API for print table-like list of elements.
> This table API might be used for print flows in curses or text mode.
>
> Signed-off-by: Vadim Kochan
Looks good in general, a few minor nits below.
> ---
> ui.c | 142
> +++
> ui.h | 53 +
How about calling the module ui_table, which is a bit more specific?
Also all function names already start with "ui_table_".
> 2 files changed, 195 insertions(+)
> create mode 100644 ui.c
> create mode 100644 ui.h
>
> diff --git a/ui.c b/ui.c
> new file mode 100644
> index 000..d5fe1a7
> --- /dev/null
> +++ b/ui.c
> @@ -0,0 +1,142 @@
> +/*
> + * netsniff-ng - the packet sniffing beast
> + * Subject to the GPL, version 2.
> + */
> +
> +#include "ui.h"
> +#include "xmalloc.h"
> +
> +#include
Global headers should come first, i.e.
#include
#include "ui.h"
#include "xmalloc.h"
> +void ui_table_init(struct ui_table *tbl)
> +{
> + memset(tbl, 0, sizeof(*tbl));
> +
> + getsyx(tbl->y, tbl->x);
> +
> + tbl->rows_y = tbl->y;
> + tbl->width = COLS;
> + tbl->col_pad = 1;
> +
> + INIT_LIST_HEAD(&tbl->cols);
> +}
> +
> +void ui_table_uninit(struct ui_table *tbl)
> +{
> + struct ui_col *col, *tmp;
> +
> + list_for_each_entry_safe(col, tmp, &tbl->cols, entry)
> + xfree(col);
> +}
> +
> +void ui_table_pos_set(struct ui_table *tbl, int y, int x)
> +{
> + tbl->y = y;
> + tbl->x = x;
> + tbl->rows_y = y;
> +}
> +
> +static struct ui_col *ui_table_col_get(struct ui_table *tbl, uint32_t id)
> +{
> + struct ui_col *col;
> +
> + list_for_each_entry(col, &tbl->cols, entry) {
> + if (col->id == id)
> + return col;
> + }
> +
> + /* Should not happen in normal case */
> + panic("Invalid column id %u\n", id);
I'd rather omit the panic() here, return NULL and gracefully handle (if
possible) NULL returns in the callers.
> +}
> +
> +static void __ui_table_pos_update(struct ui_table *tbl)
> +{
> + struct ui_col *col;
> + uint32_t pos = tbl->x;
> +
> + list_for_each_entry(col, &tbl->cols, entry) {
> + col->pos = pos;
> + pos += col->len + tbl->col_pad;
> + }
> +}
> +
> +void ui_table_col_add(struct ui_table *tbl, uint32_t id, char *name,
> uint32_t len)
> +{
> + struct ui_col *col = xzmalloc(sizeof(*col));
> +
> + col->id= id;
> + col->name = name;
> + col->len = len;
> + col->align = UI_ALIGN_LEFT;
> +
> + list_add_tail(&col->entry, &tbl->cols);
> +
> + __ui_table_pos_update(tbl);
> +}
> +
> +void ui_table_col_color_set(struct ui_table *tbl, int col_id, int color)
> +{
> + struct ui_col *col = ui_table_col_get(tbl, col_id);
> +
> + col->color = color;
> +}
> +
> +void ui_table_col_align_set(struct ui_table *tbl, int col_id, enum ui_align
> align)
> +{
> + struct ui_col *col = ui_table_col_get(tbl, col_id);
> +
> + col->align = align;
> +}
> +
> +void ui_table_row_add(struct ui_table *tbl)
> +{
> + tbl->rows_y++;
> +}
> +
> +void ui_table_clear(struct ui_table *tbl)
> +{
> + tbl->rows_y = tbl->y;
> +}
> +
> +#define UI_ALIGN_COL(col) (((col)->align == UI_ALIGN_LEFT) ? "%-*.*s" :
> "%*.*s")
> +
> +static void __ui_table_row_print(struct ui_table *tbl, struct ui_col *col,
> + const char *str)
> +{
> + mvprintw(tbl->rows_y, col->pos, UI_ALIGN_COL(col), col->len, col->len,
> str);
> + mvprintw(tbl->rows_y, col->pos + col->len, "%*s", tbl->col_pad, " ");
> +}
> +
> +void ui_table_row_print(struct ui_table *tbl, uint32_t col_id, const char
> *str)
> +{
> + struct ui_col *col = ui_table_col_get(tbl, col_id);
> +
> + attron(col->color);
> + __ui_table_row_print(tbl, col, str);
> + attroff(col->color);
> +}
> +
> +void ui_table_header_color_set(struct ui_table *tbl, int color)
> +{
> + tbl->hdr_color = color;
> +}
> +
> +void ui_table_header_print(struct ui_table *tbl)
> +{
> + struct ui_col *col;
> + int max_width = tbl->width;
> + int width = 0;
> +
> + attron(tbl->hdr_color);
> +
> + mvprintw(tbl->y, tbl->x, "%-*.*s", max_width - tbl->x, max_width -
> tbl->x, "");
> + mvprintw(tbl->y, tbl->x, "");
> +
> + list_for_each_entry(col, &tbl->cols, entry) {
> + __ui_table_row_print(tbl, col, col->name);
> + width += col->len + tbl->col_pad;
> + }
> +
Trailing whitespace (tab) here.
> + mvprintw(tbl->y, width, "%*s", max_width - width, " ");
> + attroff(tbl->hdr_color);
> +}
> diff --git a/ui.h b/ui.h
> new file mode 100644
> index 000..02d1da2
> --- /dev/null
> +++ b/ui.h
> @@ -0,0 +1,53 @@
> +#ifndef UI_H
> +#define UI_H
> +
> +#include "list.h"
> +
> +#include
> +#include
Glo
