6
\$\begingroup\$

This is the beginning of a simple X11/Xlib paint program. I got most of this code from an online source and started to modify it. But since I'm new to X11, I thought I'd ask before I add on to bad code.

#include <X11/X.h>
#include <X11/Xlib.h>
#include <X11/Xutil.h>
#include <X11/XKBlib.h>
#include <X11/Xatom.h>
#include <X11/Xft/Xft.h>
#include <stdio.h>
#include <stdlib.h>
#include <err.h>

#define POSX    500
#define POSY    500
#define WIDTH   750
#define HEIGHT  500
#define BORDER  5
#define LINE    4
#define HX_BLUE     "#0000ff"
#define HX_GREEN    "#00ff00"
#define HX_RED      "#ff0000"
#define HX_YELLOW   "#ffff00"
#define HX_PINK     "#ff00ff"
#define HX_CYAN     "#00ffff"
#define HX_WHITE    "#ffffff"
#define HX_BLACK    "#000000"
#define HX_ORANGE   "#ff8040"
#define HX_GRAY     "#c0c0c0"
#define HX_DARKGRAY "#808080"
#define HX_BROWN    "#804000"

#define CHILD_W 25
#define CHILD_H 25
#define CHILD_B 0

typedef XftColor Clr;

typedef struct {
    Window win;
    Clr *color;
} ColorButtons;

static Display *dpy;
static int scr;
static Window root;
static Visual *vis;
static const char *colors[] = {HX_BLUE, HX_GREEN, HX_RED, HX_YELLOW, HX_PINK, HX_CYAN,
                            HX_WHITE, HX_BLACK, HX_ORANGE, HX_GRAY, HX_DARKGRAY, HX_BROWN};
#define NUM_COLORS  (int)(sizeof(colors) / sizeof(colors[0]))
static ColorButtons color_buttons[NUM_COLORS];


static Window create_win(int x, int y, int w, int h, int b, Window* parent, int idx)
{
    Window win;
    XSetWindowAttributes xwa = 
    {
        .background_pixel = WhitePixel(dpy, scr),
        .border_pixel = BlackPixel(dpy, scr),
    };

    if (parent == NULL)
    {
        xwa.event_mask = Button1MotionMask | ButtonPressMask | ButtonReleaseMask | KeyPressMask;
        
        win = XCreateWindow(dpy, root, x, y, w, h, b, DefaultDepth(dpy, scr),
                        InputOutput, vis,
                        CWBackPixel | CWBorderPixel | CWEventMask, &xwa);
    }
    else
    {
        xwa.event_mask = ButtonPress;
        xwa.background_pixel = color_buttons[idx].color->pixel;
        xwa.border_pixel = WhitePixel(dpy, scr);
        
        win = XCreateWindow(dpy, *parent, x, y, w, h, b, DefaultDepth(dpy, scr),
                                InputOutput, vis,
                                CWBackPixel | CWBorderPixel | CWEventMask, &xwa);
    }
    return win;
}

static GC create_gc(int line_width)
{
    GC gc;
    XGCValues xgcv;
    unsigned long valuemask;

    xgcv.line_style = LineSolid;
    xgcv.line_width = line_width;
    xgcv.cap_style  = CapButt;
    xgcv.join_style = JoinMiter;
    xgcv.fill_style = FillSolid;
    xgcv.foreground = BlackPixel(dpy, scr);
    xgcv.background = WhitePixel(dpy, scr);

    valuemask = GCForeground | GCBackground | GCFillStyle | GCLineStyle
                | GCLineWidth | GCCapStyle | GCJoinStyle;
    gc = XCreateGC(dpy, root, valuemask, &xgcv);

    return gc;
}

static void run(GC gc)
{
    XEvent ev;
    Window cur_win;
    int init = 0;
    int prev_x = 0, prev_y = 0;
    
    while (!(XNextEvent(dpy, &ev)))
    {
        switch (ev.type)
        {
            case ButtonPress:
                if (ev.xbutton.button == Button1)
                {
                    cur_win = ev.xbutton.window;

                    int flag = 0;
                    for (int i = 0; i < NUM_COLORS; i++)
                    {
                        if (cur_win == color_buttons[i].win)
                        {
                            XSetForeground(dpy, gc, color_buttons[i].color->pixel);
                            flag = 1;
                            break;
                        }
                    }
                    if (!flag)
                        XDrawPoint(dpy, ev.xbutton.window, gc, ev.xbutton.x, ev.xbutton.y);
                }
                else if (ev.xbutton.button == Button3)
                    XSetForeground(dpy, gc, BlackPixel(dpy, scr));
                break;
            case MotionNotify:
                if (init)
                {
                    XDrawLine(dpy, ev.xbutton.window, gc, prev_x, prev_y, ev.xbutton.x, ev.xbutton.y);
                }
                else
                {
                    XDrawPoint(dpy, ev.xbutton.window, gc, ev.xbutton.x, ev.xbutton.y);
                    init = 1;
                }
                prev_x = ev.xbutton.x, prev_y = ev.xbutton.y;
                break;
            case ButtonRelease:
                init = 0;
                break;
            case KeyPress:
                if (XkbKeycodeToKeysym(dpy, ev.xkey.keycode, 0, 0) == XK_q)
                    return;
        }
    }    
}

int main()
{
    Window main_win;
    GC gc;
    int x = 200;
    int y = HEIGHT - (2 * (CHILD_H + CHILD_B));
    
    if (!(dpy = XOpenDisplay(NULL)))
        errx(1, "Can't open display");

    scr = DefaultScreen(dpy);
    root = RootWindow(dpy, scr);
    vis = DefaultVisual(dpy, scr);
    main_win = create_win(POSX, POSY, WIDTH, HEIGHT, BORDER, NULL, 0);

    for (int i = 0; i < NUM_COLORS; i++)
    {
        color_buttons[i].color = malloc(sizeof(Clr));

        if (!color_buttons[i].color)
            errx(1, "Can't allocate memory for color");

        if (!XftColorAllocName(dpy, vis, DefaultColormap(dpy, scr),
                            colors[i], color_buttons[i].color))
            errx(1, "Can't allocate xft color");

        color_buttons[i].color->pixel |= 0xff << 24;
        color_buttons[i].win = create_win(x, y, CHILD_W, CHILD_H, CHILD_B, &main_win, i);
        
        if (i == (NUM_COLORS/2)-1)
        {
            y += CHILD_H + CHILD_B;
            x = 200;
        }
        else
        {
            x += CHILD_W + CHILD_B;
        }
    }
    
    gc = create_gc(LINE);
    
    XSizeHints xsh = {.min_width = WIDTH, .min_height = HEIGHT,
                    .max_width = WIDTH, .max_height = HEIGHT};
    xsh.flags = PMinSize | PMaxSize;
    XSetSizeHints(dpy, main_win, &xsh, XA_WM_NORMAL_HINTS);
    
    XStoreName(dpy, main_win, "Simple Paint");
    XMapWindow(dpy, main_win);
    XMapSubwindows(dpy, main_win);
    
    run(gc);

    /* cleanup */
    XUnmapWindow(dpy, main_win);
    XUnmapSubwindows(dpy, main_win);
    XDestroySubwindows(dpy, main_win);
    XDestroyWindow(dpy, main_win);
    XFreeGC(dpy, gc);
    for (int i = 0; i < NUM_COLORS; i++)
        XftColorFree(dpy, vis, DefaultColormap(dpy, scr), color_buttons[i].color);
    XCloseDisplay(dpy);
    
    return 0;
}

And here is the Makefile:

PROG = paint
SRC  = ${PROG}.c
OBJ  = ${SRC:.c=.o}

CC   = cc
INCS = -I/usr/include/X11 `pkg-config --cflags xft`
LIBS = -lX11 -lXft

LDFLAGS = ${LIBS}
CFLAGS  = -Wall -Wextra -O0 ${INCS}

all: ${PROG}

${PROG}: ${OBJ}
    ${CC} -o $@ ${OBJ} ${LDFLAGS}

%.o: %.c
    ${CC} -c $< ${CFLAGS}

clean:
    -rm ${OBJ} ${PROG}

.PHONY: all clean
\$\endgroup\$
2
  • 1
    \$\begingroup\$ @toolic Yes, I did. I should say that I didn't take someone's code, but rather I implemented what I saw in a tutorial and then added some changes. \$\endgroup\$ Commented Jan 14 at 18:11
  • 1
    \$\begingroup\$ One point not worth an answer: if you’re using pkg-config in your makefile, you might as well use it for all your libraries. You’re both compiling with the headers (--cflags) and linking to the libraries (--libs). You also probably want to use XLib-XCB or just XCB. (Although this requires some changes to the source code.) So something like $(pkg-config --cflags --libs xlib-xcb xcb-util xcb-xkb xcb-atom xft) Alternatively, maybe --cflags --libs x11 xft. \$\endgroup\$ Commented Jan 16 at 0:21

2 Answers 2

Reset to default
5
\$\begingroup\$

Some minor things:

Allocation sizing

Is sizeof(Clr) correct in the following?

color_buttons[i].color = malloc(sizeof(Clr));

First let us find color_buttons[]. It is not in the function, so maybe its global. OK found it dozens of lines toward the top as:

static ColorButtons color_buttons[NUM_COLORS];

2nd, let us look for ColorButtons definition and the member .color. OK, that was not too far away as

typedef struct {
    Window win;
    Clr *color;
} ColorButtons;

So sizeof(Clr) is correct.

Alternatively, if code was sized to the referenced object, not type:

color_buttons[i].color = malloc(sizeof color_buttons[i].color[0]);

we know it the right size without all that searching.

This is easier to code right, review and maintain.

Careful with left shifts

0xff << 24 in color_buttons[i].color->pixel |= 0xff << 24; is undefined behavior (UB), shifting a 1 into the sign bit of an int. This occurs if int is 32 or 16 bit.

Alternatives include

color_buttons[i].color->pixel |= 0xffU << 24; // OK as long as `unsigned` is 32-bit
color_buttons[i].color->pixel |= 0xffLU << 24;
color_buttons[i].color->pixel |= 0xff000000;

// Preferred approaches that avoids naked magic numbers:
color_buttons[i].color->pixel |= Some_named_const_object_of_same_type_as_.pixel;
color_buttons[i].color->pixel |= SOME_DESCRIPTION_MACRO_NAME;

() most macros

The below define does not tie the cast to the quotient that well.

#define NUM_COLORS  (int)(sizeof(colors) / sizeof(colors[0]))

is the same as

#define NUM_COLORS  ((int)(sizeof(colors)) / sizeof(colors[0]))

Which results in a type size_t.

I suspect OP really wants:

#define NUM_COLORS  ((int)(sizeof(colors) / sizeof(colors[0])))

Advanced: Consider leaving as size_t

#define NUM_COLORS  ((sizeof(colors) / sizeof(colors[0]))

... and adjust other code to perform sizing and indexing with size_t math. Be mindful that a size_t is never negative.

As others opinions still go for int here, follow your group's coding standard.

Formatting

Code appears manually formatted. Save time and improve consistency and use an auto formatter.

Robust code looks for allocation failures

This code has color_buttons[i].color = malloc(sizeof(Clr));, yet lacks a following if (color_buttons[i].color == NULL test.

Documentation

I find code comments scant and deserving of some higher level ones.

\$\endgroup\$
0
5
\$\begingroup\$

DRY

In create_win() there are two calls to XCreateWindow(), each having a long, long list of parameters.
Turns out, only one of the parameters is different.

While each branch of the if/else performs different preparatory work, please find a way to use a stack variable to distinguish root from *parent. (It feels like the datatype Window hides its nature; being a pointer. Hidden 'splats' are an anathema.) This could eliminate one instance of a monster function call.

Trivial

Many of the 2D token names are based on Height and Width, but then others use x and y (which refer to "width" and "height" respectively.) Note how the 'sequence' is subtly swapped.

Please stick to one convention. Perhaps int h = SOME_HEIGHT, w = SOME_WIDTH;.

Reduce the burden on the reader who is trying to determine which way is up in unfamiliar code.

Preprocessor macros or enums

The preprocessor is a wonderful "editing" tool, allowing tokens to be replaced by constants (and much more) before the compiler even sees the code.

The downside is that those tokens that can be read in source code are not available when debugging.

Suggestion:

    enum {
        POSX = 500,
        POSY = 500,
        WIDTH = 750,
        HEIGHT = 500,
        BORDER = 5,
        LINE = 4
    };

These compile time token names are available within a debug build.

More (kinda) trivial

When working with code written/maintained by others, it's probably best to adapt to their conventions (as much as possible.)

// static Window create_win( // snake case
   static Window createWin(  // camel case is, at least, half way to their standard

Worrisome

create_gc() uses an uninitialised struct XGCValues xgcv; defined in its current stack frame. The code goes on to explicitly set some struct members. Are ALL members assigned a definite value? The nervous reader is compelled to seek out documentation hoping to quell their uncertainty.

Suggest:

XGCValues xgcv = {0};

to ensure that ALL members will always be set consistently. Fluctuating values of uninitialised stack variables are a major pain to diagnose. (Sometimes the program works; sometimes it doesn't.) Generally, the reader should be able to see all variables being assigned definite values.

The same sort of trepidation arises from

        color_buttons[i].color->pixel |= 0xff << 24;

This line mysteriously initialises one member of a struct whose complete definition (and use) requires a bit of research. malloc() is not calloc(), so there's every possibility that uninitialised values may be different between compilations. What appeared to work for the past month suddenly exhibits bizarre behaviour.

Don't allow variables to wander off uninitialised!

Finally, why create this alias?

typedef XftColor Clr;

There are so many variations of "color" (abbreviated, plural, w/ suffix) that the code becomes difficult to track. As noted by @Chux, the only place Clr is used is where it should not have been used. Try to simplify, not to make things more opaque.

Avoid extra variables

There's a very small gain to be had by eliminating one superfluous variable:

    int flag = 0; // Say bye-bye, flag!
    for (int i = 0; i < NUM_COLORS; i++)
    {
        if (cur_win == color_buttons[i].win)
        {
            XSetForeground(dpy, ...
            flag = 1;
            break;
        }
    }
    if (!flag)
        XDrawPoint(dpy, ...

And, without flag:

    int i = 0;
    while( i < NUM_COLORS && cur_win != color_buttons[i].win )
        i++; // scanning through array
    if ( i < NUM_COLORS )
        XSetForeground(dpy, ...
    else
        XDrawPoint(dpy, ...

It's fewer lines, and one trivial stack variable removed.
In this instance, the reader is trying to keep track of these (unfamiliar?) library objects and functions, the sequence of operations, the coder's approach to solving the project, and a bevy of transient variables.

As long as clarity (and correctness!) is not sacrificed, strive to maximise the utility of fewer resources.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Personally, I'm unsettled that this app code uses malloc(), but then some mysterious library function is entrusted to perform free(). This feels unbalanced. The OP should run this with Valgrind (or something) to ensure all dynamic allocations have been released. This "sandbox" code could be destined for incorporation into a larger project... \$\endgroup\$ Commented Jan 17 at 0:47
  • 1
    \$\begingroup\$ In new C23 code, you have constexpr constants (but not functions). This can replace enum. \$\endgroup\$ Commented Jan 17 at 2:08
  • 1
    \$\begingroup\$ @Fe2O3 Good tip on Valgrind. First time using it. My program has grown since what I posted above, but glad to see that I have zero issues. \$\endgroup\$ Commented Jan 19 at 1:06

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.