Skip to content

Conversation

@FiXato
Copy link

@FiXato FiXato commented Jan 17, 2021

With the XDG Base Directory specification becoming more widely accepted, it would be nice if fzf-marks could also support this.

This change defaults the FZF_MARKS_FILE location to $XDG_DATA_HOME/fzf-marks/default_bookmarks.fzf-marks, which usually would be ~/.local/share/fzf-marks/default_bookmarks.fzf-marks.

A special case is still made however for the previous location of ~/.fzf-marks, so upgrading users won't suddenly find their marks empty. If no marks file is found in the new location, and one is found in the legacy location, then a warning is raised via STDERR, and FZF_MARKS_FILE is set to the legacy location. This will be done only once every time the script is sourced till the file is either moved, or FZF_MARKS_FILE is explicitly set.

I've only implemented this in Bash, as I'm not familiar with zsh myself, but it should probably be trivial to port over these changes to the zsh script too if this change is desirable.

While no support for config file(s) has been added yet (I do have some ideas for this), a CONFIG_DIR var has already been added too, so those also can be added in a location that follows the XDG Base Directories spec.

My changes follow examples as posted on https://www.ctrl.blog/entry/xdg-basedir-scripting.html

With the [XDG Base Directory specification](https://specifications.freedesktop.org/basedir-spec/latest/) becoming more widely accepted, it would be nice if fzf-marks could also support this.

This change defaults the FZF_MARKS_FILE location to $XDG_DATA_HOME/fzf-marks, which usually would be ~/.local/share/fzf-marks.

A special case is still made however for the previous location of ~/.fzf-marks, so upgrading users won't suddenly find their marks empty. If no marks file is found in the new location, and one is found in the legacy location, then a warning is raised via STDERR, and FZF_MARKS_FILE is set to the legacy location. This will be done only once every time the script is sourced till the file is either moved, or FZF_MARKS_FILE is explictly set.

I've only implemented this in Bash, as I'm not familiar with zsh myself, but it should probably be trivial to port over these changes to the zsh script too if this change is desirable.

While no support for config file(s) has been added yet (I do have some ideas for this), a CONFIG_DIR var has already been added too, so those also can be added in a location that follows the XDG Base Directories spec.

My changes follow examples as posted on https://www.ctrl.blog/entry/xdg-basedir-scripting.html
Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

I think it is useful to add the same codes to the zsh config (fzf-marks.plugin.zsh). [Edit: sorry, I missed the corresponding paragraph in the original post. Actually you can apply exactly the same changes to the zsh config because the changes you have made in the bash config are completely valid also in zsh.] I also have some comments as follows. Thank you!

fi

if [[ ! -f ${FZF_MARKS_FILE} ]]; then
if [[ ! -f "${FZF_MARKS_FILE}" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

These quotes (") are not needed in the conditional construct [[ ... ]]. In the conditional construct, arguments are not subject to word splitting and pathname expansions.

Similarly, the right-hand side of the variable assignments is neither subject to word splitting and pathname expansions, so the quotes in FZF_MARKS_FILE="${DATA_DIR}/default_bookmarks.fzf-marks" are also unneeded.

Copy link
Author

Choose a reason for hiding this comment

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

I guess I'm just used to using double quotes where possible to prevent word splitting. :)
Is there a downside to using quotes inside the conditional construct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I think there is no downside to quote the parameter expansion, so yeah, this is a matter of preference. One of the reasons that the keyword [[ is introduced as a variant of the command [ is to resolve quotation problems in [, so I just felt it natural to quote the arguments only when necessary.

Copy link
Owner

Choose a reason for hiding this comment

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

@akinomyoga makes a good point. At some point in the future, I'll go through the codes and make sure we are consistent everywhere. Same with $FZF_MARKS_FILE vs ${FZF_MARKS_FILE}. Since the quotes, and in the latter case the curly brackets, are unnecessary, we could do away with them, but for this pull request this does not matter much.

- lowercased and prefixed private variable names with _fzm_
- removed double quotes where not deemed necessary, such as inside double-bracketed conditional constructs.
- replaced `mkdir && chmod` with `(umask; mkdir)` suggestion.
Copy link
Owner

@urbainvaes urbainvaes left a comment

Choose a reason for hiding this comment

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

Thank you very much for your suggestions, following the XDG conventions seems very sensible!

if [[ ! -f "${FZF_MARKS_FILE}" ]]; then
# Fallback support for the previous default location of the bookmarks file.
if [[ -f "$LEGACY_FZF_MARKS_FILE" ]]; then
echo "Your FZF_MARKS_FILE is still in its legacy location of $LEGACY_FZF_MARKS_FILE; you might want to consider moving it to "$FZF_MARKS_FILE" so it follows the XDG standard, or explicitly set FZF_MARKS_FILE='$LEGACY_FZF_MARKS_FILE'." >&2
Copy link
Owner

@urbainvaes urbainvaes Jan 18, 2021

Choose a reason for hiding this comment

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

Are the quotes around "$FZF_MARKS_FILE" necessary here? Since there are no quotes around $LEGACY_FZF_MARKS_FILE, it might be good to remove them for consistency?

Copy link
Author

Choose a reason for hiding this comment

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

I believe I already changed that based on @akinomyoga 's suggestions :)
See 462bfd9 :)

Copy link
Owner

Choose a reason for hiding this comment

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

Looks good, many thanks! :)

@FiXato
Copy link
Author

FiXato commented Jan 18, 2021

Don't really have time today to further review and change the variable names, but hopefully tomorrow or later this week. :)

Once the changes to the Bash version are satisfactory, I'll see about applying them to the zsh script. The fish version might be easier to be updated by someone who has experience with fish. :)

Note that my current changes just take the user XDG_*_HOME dirs into account. I guess that ideally it should also look into $XDG_DATA_DIRS (and for configs later on in $XDG_CONFIG_DIRS) for existing mark files, at least for reading. This might be more of a future improvement for another ticket though. :)

@urbainvaes
Copy link
Owner

This might be more of a future improvement for another ticket though. :)

I agree. Your current suggestion is already a big improvement! Thanks again.

@urbainvaes
Copy link
Owner

Once the changes to the Bash version are satisfactory, I'll see about applying them to the zsh script.

Do you suggest that we merge this now and look at the zsh version later, or do you intend to apply the zsh changes also for this pull request?

Have a good weekend!

@FiXato
Copy link
Author

FiXato commented Jan 22, 2021

Do you suggest that we merge this now and look at the zsh version later, or do you intend to apply the zsh changes also for this pull request?

I meant that once I have applied the last changes I still need to apply to the Bash version, and you both have no further comments on it, I'll apply the same changes to the ZSH script in this same PR, and then you can merge the changes in a single PR. :)

I hope to have some time to work on that tomorrow. :)

(Though it might be an idea to pull in the changes to a separate feature branch of your own, so you can apply similar changes to the Fish script so there is no difference expected file locations regardless of shell once it's pulled into the main branch?)

@daiaji
Copy link

daiaji commented Nov 30, 2022

Hope to merge soon.

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.

4 participants