[Issue #21] Adds the ability to put count and ratio in the layout. #22

hfytr · 2024-12-01T03:02:06Z

This PR adds a --layout-format flag which enables setting the layout to an arbitrary string containing count {c}, ratio {r}, and layout image {l}. for example passing --layout-format "{c} {r}\n{l}" may result in something like:

3 0.5
│├──┤

This flag is optional, and takes the default value {l}, which should have the same behaviour as before this PR. Within the string, brackets are allowed using \{, \}. \n, \r, \t, \v, and nerdfont icons are also supported.

Tests have not been written, and I am definitely not a C programmer, so I would be more surprised than not if the code has no vulnerabilities / edge cases that I am missing.

NOTE 1: I committed the flake.lock / flake.nix for future contributors, but its up to you if you want to keep them.

NOTE 2: There is no clang-format, so my formatting is likely wrong.

EDIT: emojis "just work." I'll take it. EDIT2: everything good now

alex-courtis · 2024-12-01T05:10:11Z

This looks great - very easy to define.

I'll test and review tomorrow.

alex-courtis · 2024-12-01T05:11:05Z

Happy to help out with tests; they should be straightforward unit.

hfytr · 2024-12-01T17:04:56Z

Hello, it seems that I ran make test, and then make iwyu. (thoughtlessly) Incorporating the suggestions of make iwyu broke the tests. Anyway, should be good now.

alex-courtis · 2024-12-02T03:17:00Z

Hello, it seems that I ran make test, and then make iwyu. (thoughtlessly) Incorporating the suggestions of make iwyu broke the tests. Anyway, should be good now.

Inconsistent iwyu is becoming a problem, I think I'll just remove it.

I've just made a config change #23 to cppcheck, for versions greater than 2.7 (CI)

It looks like everything is passing now, nice work.

If it's OK with you I'll:

update this branch
write and push tests

hfytr · 2024-12-02T03:21:13Z

Looks good! Thanks for putting up with me not following CONTRIBUTING.md. For future, how do I run checks locally?

alex-courtis · 2024-12-02T03:27:57Z

Looks good! Thanks for putting up with me not following CONTRIBUTING.md. For future, how do I run checks locally?

You're all good. It looks like you've been running all of them. cppcheck is the useful one: https://github.com/alex-courtis/wideriver/blob/master/CONTRIBUTING.md#lint

Not sure how your environment's setup, but you can manually check cross-compiler:

make CC=gcc clean all
make CC=clang clean all

I'll deal with iwyu

alex-courtis · 2024-12-02T04:12:28Z

src/layout.c

+
+	return desc;
+}
+
 const char *description_debug(const struct Demand* const demand, const struct Tag* const tag) {


We can delete this now :)

description_info can then be moved into layout_description

alex-courtis

Looking good, I'm using it right now.

I've added a test suite for layout and added one basic test for cfg_set_layout_format.

Please

add {n}
delete description_debug and move description_info into layout_description
complete test cases for cfg_set_layout_format

I'll think about how we can simplify description_info. I've seen something similar but I can't recall it right now.

I'm unavailable until next weekend.

alex-courtis · 2024-12-02T06:06:42Z

src/cfg.c

+			escaped = 0;
+	}
+
+	strcpy(c.layout_format, s);


This will need to be a strncpy - it will trip the linux package security scanners.

alex-courtis · 2024-12-02T06:19:45Z

src/layout.c

+			break;
+	}
+
+	static char desc[LAYOUT_FORMAT_LEN+5];


This works, however it's likely to trip the linux package security scanners.

You're correctly using snprintfs and checking bounds, however the scanners are very strict and won't be able to follow the complex logic here.

First thoughts are that we could just a man 3 regexec, which is known to safely handle buffers and allocation.

Alternatively, we could use strstr or strtok to find the exact tokens instead of looping, as we don't really need a complex pattern.

inc/enum.h

alex-courtis · 2024-12-02T06:35:21Z

NOTE 2: There is no clang-format, so my formatting is likely wrong.

Sorry, I have to get to that...

What IDE do you use? vim gg=G is fine.

alex-courtis · 2024-12-07T03:59:40Z

We could use a simple string replacer.

Let me know if that makes sense and I'll push it.

	static char desc[LAYOUT_FORMAT_LEN+5];

	snprintf(sratio, 10, "%g", ratio);
	snprintf(scount, 10, "%d", count);

	char *counted = replace_string(cfg->layout_format, "{c}", scount);

	char *ratioed = replace_string(counted, "{r}", sratio);
	free(counted);

	char *imaged = replace_string(ratioed, "{l}", image);
	free(ratioed);

	strncpy(desc, imaged, LAYOUT_FORMAT_LEN + 5);
	free(imaged);

	return desc;

// return a copy of str with instances of target substituted with replacement
// call frees
char *replace_string(const char * const str, const char * const target, const char * const replacement) {
	char *match = strstr(str, target);
	if (!match) {
		return strdup(str);
	}

	// allocate result
	size_t len = strlen(str) + strlen(replacement) - strlen(target) + 1;
	char *res = calloc(len, sizeof(char));

	// find string up to match
	size_t len_pre = match - str + 1;
	char pre[len_pre];
	snprintf(pre, len_pre, "%s", str);

	// format
	snprintf(res, len, "%s%s%s", pre, replacement, match + strlen(target));

	// recurse
	char *next = replace_string(res, target, replacement);
	free(res);
	return next;
}

hfytr · 2024-12-07T22:24:01Z

OK, so I added {n}, changed some things to use strn functions. my impl of {n} is pretty bad, as I wasn't sure how to integrate with the existing switch / case. Also, I/you will have to change it to use your string replacer (which LGTM).

EDIT: Your tests were using (the now nonexistent) description_info, so i switched them to layout_description() instead. Sorry for the horrible git history.

Merge remote-tracking branch 'refs/remotes/origin/master'

alex-courtis · 2024-12-09T05:24:11Z

OK, so I added {n}, changed some things to use strn functions. my impl of {n} is pretty bad, as I wasn't sure how to integrate with the existing switch / case. Also, I/you will have to change it to use your string replacer (which LGTM).

Looks good, moved it to the very hidden function from enum.c

EDIT: Your tests were using (the now nonexistent) description_info, so i switched them to layout_description() instead.

Updated layout_description to use string replace. Changed the layout functions to be caller frees, as per the original comments.

Sorry for the horrible git history.

Don't worry about git history, it will be squashed on merge.

We do need to take care of the documentation - man and readme. I'd be grateful if you could update doc/templ/40.man.readme.md with the new options.
Don't worry if you can't run make doc, it's pretty hacky.

…cation

alex-courtis · 2024-12-09T05:25:59Z

Unfortunately I'm out of time; I'll get back to this next weekend.

This one will be a slow burn...

alex-courtis · 2024-12-09T05:26:56Z

src/usage.c

@@ -17,59 +17,77 @@ void usage(const int status) {
 			"\n"
 			"  --stack                         %s|%s|%s                 %s\n"
 			"\n"
-			"  --count-master                  count                                 %d           %d <= count\n"
-			"  --ratio-master                  ratio                                 %.02f      %.1g <= ratio <= %.1g\n"
+			"  --count-master                  count                             "


Please don't change formatting for existing lines.

…gop-truncation" This reverts commit 136b995.

alex-courtis · 2024-12-09T05:47:01Z

src/cfg.c

@@ -190,6 +191,39 @@ bool cfg_set_border_width_monocle(const char *s) {
 	}
 }

+bool cfg_set_layout_format(const char *s) {


I don't quite understand the failure cases of this function; I might be missing the point, but could it just be "garbage in, garbage out"?

Can you please add tests to tst-args_cli.c to cover these cases?

…tion" This reverts commit bf749b9.

…nings

Works, still need \n and emojis;

53917a4

EDIT: emojis "just work." I'll take it. EDIT2: everything good now

lint / check includes / updated help

a5a7627

hfytr force-pushed the master branch from 6c73eed to a5a7627 Compare December 1, 2024 17:01

linting

0012689

alex-courtis added 2 commits December 2, 2024 14:28

Merge branch 'master' into master

41f5149

add iwyu mappings

b063e16

alex-courtis reviewed Dec 2, 2024

View reviewed changes

alex-courtis added 3 commits December 2, 2024 16:36

add layout test and add layout-format to args test

70b9469

terminate static layout desc string

88a8213

add default layout to usage

572f702

alex-courtis requested changes Dec 2, 2024

View reviewed changes

added {n} for name (left/right/monocle/etc)

f6edd4a

Archim Jhunjhunwala and others added 2 commits December 7, 2024 17:09

string vulnerability things

28d675f

merged

9b45aac

hfytr and others added 7 commits December 7, 2024 18:39

merged

6679c64

merge

d853880

Merge remote-tracking branch 'refs/remotes/origin/master'

add string_replace

7f47ae5

layout_description uses string_replace

7c8cc9e

layout_description uses string_replace

b26038b

layout_description and image are caller frees

40046c2

temporarily disable gcc due to false positive stringop-truncation

bf749b9

temporarily use clang for test-vg due to false positive stringop-trun…

136b995

…cation

alex-courtis reviewed Dec 9, 2024

View reviewed changes

Revert "temporarily use clang for test-vg due to false positive strin…

f63c976

…gop-truncation" This reverts commit 136b995.

alex-courtis reviewed Dec 9, 2024

View reviewed changes

alex-courtis added 2 commits December 13, 2024 19:27

Revert "temporarily disable gcc due to false positive stringop-trunca…

93008bb

…tion" This reverts commit bf749b9.

use memcpy (recommended) instead of stpncpy to avoid spurious gcc war…

0f6b118

…nings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #21] Adds the ability to put count and ratio in the layout. #22

[Issue #21] Adds the ability to put count and ratio in the layout. #22

hfytr commented Dec 1, 2024 •

edited

Loading

alex-courtis commented Dec 1, 2024

alex-courtis commented Dec 1, 2024

hfytr commented Dec 1, 2024 •

edited

Loading

alex-courtis commented Dec 2, 2024 •

edited

Loading

hfytr commented Dec 2, 2024

alex-courtis commented Dec 2, 2024

alex-courtis Dec 2, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis Dec 2, 2024

alex-courtis Dec 2, 2024

alex-courtis commented Dec 2, 2024

alex-courtis commented Dec 7, 2024 •

edited

Loading

hfytr commented Dec 7, 2024 •

edited

Loading

alex-courtis commented Dec 9, 2024

alex-courtis commented Dec 9, 2024

alex-courtis Dec 9, 2024

alex-courtis Dec 9, 2024

[Issue #21] Adds the ability to put count and ratio in the layout. #22

Are you sure you want to change the base?

[Issue #21] Adds the ability to put count and ratio in the layout. #22

Conversation

hfytr commented Dec 1, 2024 • edited Loading

alex-courtis commented Dec 1, 2024

alex-courtis commented Dec 1, 2024

hfytr commented Dec 1, 2024 • edited Loading

alex-courtis commented Dec 2, 2024 • edited Loading

hfytr commented Dec 2, 2024

alex-courtis commented Dec 2, 2024

alex-courtis Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

alex-courtis Dec 2, 2024

Choose a reason for hiding this comment

alex-courtis Dec 2, 2024

Choose a reason for hiding this comment

alex-courtis commented Dec 2, 2024

alex-courtis commented Dec 7, 2024 • edited Loading

hfytr commented Dec 7, 2024 • edited Loading

alex-courtis commented Dec 9, 2024

alex-courtis commented Dec 9, 2024

alex-courtis Dec 9, 2024

Choose a reason for hiding this comment

alex-courtis Dec 9, 2024

Choose a reason for hiding this comment

hfytr commented Dec 1, 2024 •

edited

Loading

hfytr commented Dec 1, 2024 •

edited

Loading

alex-courtis commented Dec 2, 2024 •

edited

Loading

alex-courtis Dec 2, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis commented Dec 7, 2024 •

edited

Loading

hfytr commented Dec 7, 2024 •

edited

Loading