Skip to content

config: add touch_on_select #246

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

N-R-K
Copy link
Contributor

@N-R-K N-R-K commented Mar 12, 2025

this makes an entry bump to the newest slot when serving (e.g selecting via clipmenu).

@N-R-K N-R-K marked this pull request as ready for review March 12, 2025 23:51
@cdown
Copy link
Owner

cdown commented Mar 23, 2025

Hmm, clipserve is somewhat internal and can also be called when starting clipboard ownership. What are your thoughts on doing it from clipmenu when selecting the clip?

@N-R-K
Copy link
Contributor Author

N-R-K commented Mar 24, 2025

Hmm, clipserve is somewhat internal and can also be called when starting clipboard ownership.

In those cases, isn't the entry already on top and thus make_newest() would be a no-op?

@cdown
Copy link
Owner

cdown commented Mar 24, 2025

I was going to say we can end up racing and bumping something from another selection, but I suppose that's no worse than the current situation since post-fork execution is unbounded, so I think you're right that it doesn't matter.

Maybe since "serve" is a bit internal, it should be "touch on select"? What do you think?

N-R-K added 3 commits March 24, 2025 08:40
WEXITSTATUS is only valid if WIFEXITED returned non-zero.
this makes an entry bump to the newest slot when it is selected
via `clipmenu`.
@N-R-K
Copy link
Contributor Author

N-R-K commented Mar 24, 2025

Maybe since "serve" is a bit internal, it should be "touch on select"?

Sounds fine to me.

I was going to say we can end up racing and bumping something from another selection, but I suppose that's no worse than the current situation since post-fork execution is unbounded, so I think you're right that it doesn't matter.

Since it's called touch_on_select now, I guess it makes more sense to put it in clipmenu.c now, even if the outcome doesn't really change.

While doing that, I noticed it was using WEXITSTATUS without checking WIFEXITED first, so fixed that. Also added EINTR handling too.

@N-R-K
Copy link
Contributor Author

N-R-K commented Mar 24, 2025

The CI failure seems to be unrelated? It passed earlier on my repo.

@N-R-K N-R-K changed the title config: add touch_on_serve config: add touch_on_select Mar 24, 2025
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