Cleanup build flags, global namespace and includes by Caellian · Pull Request #1841 · brndnmtthws/conky

Caellian · 2024-04-19T01:18:48Z

Changes

Flags

Switch BUILD_MOUSE_EVENTS dependency from OWN_WINDOW to BUILD_X11.
Remove BUILD_XINPUT dependency on BUILD_MOUSE_EVENTS (now BUILD_X11 directly).

Refactor

Removed weird linker magic for display output init and replaced it with simpler (and maybe faster?) template functions that behave the same.
- This separates display output compilation units so they don't have to be all built even when their features are disabled.
- Replaced disabled_display_output classes with log messages.
Add explicit errors when feature dependent headers get included where they shouldn't.
Completely remove use of X11 from unrelated code.
Rename special_t to special_node and special_type to text_node_t.
Added #undef for HANDLE_EV macro at the end of the function.
Added axis_align for single axis alignment matching.

Mouse event fixes/improvements

As flag is now separated from XInput one, conky uses newer XInput instead of basic X11 events.
Guarded XI_Motion which means that building without BUILD_MOUSE_EVENTS produces a more performant binary (due to not tracking global (root) mouse movements)

Documentation

Add documentation links to sample configs.

Related issues

[Bug]: conky doesn't compile on MacOS with default flags #1836 - has shown that disabling OWN_WINDOW doesn't compile
[Feature request] Support a small subset of CSS (or CSS-like) rules #1822 - changes to colour.cc make it a lot easier to string (huehue) on new color parsers

Testing

Tested for inclusion errors with various build flag permutations
- BUILD_WAYLAND, BUILD_X11, OWN_WINDOW, BUILD_MOUSE_EVENTS, BUILD_XINPUT
Tested expected behavior for independent XInput handling
Tested display output registration

Remaining work

Demonstrate include improvement using visualization

netlify · 2024-04-19T01:19:05Z

✅ Deploy Preview for conkyweb ready!

Name	Link
🔨 Latest commit	`d23c117`
🔍 Latest deploy log	https://app.netlify.com/sites/conkyweb/deploys/6628201f8bcf6c0007484345
😎 Deploy Preview	https://deploy-preview-1841--conkyweb.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Caellian · 2024-04-19T01:50:13Z

Checked for conflicts, PRs that would require some changes are:

Implement vgoto #1559 - adding a variable without enum scope
SDL GUI backend #1251 - registering a backend using the removed linker dependency injection

Those are minor however, I don't mind helping out if needed.

Caellian · 2024-04-19T02:00:53Z

src/gui.h

+enum class alignment : uint8_t {
+  NONE = 0,
+  NONE_LEFT = 0b0001,
+  NONE_MIDDLE = 0b0010,
+  NONE_RIGHT = 0b0011,
+  TOP_LEFT = 0b0101,
+  TOP_MIDDLE = 0b0110,
+  TOP_RIGHT = 0b0111,
+  MIDDLE_LEFT = 0b1001,
+  MIDDLE_MIDDLE = 0b1010,
+  MIDDLE_RIGHT = 0b1011,
+  BOTTOM_LEFT = 0b1101,
+  BOTTOM_MIDDLE = 0b1110,
+  BOTTOM_RIGHT = 0b1111,
+};
+constexpr uint8_t operator*(alignment index) {
+  return static_cast<uint8_t>(index);
+}


This is an example of how I changed global enums to enum class. I did this because headers like gui.h are included in a lot of places and using pre C++11 style enums declares enum variants as global constants. This in turn means that no other enum can use same variant names (e.g. NONE).

The bigger issues are:

Variants cast implicitly into int and bool

Promotes poor code hygiene because function declarations can use any int argument value which leads to same include mess this PR aims to solve.

Variant names can make it unclear which constant is referenced.

Advantages:

enum class requires exhaustive matching in switch statements which ensures all places referencing them get updated when new variants are added.

Also, purely matter of taste, window_type::NORMAL looks much nicer than TYPE_NORMAL (and is more descriptive).

- Separated displays so they don't have to be all built even when the features are disabled. - Removed weird linker magic for display init and replaced it with straightforward (and faster) template functions that behave the same. - Replaced disabled_display_output classes with log messages. - Add explicit errors when feature dependent headers get included where they shouldn't. - Use gperf to build perfect hash for color names instead of custom X11 lookup. This should make it easier to add more color names to the list with no extra cost. - Made alignment use a specific bit layout which greatly simplifies code. - Switch BUILD_MOUSE_EVENTS dependency from OWN_WINDOW to BUILD_X11. - Other minor improvements to some existing code which shouldn't affect behavior. - Add documentation links to sample configs. Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian · 2024-04-19T02:32:50Z

data/conky-dfly-smp.conf

-    own_window_type = 'desktop',
+    own_window_type = 'normal',
+    own_window_hints = 'undecorated,sticky,below,skip_taskbar,skip_pager',


Changed this to normal because it's the type that most commonly works, on WMs/DEs that are most commonly used (with desktops).

Caellian

Explanation of changes to display output registration.

Caellian · 2024-04-19T02:49:28Z

src/display-output.cc

+  outputs.reserve(static_cast<size_t>(output_t::OUTPUT_COUNT));
+  register_output<output_t::CONSOLE>(outputs);
+  register_output<output_t::NCURSES>(outputs);
+  register_output<output_t::FILE>(outputs);
+  register_output<output_t::HTTP>(outputs);
+  register_output<output_t::X11>(outputs);
+  register_output<output_t::WAYLAND>(outputs);
+
+  for (auto out : outputs) { NORM_ERR("FOUND: %s", out->name.c_str()); }


Given that we "called" noop init_*_output functions in order to trigger static initializers, I wondered why not just initialize them from those functions instead of relying on execution flow hidden in constructors of static objects.

Caellian · 2024-04-19T02:51:04Z

src/display-output.hh

+enum class output_t : uint32_t {
+  CONSOLE,
+  NCURSES,
+  FILE,
+  HTTP,
+  X11,
+  WAYLAND,
+  OUTPUT_COUNT
+};
+template <output_t Output>
+void register_output(display_outputs_t &outputs);


This can be handled by using a templated function, ideally with an enum representing different outputs - but could've been strings or other values, or even discrete types (though that would cause recursive dependency).

It's useful to have an enum of supported outputs in display_output.hh anyway.

Caellian · 2024-04-19T02:54:56Z

src/display-output.cc

+#ifndef BUILD_WAYLAND
+template <>
+void register_output<output_t::WAYLAND>(display_outputs_t &outputs) {
+  log_missing("Wayland", "BUILD_WAYLAND");
+}
+#endif


Given that register_output functions are templated, this means that we can just log if a feature is not enabled. And this causes a linker error if either the display output isn't in compilation or a fallback isn't defined. This function will almost always be inlined by compiler, even with -O1 (unlike class constructors).

I find that to be much less error prone than hoping some global static in some other file gets constructed.

Caellian · 2024-04-19T02:55:49Z

src/display-output.hh

-class disabled_display_output : public display_output_base {
- public:
-  const std::string define;
-  disabled_display_output(const std::string &name, const std::string &define);
-};
-


So disabled_display_output is no longer needed.

Caellian · 2024-04-19T02:56:49Z

src/display-wayland.cc

-namespace {
-
 #ifdef BUILD_WAYLAND
+namespace {
 conky::display_output_wayland wayland_output;
-#else
-conky::disabled_display_output wayland_output_disabled("wayland",
-                                                       "BUILD_WAYLAND");
-#endif
-
 }  // namespace
-extern void init_wayland_output() {}
+
+template <>
+void register_output<output_t::WAYLAND>(display_outputs_t &outputs) {
+  outputs.push_back(&wayland_output);
+}
+#endif /* BUILD_WAYLAND */


The functions are pretty straightforward to write, take up less space, and it's much more clear what's going on.

Caellian · 2024-04-19T02:58:14Z

src/display-output.cc

-void do_register_display_output(const std::string &name,
-                                display_output_base *output) {
-  struct display_output_constructor {
-    display_output_constructor() { display_outputs = new display_outputs_t(); }
-    ~display_output_constructor() {
-      delete display_outputs;
-      display_outputs = nullptr;
-    }
-  };
-  static display_output_constructor constructor;
-
-  bool inserted = display_outputs->insert({name, output}).second;
-  if (!inserted) {
-    throw std::logic_error("Display output with name '" + name +
-                           "' already registered");
-  }
-}


Plus we don't need to handle possible out of order initialization of the static output list, nor possibility of double insertion because it's all handled in a single place.

Caellian · 2024-04-19T03:01:35Z

src/display-output.cc

-extern void init_console_output();
-extern void init_ncurses_output();
-extern void init_file_output();
-extern void init_http_output();
-extern void init_x11_output();
-extern void init_wayland_output();


And this gets replaced by explicit template specialization, which is a well supported language mechanism.

Fix missing Colour construct alpha argument. Add Makefile ignore to 3rdparty/toluapp Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian · 2024-04-20T05:23:35Z

Good to see labeler changes working 😆

Moved color parsing into a stacked PR: #1848

Removed majority of changed lines

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian · 2024-04-20T06:36:24Z

Moved alignment changes into #1849.

Caellian · 2024-04-20T07:43:11Z

Did something... for visualization of includes, does need a lot more work though. Not sure if it will be useful before finishing this PR, but I'll be able to create some visualization of it at least.

Display files are now no longer compiled if their features are disabled Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

This reaches 0% unneeded X11 polution in the sources. Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Added documentation for it. Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

brndnmtthws

There's a lot in this PR, but overall I like it :)

Caellian · 2024-04-20T16:50:19Z

There's a lot in this PR, but overall I like it :)

Thank you. I'll maybe tweak it a tiny bit more before merging (some added checks aren't needed). I wanted to change those things (like enums) all at once so it's just a single merge for others.

Caellian · 2024-04-22T20:30:41Z

There's a linking error with a specific build flag configuration, holding this off until I resolve it. Maybe it was there before, maybe it wasn't.

EDIT: Only locally apparently, I wasn't able to get Imlib2.so to link for some reason. Works on CI.

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Hide lua state from conky-imlib2.cc Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

github-actions bot added 3rdparty suggests changes to 3rd party dependencies and removed dependencies adds or removes dependencies, or suggests alternatives labels Apr 19, 2024

Caellian force-pushed the dev/cleanup-globals branch 4 times, most recently from 3fee8d7 to 2cc7944 Compare April 19, 2024 01:33

Caellian marked this pull request as draft April 19, 2024 01:41

Caellian commented Apr 19, 2024

View reviewed changes

Caellian force-pushed the dev/cleanup-globals branch 3 times, most recently from 60843da to 063f300 Compare April 19, 2024 02:29

github-actions bot removed the 3rdparty suggests changes to 3rd party dependencies label Apr 19, 2024

Caellian force-pushed the dev/cleanup-globals branch from 063f300 to 4cd6714 Compare April 19, 2024 02:30

github-actions bot removed the documentation suggests documentation changes or improvements label Apr 19, 2024

Caellian force-pushed the dev/cleanup-globals branch from 4cd6714 to c23afeb Compare April 19, 2024 02:31

Caellian commented Apr 19, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

Disable mouse tracking if BUILD_MOUSE_EVENTS=off

a04f133

Fix missing Colour construct alpha argument. Add Makefile ignore to 3rdparty/toluapp Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

github-actions bot added the 3rdparty suggests changes to 3rd party dependencies label Apr 19, 2024

Revert set_struts changes

dc8b241

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

github-actions bot added the lua related to Lua integration in conky label Apr 20, 2024

Caellian added 2 commits April 20, 2024 09:53

Remove unneeded guards

c809867

Display files are now no longer compiled if their features are disabled Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Remove X11 from unrelated headers

7c4fea5

This reaches 0% unneeded X11 polution in the sources. Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian force-pushed the dev/cleanup-globals branch 2 times, most recently from 550f276 to 9a2ec7a Compare April 20, 2024 08:42

Reenabled parts of window_type which would work in Wayland

c69dcaf

Added documentation for it. Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian force-pushed the dev/cleanup-globals branch from 9a2ec7a to c69dcaf Compare April 20, 2024 08:45

Caellian requested a review from brndnmtthws April 20, 2024 08:46

Caellian marked this pull request as ready for review April 20, 2024 08:48

Caellian mentioned this pull request Apr 20, 2024

[Feature request] Support a small subset of CSS (or CSS-like) rules #1822

Open

brndnmtthws approved these changes Apr 20, 2024

View reviewed changes

Move dpi_scale to display-output.hh

3093cdd

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian force-pushed the dev/cleanup-globals branch 4 times, most recently from 46767e8 to cba15e6 Compare April 23, 2024 20:37

Move saved_coordinates to imlib

8e07629

Hide lua state from conky-imlib2.cc Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian force-pushed the dev/cleanup-globals branch from cba15e6 to 8e07629 Compare April 23, 2024 20:39

Update docs

d23c117

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>

Caellian merged commit 6adf6b9 into main Apr 23, 2024

Caellian deleted the dev/cleanup-globals branch April 23, 2024 21:15

Caellian mentioned this pull request Apr 30, 2024

Remove dpi scaling of maximum width #1877

Merged

1 task

This was referenced May 20, 2024

[Bug]: Conky are only in window not at desktop on sway #1916

Closed

[Bug]: Wrong hr line thickness #1930

Closed

Uh oh!

Conversation

Caellian commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Flags

Refactor

Mouse event fixes/improvements

Documentation

Related issues

Testing

Remaining work

Uh oh!

netlify bot commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for conkyweb ready!

Uh oh!

Caellian commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Caellian Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

Caellian left a comment

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Caellian Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Caellian commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Caellian commented Apr 20, 2024

Uh oh!

Caellian commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brndnmtthws left a comment

Choose a reason for hiding this comment

Uh oh!

Caellian commented Apr 20, 2024

Uh oh!

Caellian commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Caellian commented Apr 19, 2024 •

edited

Loading

netlify bot commented Apr 19, 2024 •

edited

Loading

Caellian commented Apr 19, 2024 •

edited

Loading

Caellian Apr 19, 2024 •

edited

Loading

Caellian Apr 19, 2024 •

edited

Loading

Caellian Apr 19, 2024 •

edited

Loading

Caellian Apr 19, 2024 •

edited

Loading

Caellian Apr 19, 2024 •

edited

Loading

Caellian Apr 19, 2024 •

edited

Loading

Caellian commented Apr 20, 2024 •

edited

Loading

Caellian commented Apr 20, 2024 •

edited

Loading

Caellian commented Apr 22, 2024 •

edited

Loading