Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add colorful tags patch #206

Closed
wants to merge 0 commits into from
Closed

Add colorful tags patch #206

wants to merge 0 commits into from

Conversation

UtkarshVerma
Copy link
Contributor

This PR adds the colorful tags patch by fitrh: fitrh/dwm#1

I have tested it and it works well. Here's how it looks with alpha, hidevacanttags, and underline patches.
image

UtkarshVerma
UtkarshVerma commented Dec 26, 2021
View reviewed changes
dwm.c Outdated Show resolved Hide resolved
bakkeby
bakkeby reviewed Dec 26, 2021
View reviewed changes
Copy link
Owner

@bakkeby bakkeby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nitpicks.

config.def.h Outdated Show resolved Hide resolved
config.def.h Outdated Show resolved Hide resolved
dwm.c Outdated
@@ -479,6 +492,9 @@ struct Monitor {
#if BAR_ALTERNATIVE_TAGS_PATCH
unsigned int alttag;
#endif // BAR_ALTERNATIVE_TAGS_PATCH
#if BAR_COLORFULTAGS_PATCH
unsigned int colorseltag;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is curious that this is on a per monitor basis rather than being applied globally. Wonder what the use case is for being able to toggle this during runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, I just wrote it for the sake of the patch. I guess it would be more logical to have it as a global constant.

Apart from that, we could completely remove this variable and just turn it on in the code whenever underline tags are enabled. Since this is dwm-flexipatch we're talking about, I guess it would be reasonable to consider this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bakkeby please let me know if we should keep this variable. I personally don't see any reason for it being there. I just included it for the sake of the patch.

config.def.h Outdated
@@ -290,6 +309,19 @@ static char *colors[][ColCount] = {
[SchemeHidNorm] = { hidnormfgcolor, hidnormbgcolor, c000000, c000000 },
[SchemeHidSel] = { hidselfgcolor, hidselbgcolor, c000000, c000000 },
[SchemeUrg] = { urgfgcolor, urgbgcolor, urgbordercolor, urgfloatcolor },
#if BAR_COLORFULTAGS_PATCH
[SchemeTag] = { "#666666", "#1b1d1e", c000000 },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The colour codes here are inline. You may want to move these out to variables instead as you would want to be able to override these using Xresources (and you'll want to update patch/xrdb.c as well in this respect).

Being able to override these using Xresources without having to use variable names would be great, but I have so far not found a way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those colors are just placeholder. I'm still new to the codebase so I left it like that. I guess we can expect users to override these in their own config.

dwm.c Outdated
@@ -673,6 +689,9 @@ static pid_t spawncmd(const Arg *arg);
static void tag(const Arg *arg);
static void tagmon(const Arg *arg);
static void togglebar(const Arg *arg);
#if BAR_COLORFULTAGS_PATCH
static void togglecolorseltag();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could possibly be moved to a new file patch/colorfultags.h, but more importantly the function should take a const Arg *arg argument if the function is intended to be used as a keybinding.

On the same note the function call should be made available via IPC commands and fsignals, maybe even dwmc.

patch/bar_tags.c Outdated Show resolved Hide resolved
patch/bar_tags.c Outdated Show resolved Hide resolved
patch/bar_tags.c Outdated Show resolved Hide resolved
patch/bar_tags.c Outdated
m->tagset[m->seltags] & 1 << i
#endif // BAR_UNDERLINETAGS_PATCH
? m->colorseltag
? tagschemes[i]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine as is, but I just wanted to note that if the user configures their build to have, say, 11 tags then dwm will likely crash at some point referring to values that are out of bounds here.

One way to avoid this could be to do something like tagschemes[i % NUMTAGS] at least then it would wrap around.

On the same note the entire tagschemes array is redundant in this context as you could just replace the above with

? SchemeTag1 + i

(of course you still risk running out of color schemes if increasing the number of tags)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right about tagschemes, but if we remove that, we'll have to assume that the enums are declared sequentially. Would it be safe to assume that?
Regarding the number of tags, I guess we could mention in the patch description that the user needs to define more colors if they are using more than 9 tags.

patches.def.h Outdated
* https://github.com/fitrh/dwm/issues/1
*/
#define BAR_COLORFULTAGS_PATCH 0

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved out of the bar module section and into the bar options section, probably right beneath BAR_COLOR_EMOJI_PATCH.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to place it under BAR_TAGS_PATCH as it would be easier to navigate.

@bakkeby bakkeby mentioned this pull request Feb 4, 2022
Open
@UtkarshVerma
Copy link
Contributor Author

I recently added support for colorful tags in the XRDB_PATCH as well. This is what I added to config.h:

#if BAR_COLORFULTAGS_PATCH
static char tag1fgcolor[]                = "#f92672";
static char tag2fgcolor[]                = "#a6e22e";
static char tag3fgcolor[]                = "#f4bf75";
static char tag4fgcolor[]                = "#66d9ef";
static char tag5fgcolor[]                = "#ae81ff";
static char tag6fgcolor[]                = "#f8f8f2";
static char tag7fgcolor[]                = "#75715e";
static char tag8fgcolor[]                = "#f4bf75";
static char tag9fgcolor[]                = "#a1efe4";

static char tag1bgcolor[]                = "#272822";
static char tag2bgcolor[]                = "#272822";
static char tag3bgcolor[]                = "#272822";
static char tag4bgcolor[]                = "#272822";
static char tag5bgcolor[]                = "#272822";
static char tag6bgcolor[]                = "#272822";
static char tag7bgcolor[]                = "#272822";
static char tag8bgcolor[]                = "#272822";
static char tag9bgcolor[]                = "#272822";

static char layoutfgcolor[]				 = "#ae81ff";
static char layoutbgcolor[]				 = "#272822";
#endif


// inside colors variable
	#if BAR_COLORFULTAGS_PATCH
	[SchemeTag1]       = { tag1fgcolor,		tag1bgcolor,	c000000 },
	[SchemeTag2]       = { tag2fgcolor,		tag2bgcolor,	c000000 },
    [SchemeTag3]       = { tag3fgcolor,		tag3bgcolor,	c000000 },
    [SchemeTag4]       = { tag4fgcolor,		tag4bgcolor,	c000000 },
    [SchemeTag5]       = { tag5fgcolor,		tag5bgcolor,		c000000 },
	[SchemeTag6]       = { tag6fgcolor,		tag6bgcolor,	c000000 },
	[SchemeTag7]       = { tag7fgcolor,		tag7bgcolor,	c000000 },
	[SchemeTag8]       = { tag8fgcolor,		tag8bgcolor,	c000000 },
	[SchemeTag9]       = { tag9fgcolor,		tag9bgcolor,	c000000 },
	[SchemeLayout]     = { layoutfgcolor,	layoutbgcolor,	c000000 },
	#endif

This is what I added to xrdb.c:

				#if BAR_COLORFULTAGS_PATCH
				XRDB_LOAD_COLOR("dwm.tags.fg1", tag1fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg1", tag1bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg2", tag2fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg2", tag2bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg3", tag3fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg3", tag3bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg4", tag4fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg4", tag4bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg5", tag5fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg5", tag5bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg6", tag6fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg6", tag6bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg7", tag7fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg7", tag7bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg8", tag8fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg8", tag8bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.fg9", tag9fgcolor);
				XRDB_LOAD_COLOR("dwm.tags.bg9", tag9bgcolor);
				XRDB_LOAD_COLOR("dwm.tags.layoutfg", layoutfgcolor);
				XRDB_LOAD_COLOR("dwm.tags.layoutbg", layoutbgcolor);
				#endif

I would have preferred to commit these to the branch but my tree is kind of a mess right now and I don't have time.

@UtkarshVerma
Copy link
Contributor Author

@bakkeby any updates on this?

@bakkeby
Copy link
Owner

bakkeby commented Jul 6, 2022

Updates in terms of merging this in you mean?

I do not have any immediate plans for taking this for various reasons.

@UtkarshVerma UtkarshVerma mentioned this pull request Jul 7, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bakkeby bakkeby bakkeby left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@UtkarshVerma @bakkeby