Make libXft and libfontconfig optional

[TAAPArthur]

Converting Xft to an optional dependency. Going to argue that a simple image viewer doesn't need font support. Disabling Xft also disables the bar but I didn't go so far as to change the cli options/man page.

[N-R-K]

From a quick glance

• This breaks (Xresources) custom window and mark color
• We're using tabs for indent and spaces for alignment, a lot of the alignment spaces got converted to tabs.

Might be other breaking changes introduced by this, for now these are the two I found.

[TAAPArthur]

This breaks (Xresources) custom window and mark color

Because Xft was used for the string to value conversion. Losing features by using less dependencies doesn't sound like an issue. Not sure if you were just pointing it out are citing it as a problem.

We're using tabs for indent and spaces for alignment, a lot of the alignment spaces got converted to tabs.

This is why tabs are evil. Not unwilling to fix the whitespace, but wanted to know if the change was interesting to people before I bothered.

[N-R-K]

Not against turning the status bar into an optional feature, but atm it just fractures how to configure the window colors into two completely different ways. Not a fan of that...

[N-R-K]

Any update on this? How hard would it be to add a simple parser so that existing Xresources config doesn't break?

[TAAPArthur]

Any update on this? How hard would it be to add a simple parser so that existing Xresources config doesn't break?

A simple parser to convert an arbitrary color name to a numerical value? By my definition, I don't think such a thing exists by my definition. Additionally, setting colors by hex value is simpler. Couldn't we just say that Xresources depend on Xft since currently that is what is used to parse them. The way #71 is looking users would have to explicitly opt out of it. Having config values as ints and having them be overridden by Xresources seems reasonable enough to me. Can change the code to that if that's better

I'd also add that only being able to change properties via Xresource seems like bad UI. Everything should be able to be done from the config file, but that may be more of a meta discussion.

[N-R-K]

A simple parser to convert an arbitrary color name to a numerical value?

Not color name, just "#123456" -> 0x123456
On that topic, does Xresources currently allow setting a color name, e.g "red"? If so then I guess we'll have to drop support for that if libXft was compiled out.

But parsing a hex color string, e.g "#123456" should be simple enough, no?

I'd also add that only being able to change properties via Xresource seems like bad UI.

I agree. If it was upto me I'd remove Xresources from mainline and put it up as a patch similar to other suckless softwares. But unfortunately we can't break backwards compatibility...

[TAAPArthur]

Not color name, just "#123456" -> 0x123456

Oh I didn't check. The defaults where just "white"/"black" so forget you could specify hex strings. That type of conversion would be trivial, but yeah it currently supports color names. Not sure if we only want to break some of them. If we are fine with color names depending on xft and parsing hex string normally then I'm down. Is there objection to keeping the config vars? The hex strings would replace them if present.

If it was upto me I'd remove Xresources from mainline and put it up as a patch similar to other suckless softwares. But unfortunately we can't break backwards compatibility...

I don't really care if it is in mainline or not. But anything that should be configured via the cli/Xresource should be able to be set from the config file is my stance. Going to do another pass and try to enforce this at least for Xresource.

Now I'm woundering how many features are present that no one uses/thinks should be in mainline.

[TAAPArthur]

• Added parsing for hex strings
• Added config var for mark color
• Have the config vars be defaults even when Xft is compiled in
• Fixed tab/space issues

[N-R-K]

With LibXft disabled, it just straight up segfaults. And with LibXft enabled, bar colors are not defaulting to the Xresouces win colors as it should.

Have you tested this (both LibXft on and off) on your end? I'd like to avoid putting effort into reviewing patches which are clearly broken.

[N-R-K]

Also not a fan of adding colors into config.h. Currently all colors are going into Xresources which I think is consistent enough and should be left as is.

Also atm, this patch doesn't allow bar colors in config.h but allows win and mark color. If we do decide to have config vars for colors, we should have all of them, not just 3 and leave out 2.

---

Also there's some style changes in this patch as well. It's not exactly a tiny patch, so the changes should be limited to just functional changes. Style fixes should go into #94

[TAAPArthur]

Have you tested this (both LibXft on and off) on your end? I'd like to avoid putting effort into reviewing patches which are clearly broken.

I'm sure I've commented before that you have a habit casually make such offensive, provocative statements to me that seem to have no purpose except to cause offense. Yes I have tested tested with Xft enabled/disabled. Tested with opening some random images both ways. I use it daily with libXft disabled. Tested it again just to be sure. I can open images without a crash of any kind. Want to share a stack trace or confirm you ran in a clean env, or just describe the problem in more detail?

Also not a fan of adding colors into config.h. Currently all colors are going into Xresources which I think is consistent enough and should be left as is.

Why? Why prevent users from setting defaults in their config file? It makes it hard to have a discussion when one side is just presented as an obvious truth with any justification. Why should you need xresources just to change default colors? And who are these 2 lines hurting?

Also there's some style changes in this patch as well. It's not exactly a tiny patch, so the changes should be limited to just functional changes.

Don't know what you are referring to nor do I think it is worth the effort. Is there something controversial hidden there?

[TAAPArthur]

Also atm, this patch doesn't allow bar colors in config.h but allows win and mark color. If we do decide to have config vars for colors, we should have all of them, not just 3 and leave out 2.

Right now bar bg/fg default to the window fg/bg. If we add the var to config we lose that explicit relationship. Maybe that's fine, but then should setting only window fg/bg in Xresource override the bar bg/fg in config.h? I wouldn't expect it to but doing otherwise would then change previous behavior. I of course don't really care either way since I'm not going to be using the bar anyways, but wasn't trying to change behavior for existing users

[N-R-K]

Right now bar bg/fg default to the window fg/bg.

Which was the problem. We're doing bg -> bg, fg -> fg. But looks like the manpage is wrong as well. (#106)

Why? Why prevent users from setting defaults in their config file? It makes it hard to have a discussion when one side is just presented as an obvious truth with any justification. Why should you need xresources just to change default colors? And who are these 2 lines hurting?

I think you've answered this yourself.

If we add the var to config we lose that explicit relationship. Maybe that's fine, but then should setting only window fg/bg in Xresource override the bar bg/fg in config.h? I wouldn't expect it to but doing otherwise would then change previous behavior.

This is exactly why I wanted to avoid adding the extra vars. Currently the color configuration is consistent, they're all configured in Xresources. By adding them in config.h we run into issue like this.

Don't know what you are referring to nor do I think it is worth the effort. Is there something controversial hidden there?

A couple places where multiline statement was turned into one without any other changes. We already have a PR open for dealing with such things, #94

I'm sure I've commented before that you have a habit casually make such offensive, provocative statements to me that seem to have no purpose except to cause offense.

First I compile with LibXft and see that the bar colors are not defaulting properly, then compile without it and it just segfaults. So I naturally ask weather you've tested this or not, because I find not one but two very visible faults...

[N-R-K]

or just describe the problem in more detail?

If any of the Nsxiv.* Xresources proprieties are active, that triggers it. And since asking question is apparently offensive and I do not how to ask this "politely" I'll just ask it straight,

Yes I have tested tested with Xft enabled/disabled.

Is this while having any Xresources propriety active or not? Following is mine:

! Nsxiv.mark.foreground: #FF3838
! Nsxiv.window.foreground: #eee0b8
! Nsxiv.window.background: #141818
! Nsxiv.bar.font: JetBrainsMonoNL Nerd Font Mono:antialias=true:size=12

Commented them all out, but any single one of them being active is enough to cause segfaults.

[TAAPArthur]

First I compile with LibXft and see that the bar colors are not defaulting properly, then compile without it and it just segfaults. So I naturally ask weather you've tested this or not, because I find not one but two very visible faults...

I don't know what is natural to you or not, but the comment about testing severed no purpose. What response could I have given that would have been productive to actually fixing the problem? Even if I take the comment to heart and attempt to test more, I don't know the direction to improve in since not enough info was given.

Have you tested this (both LibXft on and off) on your end? I'd like to avoid putting effort into reviewing patches which are clearly broken.

Seriously what do you expect my response to be to this? My "natural" response wouldn't have been anything productive. To me you are just making a personal attack about how I didn't bother to test and am wasting your time while providing minimal info to fix the bug because it is "clearly" obvious.

For the color bars, flipping the default colors isn't visually noticeable to me since I've never noticed the bar in the first place and have lately stopped using it. It is also just swapping two values so it isn't like the review is going to go through major changes because of that oversight. But yeah looked at the man page instead of the previous behavior. Thank you for pointing it out but don't think that was a fatal mistake.
But for the record your follow up of Which was the problem. We're doing bg -> bg, fg -> fg. would have been helpful. The issue was already fixed by then, but it made it clear what you meant and what the proper solution would be.

If you are looking for a non-offensive way to report a bug, in my opinion, your last response was a great example. It just stated what the problem was in a matter of fact manner and included steps to reproduce. Even included an xresource file to make things easier and sharing that it isn't specific to color properties is also nice to know. I'll look into it tonight. The part I found strange is that Xresources weren't loaded when Xft was disabled at the time you first reported the issue.

I think you've answered this yourself.

I asked like 4 questions and feel like we still aren't in agreement. Responding to some of them would at least help me see where/why we are disagreement. I really have no idea what you meant by this.

[N-R-K]

I asked like 4 questions and feel like we still aren't in agreement.

These are 4 things, that we wouldn't have to worry about at all if we keep colors to Xresources.

If other maintainers think it's worth the trouble to add colors to config.h as well, then I wont object. However what I will object to, is putting 3 colors in config.h but ignoring 2, that's just inconsistent and incomplete.

[eylles]

will test it in a while to see if i can reproduce the same error nrk got.

[TAAPArthur]

Can reproduce the segfault. Will have a fix shortly.

EDIT
For some reason the xresource type wasn't being loaded. Don't know why but right now your Xresource file works fine with or without xft enabled

[N-R-K]

For some reason the xresource type wasn't being loaded.

Think this should be investigated instead of removing the check.

Also currently (with libxft disabled) there's no border in thumbnails mode and neither is there any mark color. 3 char hex colors from Xresources, e.g "#f00", are also messed up.

[TAAPArthur]

3 char hex colors from Xresources, e.g "#f00", are also messed up.

Is this supposed to be red? I'm surprised Xresources even support that. Is this the only variant or do we have to worry about 1 or 12 length strings too? I think it is fair to lose that "convenience" without xft

[N-R-K]

do we have to worry about 1 or 12 length strings too?

We don't, because afaik those are not valid. 3 char hex colors on the other hand is a shorthand for 6 chars.

[TAAPArthur]

Also currently (with libxft disabled) there's no border in thumbnails mode and neither is there any mark color.

Bug that caused the foreground, background and mark color to all read from the same var. Fixed.

We don't, because afaik those are not valid. 3 char hex colors on the other hand is a shorthand for 6 chars.

Well Xft uses a 16 char format for whatever reason so just checking. Added support for these 3 char format and tested with the example in the wiki.

[N-R-K]

Before proceeding with a proper review, final gripe I have with this is that bar colors are not in config.h. Also I don't think the config.h color vars are changing, so they should be static const as well.

[N-R-K]

@TAAPArthur https://cgit.freedesktop.org/xorg/lib/libXft/tree/src/xftcolor.c

bool set_from_hex_string_if_present(win_env_t *e, win_t *win, XrmDatabase db,
                                    const char *name, unsigned long *dest) {
	XColor screen, exact;
	const char *hex = win_res(db, name, NULL);
	if (!XAllocNamedColor(e->dpy, e->cmap, hex, &screen, &exact))
		return false;
	*dest = exact.pixel;
	return true;
}

Didn't test too thoroughly but it seems to work fine. Both with hex strings and color name strings like "yellow".

[N-R-K]

Pushed the changes. Test it out and refactor things if everything is fine, otherwise feel free to revert.

Also I think we might be able to change the config.h colors to char and use either hex color strings "#fff" or "white" as before.

On second thought, let's keep this PR just to making LibXft optional. We can deal with config.h colors on a separate PR.

---

Figured out why Xresources was segfaulting, see caf5a56 @TAAPArthur

[N-R-K]

Aright, I'm happy with how this currently is. @TAAPArthur take a look and see if you find any problems. If not, then we can document the changes and then proceed with squash and merge.

---

Do we keep HAVE_LIBXFT or change it to something like HAVE_LIBXFT_FONTCONFIG or maybe HAVE_LIBFONTS ?

[TAAPArthur]

I'm in favor/neutral to the changes listed here. I kinda question the change in ifdefs. Don't think we are really consistent between ifdefs entire methods and parts of them. But don't really care and haven't checked if this is an existing problem.

Do we keep HAVE_LIBXFT or change it to something like HAVE_LIBXFT_FONTCONFIG or maybe HAVE_LIBFONTS ?

Don't have a strong opinion on this. Another point is that regardless of this flag we still search for font config headers... not sure if anyone cares and/or if this is worth fixing.

All in all, I'm basically fine to proceed.

[TAAPArthur]

Not really in favor for separating the config.h changes from this PR instead of just making them a separate commit, but if it makes reviewing easier and/or is the only thing blocking this I can live with it I guess.

But while are waiting on that final approval, may as well shift the focus to discussing that. Can move the conversation over to a new PR, if this closes first.
To re-iterate we have a problem where currently bar color defaults depend on the window colors such that changing just the window color changes the bar color. The question is should changing just the window color in Xresource change the bar color. We could

1. (yes) Keep this behavior and only expose window fg/bk and mark color to in config.h.
2. (no) Add all colors to config.h but additionally expose bar color in config and remove the association with window fg/bg. In other words bar colors won't be affect about by window properties in Xresource (they can still be explicitly set). This is the only option that breaks existing behavior, but this is better behavior in my opinion.
3. (yes). Add all colors to config.h but keep xresource behavior the same. So if one uses Xresource, the config values are ignored completely.
4. (yes) Add all colors to config.h , but only have Xresource window fg/bk change bar colors if the config values match. So if DEFAULT_WIN_FG == DEFAULT_BAR_FG and DEFAULT_WIN_BG != DEFAULT_BAR_BG, the the Xresource window properties are X and Y respectively and bar properties aren't set, then bar->fg == X and bar->bg == DEFAULT_BAR_BG != Y.
   5 (yes) Don't add the new config properties and prevent users from changing the default colors in their config.

My preference is 1 = 2 > 4 > 3 > 5 which may be (heavily) biased by the fact I don't use Xresource

[eylles]

dunno which one of the options you described would fit but i propose doing something like this
exposing all variables in config.h like this

/* appearance */
default_win_bg = "white";
default_win_fg = "black";
default_bar_bg = default_win_bg;
default_bar_fg = default_win_fg;
default_mrk_fg = default_win_fg;
/* note that these will be overridden by the colors set in Xresources */

and then in window.c something like:

	win_bg = win_res(db, RES_CLASS ".window.background", default_win_bg);
	win_fg = win_res(db, RES_CLASS ".window.foreground", default_win_fg);
	mrk_fg = win_res(db, RES_CLASS ".mark.foreground", default_mrk_fg);
...
	bar_bg = win_res(db, RES_CLASS ".bar.background", default_bar_bg);
	bar_fg = win_res(db, RES_CLASS ".bar.foreground", default_bar_fg);

[TAAPArthur]

That would basically be (2) but couldn't be done as described.
It would have to be

unsigned long default_win_bg = "white";
unsigned long default_win_fg = "black";
unsigned long default_bar_bg = "white";
unsigned long default_bar_fg = "black";
unsigned long default_mrk_fg = "black";

Because c. What you described would work fine if they were in a function but not naked in config.h

[eylles]

I only gave a rough example to be discussed.

[N-R-K]

but if it makes reviewing easier and/or is the only thing blocking this I can live with it I guess.

Yes, adding colors to config.h isn't as clean as I thought, so I'd much rather deal with that on a separate PR.

As for ifdef consistency, we have places in image.c as well as thumbs.c where we are ifdefing in the middle of function, so I'm fine with it.

Only remaining "problem" I have here is with HAVE_LIBXFT. I'm leaning towards HAVE_LIBFONTS but not exactly going to hold the PR for that. @XPhyro any thoughts on the name?

Another point is that regardless of this flag we still search for font config headers...

I'll look into it.

[XPhyro]

Only remaining "problem" I have here is with HAVE_LIBXFT. I'm leaning towards HAVE_LIBFONTS but not exactly going to hold the PR for that. @XPhyro any thoughts on the name?

Since this enables 3 dependencies related with fonts, I too think we can call this HAVE_LIBFONTS, as in a group name that has more than 1 dependency.

[TAAPArthur]

Manually squashed and merged

[XPhyro]

This one can stay as is, but for the future, I would rather the commit reference the PR it originated from like most of the others. Also, if somebody else committed to the PR, which in this case N-R-K did, I'd like them to be added as a co-author to not plagiarise their contribution.

[N-R-K]

The commit msg is wrong btw, says HAVE_LIBFONTS=1 to disable. Since there's a couple other issues too, I think I'll fix them and force push.

Also decided to change the main msg to "Make statusbar optional" which I think describes the end effect more clearly.