Added system font recognition for xfce-terminal #36

DarNCelsius · 2021-05-09T11:47:24Z

No description provided.

Hopefully an improvement

DarNCelsius · 2021-05-09T19:09:18Z

It's a little beefy perhaps. I can't but help wish we had an xml solution to use for XFCE and Openbox files. Would tidy this up quite a bit. XFCE WM Theme is also in a similar file, which I had plans to do next.
XFConf is an option too:

LinusDierheimer · 2021-05-09T19:42:11Z

Funny thing, i just pushed XFCE theme detection.

DarNCelsius · 2021-05-09T19:58:07Z

I see it. So this could be done a lot simpler.

LinusDierheimer · 2021-05-09T19:59:05Z

Yes i'm just at reviewing your PR

src/modules/terminalfont.c

LinusDierheimer · 2021-05-09T19:51:15Z

src/modules/terminalfont.c

+
+    ffParsePropFileConfig(instance, "xfce4/terminal/terminalrc", "FontUseSystem=%[^\n]", fontName.chars);
+
+    if((fontName.chars[0] == '\0') || !ffStrbufCompS(&fontName, "FALSE"))


A fontName.length == 0 is the better way to test for this when using strbufs.

Maybe an ffStrbufIgnCaseComp would be more save to get it right?

I think a ) == 0 is more readable as a ! which implicitly casts an uint32_t to a bool

LinusDierheimer · 2021-05-09T19:51:56Z

src/modules/terminalfont.c

+static void printXCFETerminal(FFinstance* instance)
+{
+    FFstrbuf fontName;
+    ffStrbufInitA(&fontName, 256);


is a capacity of 256 really needed?

LinusDierheimer · 2021-05-09T19:52:57Z

src/modules/terminalfont.c

+    FFstrbuf fontName;
+    ffStrbufInitA(&fontName, 256);
+
+    ffParsePropFileConfig(instance, "xfce4/terminal/terminalrc", "FontUseSystem=%[^\n]", fontName.chars);


You will need to do a ffStrbufRecalculateLength after directly writing to the char buffer.

LinusDierheimer · 2021-05-09T19:56:39Z

src/modules/terminalfont.c

+    ffStrbufAppendC(&absolutePath, '/');
+    ffStrbufAppendS(&absolutePath, ".config/xfce4/xfconf/xfce-perchannel-xml/xsettings.xml");


Those two lines can be combined to "/.config/x

LinusDierheimer · 2021-05-09T19:59:42Z

src/modules/terminalfont.c

+    FILE* file = fopen(absolutePath.chars, "r");
+    if(file == NULL)
+    {
+        ffPrintError(instance, FF_TERMFONT_MODULE_NAME, 0, &instance->config.termFontKey, &instance->config.termFontFormat, FF_TERMFONT_NUM_FORMAT_ARGS, "Couldn't open \"%s\"", absolutePath.chars);
+        ffStrbufDestroy(&absolutePath);
+        ffStrbufDestroy(&fontName);
+
+        return;
+    }
+
+    FFstrbuf matchText;
+    ffStrbufInitAS(&matchText, 64, "<property name=\"MonospaceFontName\" type=\"string\" value=\"");
+
+    char* line = NULL;
+    size_t len = 0;
+
+    while(getline(&line, &len, file) != -1)
+    {
+        ffStrbufAppendS(&fontName, line);
+        ffStrbufTrimLeft(&fontName, ' ');
+
+        if(ffStrbufStartsWithS(&fontName, matchText.chars))
+        {
+            ffStrbufSubstrAfter(&fontName, matchText.length -1);
+            ffStrbufSubstrBefore(&fontName, fontName.length - 4); // ["/>\n]
+            break;
+        }
+        ffStrbufClear(&fontName);
+    }
+    ffStrbufDestroy(&matchText);
+    ffStrbufDestroy(&absolutePath);
+
+    if(line != NULL)
+        free(line);
+
+    fclose(file);


I think this whole section can be replaced by a ffParsePropFileConfig, see my implementation in WM theme.

ffParsePropFileConfig fails if the line has different spacing though. That's why I trimmed the result before the compare. It's very fragile if someone decides to edit the file manually.

Could the function be modified to ignore leading/trailing whitespace somehow?

Leading would suffice.

Is there a use case for keeping leading white space intact?

I think its worth implementing a ffSettingsGetXFConf at this point using libxfconf. I will look into this soon.

But as a note, a single white space in the scanf format string counts as any number of white spaces in the scanned string, thats why i prepended a single white space before the actual format string.

Thanks for the info. I await the new functionality. 😁

You have waited long enough, here it is!

Nice. The new system looks a lot better too. 👍

LinusDierheimer · 2021-05-09T20:00:24Z

src/modules/terminalfont.c

+
+    fclose(file);
+
+    if(fontName.chars[0] != '\0')


fontName.length > 0

I was using length initially - it failed unless I used Recalculate. This approach worked though.

LinusDierheimer · 2021-05-09T20:01:10Z

src/modules/terminalfont.c


+static void printXCFETerminal(FFinstance* instance)
+{
+    FFstrbuf fontName;


With the following suggestions in mind, i'm not sure if we need a strbuf. Possible i'm wrong at this one.

I was just trying to avoid including strings.h altogether.

I am too, but if we never need to modify the buffer we need neither strbufs nor strings.h

I see. Will try. :)

DarNCelsius · 2021-05-12T05:02:31Z

Quite happy with this one, apart from the string.h requirement. I couldn't think of an elegant solution.

LinusDierheimer · 2021-05-12T17:52:36Z

This seems fine. There is no problem with including string.h, just with using some dangerous functions out of it. Good resource for this is the commit history of https://github.com/git/git/blob/master/banned.h. I don't see a problem with strcasecmp.

DarNCelsius and others added 4 commits May 9, 2021 13:40

Added system font recognition for xfce-terminal

719a5c9

Added check for manual edits

cf4b3c5

Removed auto include

89187e9

Tweaks

ef3f4d9

Hopefully an improvement

LinusDierheimer reviewed May 9, 2021

View reviewed changes

DarNCelsius added 2 commits May 11, 2021 20:18

Merge branch 'LinusDierheimer:master' into master

cac0c27

Rewrote function with XFConf

c549c7c

LinusDierheimer merged commit 1ece13c into fastfetch-cli:master May 12, 2021


		ffParsePropFileConfig(instance, "xfce4/terminal/terminalrc", "FontUseSystem=%[^\n]", fontName.chars);

		if((fontName.chars[0] == '\0') \|\| !ffStrbufCompS(&fontName, "FALSE"))

		ffStrbufAppendC(&absolutePath, '/');
		ffStrbufAppendS(&absolutePath, ".config/xfce4/xfconf/xfce-perchannel-xml/xsettings.xml");

Added system font recognition for xfce-terminal #36

Added system font recognition for xfce-terminal #36

Uh oh!

Conversation

DarNCelsius commented May 9, 2021

Uh oh!

DarNCelsius commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinusDierheimer commented May 9, 2021

Uh oh!

DarNCelsius commented May 9, 2021

Uh oh!

LinusDierheimer commented May 9, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarNCelsius commented May 12, 2021

Uh oh!

LinusDierheimer commented May 12, 2021

Uh oh!

Uh oh!

DarNCelsius commented May 9, 2021 •

edited

Loading