-
Notifications
You must be signed in to change notification settings - Fork 69
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.
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
Make libXft and libfontconfig optional #95
Conversation
From a quick glance
Might be other breaking changes introduced by this, for now these are the two I found. |
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.
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. |
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... |
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. |
Not color name, just "#123456" -> 0x123456 But parsing a hex color string, e.g "#123456" should be simple enough, no?
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... |
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.
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. |
|
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. |
Also not a fan of adding colors into Also atm, this patch doesn't allow bar colors in 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 |
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?
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?
Don't know what you are referring to nor do I think it is worth the effort. Is there something controversial hidden there? |
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 |
Which was the problem. We're doing bg -> bg, fg -> fg. But looks like the manpage is wrong as well. (#106)
I think you've answered this yourself.
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.
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
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... |
If any of the
Is this while having any
Commented them all out, but any single one of them being active is enough to cause segfaults. |
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.
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. 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 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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will test it in a while to see if i can reproduce the same error nrk got.
Can reproduce the segfault. Will have a fix shortly. EDIT |
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. |
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 |
We don't, because afaik those are not valid. 3 char hex colors on the other hand is a shorthand for 6 chars. |
Bug that caused the foreground, background and mark color to all read from the same var. Fixed.
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. |
Before proceeding with a proper review, final gripe I have with this is that bar colors are not in |
@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". |
Pushed the changes. Test it out and refactor things if everything is fine, otherwise feel free to revert.
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 |
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 |
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.
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. |
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.
My preference is 1 = 2 > 4 > 3 > 5 which may be (heavily) biased by the fact I don't use Xresource |
dunno which one of the options you described would fit but i propose doing something 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); |
That would basically be (2) but couldn't be done as described.
Because c. What you described would work fine if they were in a function but not naked in config.h |
I only gave a rough example to be discussed. |
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 Only remaining "problem" I have here is with
I'll look into it. |
Since this enables 3 dependencies related with fonts, I too think we can call this |
Manually squashed and merged |
libXft and libfontconfig are now optional dependencies and can be disabled with `HAVE_LIBFONTS=1` . Disabling them also disables the bar. Also when disabled freetype2 headers won't be searched.
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. |
The commit msg is wrong btw, says Also decided to change the main msg to "Make statusbar optional" which I think describes the end effect more clearly. |
libXft and libfontconfig are now optional dependencies which can be disabled via `HAVE_LIBFONTS=0`. Disabling them means disabling the statusbar. This also does not search for freetype2 header if disabled. Co-authored-by: NRK <nrk@disroot.org>
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.