WIP: RFC: GUI backends

[mmuman]

This is my first attempt at trying to sort out the X11-specific code from the rest, and allow implementing other display backends, to help with Haiku port, Wayland (#56), or Metal (OSX)…

It's mostly working but not really finished. The last commit kept the original code around with changed ifdefs for easier comparison until it's done, and I'll keep amending it for now.

I'm not used much to templating in C++ so maybe the classes look a bit naive, but I believe it's a reasonable implementation (maybe using templates would avoid RTTI and speed things up, I don't know, but I'm not confident enough to try it). I'm not used to non CamelCase class names, but oh well.

I'm open to comments and suggestions.

Some details:

• the BUILD_GUI define is declared when either BUILD_X11 or any later added GUI backend is enabled.
• Each backend has a priority (0 for text ones, 1 for ncurses, 2 for graphical ones), and they are sorted to try the more interesting ones first (hmm, maybe I should just use a sorted vector right away?).
• The display accessors added to conky.cc are a bit uncertain. First I wanted to just loop over each of the displays, but then part of the code actually assume graphical output, or not. So it's still a mixture of for loop on current_display_outputs and calls to display_output(). In draw_stuff() I call [un]set_display_output() to make sure the display we're drawing to is the graphical one. Although in theory it's the highest priority, so the first on the list, so always the "current" one. So this part can probably be simplified.
• I'm not entirely sure how to handle settings reloading, currently it breaks. I noticed the X11 window is actually created in the settings class, and not really closed between reloads. I'll have to think about this. On platforms where we would support more than one GUI displays (like, X11, SDL, Wayland…) having two enabled in the settings would cause trouble as even though only the first found would be initialize()d, the other one would still have the settings on and so the window created…
• In theory we could display to many outputs at once, but I believe some parts of the code just isn't designed for this. For multiple text outputs it's ok, not for others. For example X11+ncurses didn't work already. Is it desirable? I don't think it's worth it.
• the interface mimics the few X11 calls that are used in the drawing code, I believe they shouldn't be too hard to implement on other platforms (except maybe the stipple patterns on Haiku but well).
• I believe most of the X11 settings are perfectly valid for other GUIs, except maybe -w.
• I added messages about backends disabled at compile-time, but they are probably useless (and X11 on Haiku wouldn't make much sense anyway). Probably I should use the logging interface instead of cerr?
• the lua scripting interface is concerning: it exposes X11 stuff for no particular reason, I believe it should rather provide a GUI-agnostic cairo_surface_create() call. Else it means having to fix all scripts for other platforms. We'll likely have to provide fake calls on other platforms to mimic the X11 API for compatibility anyway.
• ~~I think most backend methods should just return void actually, the bool is useless.~~ DONE
• I noticed I used the American "initialize" instead of British, easy to change.
• the fonts.clear() call when not using X11 is a bit weird, and not easy to do properly from a backend that's not been initialised. Why initialize fonts if we won't use them…

[mmuman]

Hmm, the font stuff also needs work it seems.

[brndnmtthws]

Wow, this is an epic PR. Thank you for the work. It sounds like this is still a work in progress, so I will let you continue, but please let me know when you think it's time for me to take a closer look.

[mmuman]

Second round: Not much more, mostly cleanup of history and code. Fixed copyright on new files.

[lasers]

@mmuman Can you fix https://www.codefactor.io/repository/github/brndnmtthws/conky/pull/672?

[npyl]

@mmuman For macOS if you include <vector> in display-output.cc the build may pass (It was required when i tried to compile your first round of patches) Btw, cheers for this amazing PR, it will really help conky. 🍺

[mmuman]

@mmuman For macOS if you include <vector> in display-output.cc the build may pass (It was required when i tried to compile your first round of patches)

Ok.

@mmuman Can you fix https://www.codefactor.io/repository/github/brndnmtthws/conky/pull/672?

Thanks, I'll have a look.

[mmuman]

Another round: autosquashed all the fixup commits. Also moved the accessors around as I'll probably need them in font code.

[lasers]

Where do we stand now on this? Ready for a review?

[mmuman]

I was quite busy with work so I still didn't touch the font stuff, I'll try this week.

[mmuman]

I had another look at the font code, and I'm not sure how best change it:

• keep the current font_list struct and add a pointer to display-specific stuff inside,
• make fonts an std::vector<font_list *> and subclass font_list directly…
• some other way ?

Also, it seems like xftalpha is applied only to the first font, is it an oversight or intended?

[mmuman]

Ok, I managed to fix the only thing left that was holding the cleanup up (crash on config reload). I think I'll deal with the font code separately, the changes are already large enough. Please comment.

[mmuman]

Looks like including < vector > in display-output.cc isn't enough, clang complains…

[mmuman]

and we need #include algorithm for sort()… (bleh, <*> drops in markdown :p)

[mmuman]

build-wrapper-linux-x86-64: command not found Hmm, don't think it's related, is Travis setup just bork?

[lasers]

@mmuman It broke too many times in that same spot. I don't know why. I'm not even sure about this solution, but change commit hash and try again. Travis sometimes works okay on different runs.

[mmuman]

let's see…

[mmuman]

Oh well, 😴

[mmuman]

Ah yes, some //if (out_to_x.get(*state)) { to remove still.

[mmuman]

So, anyone tried yet? I didn't try to benchmark it, but I suppose the cost of going through the vtables is probably equivalent to all the removed if (out_to_x…).

[lasers]

I tried it now. Worked okay out of box. :+1: The rest is up to @brndnmtthws to review your code.

[npyl]

I will test it on macOS tomorrow (I don't have internet nor xcode :D )

Tested: works great on macOS.

(the window is slightly smaller though! Don't really know if this is a problem)

[npyl]

@mmuman BTW, i think that BUILD_GUI should be the first option to be enabled and not BUILD_X11. BUILD_X11, BUILD_METAL, ... should be available only if BUILD_GUI is (they should depend on it).

I am not sure if this is possible.

[mmuman]

(the window is slightly smaller though! Don't really know if this is a problem)

Odd. It's probably some additions to cur_y in the older code that isn't done in the new code-path. I'll look into this.

[npyl]

@mmuman BTW, I took a look into the code this morning and I have managed to separate in a better way BUILD_GUI and BUILD_X11, guess you didn't have time to do it yourself :) but since I did it I am thinking of pushing it in a separate PR to see what happens! :)

(NOTE: It will break the build on macOS because of the X11 stuff in the fonts.h and some should-be-public stuff defined only in x11.h... Linux should be fine... I guess... Can't really test it to be honest. )

[mmuman]

(the window is slightly smaller though! Don't really know if this is a problem)

Odd. It's probably some additions to cur_y in the older code that isn't done in the new code-path. I'll look into this.

Actually, I can't reproduce it here… screenshot shows official git HEAD, my branch, and Debian Sid's package, and they are perfectly aligned.

Did you compile both with the same options?

[npyl]

@mmuman I compiled it with same options! Don't really know what is going wrong... but probably it is some X11 stuff... we could probably leave this as it is right now and bother later :)

[mmuman]

Damn I should really get this mergeable someday, rebasing is always painful when things change under your feet…

[mmuman]

sigh… indentation fixes …

[mmuman]

must… fix… almost… every… commit :-(

[mmuman]

Commit log for 4d098b053 should have included "screw up any future merge / rebase" 😁

More seriously though, since it changed hundreds of lines for indentation, it's almost impossible to know what actually changed that fixed anything if I need to apply the same fix to the code I moved outside of the file… That's why cosmetics should always be done in separate commits. 😒

[mmuman]

Managed to rebase, builds with X11 but it's probably full of bugs since I don't know what was actually fixed since last time.

[mmuman]

Fixed no-X11 build.

[Caellian]

I skimmed through your code and regarding template usage, I don't see any benefit they would add. They would make your current code harder to read and in this case proper OO inheritance is a way better choice than using templates.

I don't see why subclass methods for different rendering frontends are also declared virtual. If anyone is going to override them later by making a specialized class, they can easily add virtual on their own. This way it's just unneeded and confusing syntax as it implies it's already being done somewhere else in code.

Regarding indentation, clang-tidy should've done that for you. If you're not using it, I suggest you do (although it seems you do, not sure what you meant) before this PR is merged so that every time someone does an additional change after the merge, random bits of your code don't get formatted thus forcing additional line changes that could've been avoided from the start.

Also, considering conky uses C++11, consider using types defined in <cstdint> to ensure proper size allocation for integral types. Take a look at this.

This is something very needed, good work and it's amazing you're still working on it. I asked that my commit is held off until this is accepted so that you don't have to rebase again.

[mmuman]

I don't see why subclass methods for different rendering frontends are also declared virtual. If anyone is going to override them later by making a specialized class, they can easily add virtual on their own. This way it's just unneeded and confusing syntax as it implies it's already being done somewhere else in code.

That's probably some habits of coding in BeOS & Haiku where we have a lot of inheritance… Although, some frontends reuse less specialized ones already. Besides, I wasn't sure which methods I'd need to override when I started, maybe we can clean that later.

Regarding indentation, clang-tidy should've done that for you. If you're not using it, I suggest you do (although it seems you do, not sure what you meant) before this PR is merged so that every time someone does an additional change after the merge, random bits of your code don't get formatted thus forcing additional line changes that could've been avoided from the start.

Ah, didn't know about clang-tidy, I'll give it a try.

Also, considering conky uses C++11, consider using types defined in to ensure proper size allocation for integral types. Take a look at this.

Yeah, again I'm used to old-school C++ from BeOS :-)

This is something very needed, good work and it's amazing you're still working on it. I asked that my commit is held off until this is accepted so that you don't have to rebase again.

It's still on my list, although the list is still very large and growing those days.

[Caellian]

Ah, didn't know about clang-tidy, I'll give it a try.

There are plugins for most IDEs and editors that apply the formatting automatically on file save. These will greatly improve your productivity in all projects with provided .clang-format files. I said clang-tidy, I meant clang-format. clang-tidy is for linting, format actually formats your code.

It's still on my list, although the list is still very large and growing those days.

This is always a problem with big PRs. I found it more productive to do several smaller ones just to avoid problems like rebasing and being burnt out. I'd like to help if I can.

Can you provide a TODO.md file in the root of your repo so that people can see what else is required for the PR and help you out? It can be removed before merge.

Also, use std::string for passing strings to functions like draw_string_at. It can be easily turned into char* with c_str() when needed, but is more C++ish and nicer, also C++11. Using std::u16string might be even better because of emoji support.

It's also possible to use std::pair for nicer passing of points on the screen. I would also allow for nicer and cleaner initialization and possibly defining some utility methods. draw_rect call then becomes

dp.draw_rect(from, dim);
// or
dp.draw_rect({x, y}, {w, h});
// or for some compilers
dp.draw_rect(std::make_pair(x, y), std::make_pair(w, h));

I know these sound nitpicky, but using the newest possible syntax makes updating conky a lot simpler in the future. They also allow compilers to do some additional optimizations in some use cases. For instance, in C++20, a lot of functions are constexpr allowing the compiler to do compile-time calculations, making the compiled code run a lot faster.

[mmuman]

I tried to rebase the branch and use clang-format on each commit for a cleaner history, some conflicts as expected but at least once it does something weird: it wants to align an endif comment with the comment above which has nothing to do with it… oh well? Oddly enough it didn't seem to do it the first time when I applied on the previous version of the branch (all at once).

[mmuman]

Ok, I just rebased, reformating each commit with clang-format. Seems to work as well as the previous code.

Also, considering conky uses C++11, consider using types defined in <cstdint> to ensure proper size allocation for integral types. Take a look at this.

Oh, hmm which types are you talking about, most of the ints passed don't really matter, it's not like we're reading an on-disk format or anything. Unless you ever want to have the API used by add-ons I'm not sure it's very important. I mean, we use these (actually our own versions) a lot in Haiku but that's to have a stable ABI. Maybe size_t would be better for string lengths?

Ah, didn't know about clang-tidy, I'll give it a try.

There are plugins for most IDEs and editors that apply the formatting automatically on file save. These will greatly improve your productivity in all projects with provided .clang-format files.

I tend to use several editors so I didn't really try to configure either.

I said clang-tidy, I meant clang-format. clang-tidy is for linting, format actually formats your code.

Yeah, works mostly as I mentionned.

This is always a problem with big PRs. I found it more productive to do several smaller ones just to avoid problems like rebasing and being burnt out. I'd like to help if I can.

Yes and the more time we wait the more it'll diverge and I'll have to review.

Can you provide a TODO.md file in the root of your repo so that people can see what else is required for the PR and help you out? It can be removed before merge.

I'm more used to org-mode but yeah :-)

Also, use std::string for passing strings to functions like draw_string_at. It can be easily turned into char* with c_str() when needed, but is more C++ish and nicer, also C++11. Using std::u16string might be even better because of emoji support.

Hmm, I don't know u16string but is it UTF-16 or some other weirdo encoding which still has to split codepoints anyway? Not sure it'd help. Of the (recent) OSes I know only Windows doesn't use UTF-8.

The original intent was to touch the original code as less as possible to compare and make sure we didn't drop stuff, which will be useful as although I rebased several times I didn't check much what changed since then so I'll have to review again. Like, it already unmaps the window on click which master branch doesn't, so I'll have to check this.

It's also possible to use std::pair for nicer passing of points on the screen. I would also allow for nicer and cleaner initialization and possibly defining some utility methods. draw_rect call then becomes I know these sound nitpicky, but using the newest possible syntax makes updating conky a lot simpler in the future. They also allow compilers to do some additional optimizations in some use cases. For instance, in C++20, a lot of functions are constexpr allowing the compiler to do compile-time calculations, making the compiled code run a lot faster.

Again, it's already a large piece, I'm not sure I want to add other changes yet, specially using constructs I'm not familiar with. I think we should rather merge it first then let you clean it up. Maybe have a ticket to list these?

Also, I'd someday like have a native Haiku GUI with replicant support which needs gcc2.95, but I guess the actual code already won't build with gcc2.95 anyway and I'll have to write some proxy app to draw from the main process anyway.

[mmuman]

Of course CI doesn't like arg names in the methods… pfff

[Caellian]

u16string should be UTF-16.

I agree with everything you said. I'm a bit busy with work currently, but I'm looking forward to helping out with this PR some time soon. I also sort of abandoned ricing my OS currently as there's a lot of things that need to fall into place before I feel like it's worth the effort. I'm hoping Wayland handles click events a bit better than Xorg for my commit to become actually useful on minimal WMs :smile:. Glad to see you're still working on this.

[mmuman]

Ok, what could I possibly do without Internet on a christmas day… :shrug:

I looked at all the history since I started working on this, and imported manually all the changes I missed when moving the code out of conky.cc, and splitted out the X11 font stuff. Still some things to check but it's getting there.

[mmuman]

Hmm the case where no valid output is found segfaults currently (I asked for only http out but it's built without it):

$ ./src/conky -c ../data/conky.conf 
conky: drawing to single buffer
conky: Unknown setting 'out_to_http'
Unable to find a usable display output.
conky: initialize_display_outputs() failed.
Segmentation fault

I'm wondering what to do there, either have display_output() assert or exit(1) if there's none, or some other way.

[mmuman]

Ok I just rebased over lastest main and the Xft dpi scaling and all of clang-format not able to make up its mind… Please don't make me redo this again. Let's get this merged soon.

Still some pending questions like how to deal with no output…

[mmuman]

Seems the CI complains about unused parameters warnings but I don't get them building locally even with CMAKE_BUILD_TYPE=Debug… Oh -DMAINTAINER_MODE=true ok. Should I put leave them as comments or not?

[mmuman]

I ended up leaving the parameter names as comments in the base class header for reference, and removing them in the subclass ones.

• fixed debug build
• fixed unused parameter warnings
• I noticed I added a move_win function but it's not used anywhere. Not sure it needs to stay.

But, something really odd is happening with maintainer mode: it segfaults while normal debug build doesn't. It seems some ctors aren't called. At least those that should call do_register_display_output. Also there's this unknown setting error which doesn't happen on normal build too, I guess for the same reason of missing ctor calls:

$ LC_ALL=C ./src/conky -DDD -c ../data/conky.conf
DEBUG(0) [/home/revol/devel/conky/conky/src/conky.cc:1930]: reading contents from config file '../data/conky.conf'
DEBUG(0) [/home/revol/devel/conky/conky/src/x11.cc:400]: enter init_X11()
DEBUG(0) [/home/revol/devel/conky/conky/src/x11.cc:481]: Fixed xinerama area to: 0 0 1920 1080
DEBUG(0) [/home/revol/devel/conky/conky/src/x11.cc:432]: leave init_X11()
DEBUG(0) [/home/revol/devel/conky/conky/src/x11.cc:641]: enter init_window()
DEBUG(0) [/home/revol/devel/conky/conky/src/x11.cc:481]: Fixed xinerama area to: 0 0 1920 1080
conky: desktop window (720000f) is subwindow of root window (568)
conky: window type - desktop
conky: drawing to created window (0x7a00001)
DEBUG(0) [/home/revol/devel/conky/conky/src/x11.cc:994]: leave init_window()
conky: drawing to double buffer
conky: Unknown setting 'extra_newline'
DEBUG(1) [/home/revol/devel/conky/conky/src/core.cc:2059]: no templates to replace
DEBUG(1) [/home/revol/devel/conky/conky/src/core.cc:2059]: no templates to replace
DEBUG(1) [/home/revol/devel/conky/conky/src/core.cc:689]: Adding $cpu for CPU 0
DEBUG(1) [/home/revol/devel/conky/conky/src/core.cc:703]: Adding $cpubar for CPU 0
Erreur de segmentation

[mmuman]

So, it happens with -DBUILD_TESTS=true as well, not only maintainer mode. I suppose it's due to some libs being linked statically instead of dynamically.

[mmuman]

Ohh gosh, all this because I use a global for each output to initialize the thing and add it to the list… except since nothing called it from outside the whole object was skipped by the linker. For now I added stub functions called from display-output to get them in. Another option is to get CMake to use --whole-archive in some way.

[npyl]

Hello, I can confirm that your branch builds and runs on macOS Mojave so I guess this is something! I also notice that the default config isn't being loaded on the desktop, so I guess X11 is not enabled on macOS? If that's the case then, even better, because we are going to be using Metal anyway! I can't test on linux yet.

[mmuman]

Well there are still some rough edges like the crash when no output is enabled, but at least it would bother not only me so someone can fix it before I get the time to :-D