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.

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

Follow XDG base directory specification #104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TLATER
Copy link

@TLATER TLATER commented Feb 11, 2019

Fixes #95.

For now, this implements the second suggested strategy (continue supporting the old path with a deprecation warning, but prefer the new path), and splits the configuration and other data directories between $XDG_DATA_HOME and $XDG_CONFIG_HOME.

Furthermore, this branch makes the configuration directory configurable with -c. The profile/log/adblock directory is not configurable via CLI, this is intentional since IME other applications don't allow this either, and it seems a little pointless - the user should never need to touch these files anyway, and can always just use $XDG_DATA_HOME.

I also don't really see the point to adding a webmacs://config page just to display this directory. Perhaps if we allowed in-browser editing of the init module, but that's a bit beyond the scope of this patch.

I've done some smoketesting (and am using the branch as we speak!), but I can't get the tests to run :(

@TLATER
Copy link
Author

TLATER commented Feb 11, 2019

There's slightly unexpected behavior still, in that, if a user sets -c, but there is no configuration directory there, webmacs will still use an existing ~/.webmacs. Getting around that would be a bit less clean, and we do have the deprecation warning - opinions?

@parkouss
Copy link
Owner

Hey, thanks for looking into this. :) For your question, well it would be best to honor the -c option, but it is acceptable like this I think.

I agree with the changes here, but I thought about it a bit, and maybe we should also make use of the XDG_CACHE_HOME env var to put the cache used by the browser - which can be quite huge.

Though this will be a more complex change I think, so maybe we can start with this. Do you want me to merge the patch as it is now?

For the tests, have you followed https://github.com/parkouss/webmacs#running-tests? if yes, maybe you could tell me what's wrong, it is possible that the documentation here is incomplete.

@TLATER
Copy link
Author

TLATER commented Feb 12, 2019

@parkouss I haven't quite grokked the whole project yet, so I wasn't sure how to move around the browser cache - definitely agree though, it's very handy to just go rm -rf ~/.cache.

Maybe when I get a bit more time, might be a few weeks. It's up to you to tell if this is up to par as-is :)

I have a fairly exotic setup and herbstluftwm didn't seem to like it much. I frankly gave up pretty quickly, I'll put a bit more effort in next time and document anything I need to do to get it to work over here.

@TLATER
Copy link
Author

TLATER commented Feb 21, 2019

I've updated the patch :) It now also puts webmacs.ipc in $XDG_RUNTIME_DIR (#107) and splits off the cache directory into $XDG_CACHE_HOME/webmacs.

The default $XDG_RUNTIME_DIR is either [/usr]/run/user/<uid>, or if those don't exist /tmp/.webmacs - the XDG specification isn't perfectly clear on this, but I think that's implied from some prodding around other applications.

I don't think we need a separate deprecation warning for the cache, since it's fundamentally temporary data, so we should be free to clear it.

@TLATER TLATER force-pushed the tlater/xdg-base-dirs branch 2 times, most recently from f461474 to f0c962e Compare February 21, 2019 18:39
To become a better citizen, we now follow the XDG base directory
specifications - this makes it easier to configure where we dump data,
and helps avoid cluttering users' home directories.

We still read ~/.webmacs, but we give a deprecation warning - we
intend to ignore this directory in the future.
Copy link
Owner

@parkouss parkouss left a comment

Choose a reason for hiding this comment

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

This is great work. Thanks; I started to test this and it seems to work well.

I have two minor comments below, could you address them please if you agree? If possible at the least, I would like the adblock dir to go in cache if not too hard.

If you agree, please add new commits so it will be easier for me to review.

if os.path.isdir(deprecated):
# Since logging has not been setup yet, use manual print
print("Warning: '{}' as a data directory has been deprecated. "
"Please move your profile and adblock directories to '{}'."
Copy link
Owner

Choose a reason for hiding this comment

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

adblock directory is only used for cache, it should go in the xdg_cache_home too.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, the current profile dir unfortunately contain a "cache" subdirectory which should not be copied in new data dir. Maybe you could rephrase to ask for moving profile/default instead of just cache?

Copy link
Owner

Choose a reason for hiding this comment

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

also, I started to use the warning module in some other places, such as https://github.com/parkouss/webmacs/blob/master/webmacs/keymaps/__init__.py#L545. Do you mind giving it a try instead of raw prints?

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I'll get these things fixed. Sorry, I didn't realize the warnings module even existed!

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, no problem at all! It is not really a blocking issue but it would be more consistent.
I'll wait for your updates then. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants