Use XDG_CACHE_HOME. by tupo2 · Pull Request #175 · jgmenu/jgmenu

tupo2 · 2022-06-21T14:56:23Z

Closes #174.

johanmalm · 2022-06-21T21:25:41Z

Thanks for doing such a thorough job. I've got a few comments and will do a review to discuss.

johanmalm · 2022-06-21T21:33:09Z

contrib/gtktheme/jgmenu-gtktheme.py

 def cache(themename):
-    """ save the theme-name to ~/.cache/jgmenu/.last-gtktheme """
+    """ save the theme-name to XDG_CACHE_HOME/jgmenu/.last-gtktheme """
+    from xdg.BaseDirectory import xdg_cache_home


Importing from xdg crashes for me - I obviously do not have the xdg module installed
In order to avoid creating work for distro maintainers (@johnraff ??) I prefer going manual on this one.
Any chance of manually using XDG_CACHE_HOME if set/non-empty or else use ~/.cache

Traceback (most recent call last): File "/home/johan/tmp/jgmenu/./contrib/gtktheme/jgmenu-gtktheme.py", line 106, in <module> main() File "/home/johan/tmp/jgmenu/./contrib/gtktheme/jgmenu-gtktheme.py", line 74, in main cache(themename) File "/home/johan/tmp/jgmenu/./contrib/gtktheme/jgmenu-gtktheme.py", line 60, in cache from xdg.BaseDirectory import xdg_cache_home ModuleNotFoundError: No module named 'xdg'

XDG_CACHE_HOME is unset by default in Debian, so a test to only use that envvar if it is set, otherwise reverting to the default $HOME/.cache, would be desirable IMO.

Most of these XDG envvars seem to be unset by default in fact - the user is only expected to set them if they want to use a custom value.

src/cache.c

johanmalm · 2022-06-21T21:46:00Z

src/cache.c

 	cache_location = xmalloc(sizeof(struct sbuf));
 	sbuf_init(cache_location);
-	sbuf_cpy(cache_location, CACHE_LOCATION);
+	if (access(xdg_cache_home, F_OK) == 0) {


Suggest: #define DEFAULT_CACHE_LOCATION="~/.cache" and then

if (xdg_cache_home && *xdg_cache_home) { sbuf_cpy(cache_location, xdg_cache_home) } else { sbuf_cpy(cache_location, DEFAULT_CACHE_LOCATION) } sbuf_addstr(cache_location, "jgmenu/icons") sbuf_expand_tilde.... and so on...

johanmalm · 2022-06-21T21:48:19Z

src/icon.c


 static struct sbuf icon_theme;

+extern struct sbuf *cache_location;


As discussed above, I prefer to remove this extern in favor of a cache_get_dir(). It's just tidier and seems less like using global variables.

tupo2 · 2022-06-22T15:28:29Z

Thanks for the suggestions, I took them into consideration in a second patch.

codecov-commenter · 2022-06-24T21:30:20Z

Codecov Report

Merging #175 (9c8c9a7) into master (57f5e88) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #175   +/-   ##
=======================================
  Coverage   38.78%   38.78%           
=======================================
  Files           8        8           
  Lines         856      856           
=======================================
  Hits          332      332           
  Misses        524      524

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57f5e88...9c8c9a7. Read the comment docs.

tupo2 · 2022-06-24T21:31:37Z

Improved the patch.

johanmalm · 2022-06-24T21:39:16Z

Thanks. I'll have a look tomorrow if that's okay.

johanmalm · 2022-06-25T09:26:54Z

I think we could simplify this even further 😄

I suggest the following (although have not tried putting it into code):

Just use cache_location as the one variable that stores the cache_location.
Set cache_location during cache_init(). Don't bother re-setting it later.
It's already free'd in cache_atexit_cleanup() so nothing to do there.

That way you can get rid of tmp_cl and tmp_icon_cl and associated cache_dir_cleanup()
and also re-write cache_icon_get_dir() to simply this:

char *cache_icon_get_dir(void)
{
	return cache_location.buf;
}

Also, in cache_get_dir() you call access() to check for the existance of
a cache dir (if XDG_CACHE_HOME is set). I'd argue that this is not needed.
If the user has specified an XDG_CACHE_HOME that doesn't exist, we just
mkdir_p() it rather than defaulting to DEFAULT_CACHE_LOCATION.

johanmalm · 2022-06-25T09:30:50Z

I'm happy to get rid of the following icon.c lines altogether. I actually think it's excessive logging which might be useful during development, but not for users.

if (nr_symlinks)
		fprintf(stderr, "info: created %d symlinks in %s\n", nr_symlinks, cache_icon_get_dir());

That would simplify things ever more 😃

tupo2 · 2022-06-25T12:06:51Z

That way you can get rid of tmp_cl and tmp_icon_cl and associated cache_dir_cleanup() and also re-write cache_icon_get_dir() to simply this:
char *cache_icon_get_dir(void)
{
	return cache_location.buf;
}

Good idea, I made some tests and it works fine.

Also, in cache_get_dir() you call access() to check for the existance of a cache dir (if XDG_CACHE_HOME is set). I'd argue that this is not needed. If the user has specified an XDG_CACHE_HOME that doesn't exist, we just mkdir_p() it rather than defaulting to DEFAULT_CACHE_LOCATION.

I don't understand here, because we need to test if XDG_CACHE_HOME is set or not.
And if it is not, use DEFAULT_CACHE_LOCATION.

tupo2 · 2022-06-25T12:07:29Z

I'm happy to get rid of the following icon.c lines altogether. I actually think it's excessive logging which might be useful during development, but not for users.
if (nr_symlinks)
		fprintf(stderr, "info: created %d symlinks in %s\n", nr_symlinks, cache_icon_get_dir());
That would simplify things ever more smiley

I prefer to keep it in case we forgot something and removes it later.

johanmalm · 2022-06-25T15:01:22Z

Also, in cache_get_dir() you call access() to check for the existance of a cache dir (if XDG_CACHE_HOME is set). I'd argue that this is not needed. If the user has specified an XDG_CACHE_HOME that doesn't exist, we just mkdir_p() it rather than defaulting to DEFAULT_CACHE_LOCATION.

I don't understand here, because we need to test if XDG_CACHE_HOME is set or not. And if it is not, use DEFAULT_CACHE_LOCATION.

You can test if the variable is unset and/or empty the the code below - without trying to access the file:

const char *xdg_cache_home = getenv("XDG_CACHE_HOME");
if (!xdg_cache_home)
  printf("unset\n");
if (xdg_cache_home && !*xdg_cache_home)
  printf("empty string\n");  /* in other words "" */

So in our case I'd suggest:

if (!xdg_cache_home || !*xdg_cache_home) {
  /* Use CACHE_DEFAULT_LOCATION */
}

If C is not the language you normally code in I can explain further if helpful 😄

johanmalm · 2022-06-25T17:17:08Z

I'm happy to get rid of the following icon.c lines altogether. I actually think it's excessive logging which might be useful during development, but not for users.
if (nr_symlinks)
		fprintf(stderr, "info: created %d symlinks in %s\n", nr_symlinks, cache_icon_get_dir());
That would simplify things ever more smiley
I prefer to keep it in case we forgot something and removes it later.

👍

tupo2 · 2022-06-25T17:39:30Z

So in our case I'd suggest:

if (!xdg_cache_home || !*xdg_cache_home) {
  /* Use CACHE_DEFAULT_LOCATION */
}

ok I didn't know about that, thanks.

johanmalm · 2022-06-26T21:20:29Z

struct sbuf *cache_location; should be static struct sbuf *cache_location;. Otherwise it defaults to extern which makes it global. static keeps it within the translation unit (this file) which it much tidier.

cache_icon_get_dir() leaks memory if called more than once - which is the case with the patch.
Both xmalloc() and sbuf_init() allocate memory.
We should only allocate/free variable cache_location once.

I suggest doing the allocation in cache_init(), so in practical terms move all the stuff in cache_get_dir() and cache_icon_get_dir() to cache_init()

If you think it's tidier, it could be broken out into a separate function static void whatever() but it should only be called once (from cache_init())

cache_icon_get_dir() - which is called from icon.c should just be reduced to:

char *cache_icon_get_dir(void)
{
	return cache_location->buf;
}

There is no need to do anything else in there... the variable cache_location->buf is stored for the life of the jgmenu instance, so cache_icon_get_dir() is merely a "getter" if you're used to that terminology. It was common in object orientated programming a few years ago :)

Use XDG_CACHE_HOME.

47b2847

johanmalm reviewed Jun 21, 2022

View reviewed changes

Use XDG_CACHE_HOME - patch2.

4c4b0a4

Use XDG_CACHE_HOME - patch3.

28699ce

Use XDG_CACHE_HOME - patch4.

7edfdba

Use XDG_CACHE_HOME - patch5.

9c8c9a7

johanmalm merged commit 9f6570d into jgmenu:master Jun 27, 2022


		static struct sbuf icon_theme;

		extern struct sbuf *cache_location;

Conversation

tupo2 commented Jun 21, 2022

Uh oh!

johanmalm commented Jun 21, 2022

Uh oh!

johanmalm Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

johnraff Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johanmalm Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

johanmalm Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

tupo2 commented Jun 22, 2022

Uh oh!

codecov-commenter commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tupo2 commented Jun 24, 2022

Uh oh!

johanmalm commented Jun 24, 2022

Uh oh!

johanmalm commented Jun 25, 2022

Uh oh!

johanmalm commented Jun 25, 2022

Uh oh!

tupo2 commented Jun 25, 2022

Uh oh!

tupo2 commented Jun 25, 2022

Uh oh!

johanmalm commented Jun 25, 2022

Uh oh!

johanmalm commented Jun 25, 2022

Uh oh!

tupo2 commented Jun 25, 2022

Uh oh!

johanmalm commented Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnraff Jun 22, 2022 •

edited

Loading

codecov-commenter commented Jun 24, 2022 •

edited

Loading

johanmalm commented Jun 26, 2022 •

edited

Loading