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

systemd user service display support and manual setup docs #107

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

Conversation

nuclearglow
Copy link

  • adds graphical target to systemd user service
  • adds manual configuration section in systemd details docs

related: https://github.com/tmux-plugins/tmux-continuum/pull/106/files
related: https://github.com/tmux-plugins/tmux-continuum/pull/102/files

@bruno-
Copy link
Member

bruno- commented May 29, 2022

Hi,
I've usually just merged systemd contributions in the past without trying to understand everything.

But I feel more and more is unclear to me. For example: when should a user do manual systemd configuration vs letting this plugin auto configure things?

The current docs state this: The first time tmux starts when @continuum-boot is set to 'on' tmux-continuum will generate a user level systemd unit

@bruno-
Copy link
Member

bruno- commented May 29, 2022

Oh, and also: I would appreciate help with maintaining linux/systemd stuff.

@nuclearglow
Copy link
Author

nuclearglow commented May 29, 2022

@bruno- thanks for taking a look. You are of course right, the docs should be more clear about what to do. The autoconfiguration looks good, but this PR should be merged so that it works correctly. In my case, the systemd service file was not created, since I did not have the systemd user folder on my system. Hence, I was forced to manually configure the service (and the docs reflect this approach). I can work on this PR a bit more and try to help with the tmux / linux autoconfigure script. The manual approach could be converted to an explanation section. what do you think?

Regarding your second comment: While I use your great plugin and tmux daily, I could maybe help, but I am not a tmux expert at all. Let's work on improving support for linux / systemd for the plugin for now and then we shall see, alright?

@bruno-
Copy link
Member

bruno- commented May 30, 2022

@nuclearglow thanks for clarifying things. I have merged #106 based on your input.

The manual approach could be converted to an explanation section

I'm worried this becomes detailed documentation that duplicates the content from the actual scripts. For example: if the docs contain the systemd unit file, then whenever we make a change to scripts/handle_tmux_automatic_start/systemd_enable.sh we would need to update the docs as well.

I would rather focus on making sure our automated scripting approach works flawlessly. What do you think?

@bruno-
Copy link
Member

bruno- commented May 30, 2022

What about DISPLAY related lines from your PR? Is this still needed Environment=DISPLAY=${display}?

I could maybe help, but I am not a tmux expert at all. Let's work on improving support for linux / systemd for the plugin for now and then we shall see, alright?

That works. Yes, I only need help with linux and systemd so let's focus on that. My main problem is I don't have a linux machine where I can test stuff. And I'm not sure a virtual server has the same environment as a regular laptop installation.

Would you be ok if we contacted you for any change to these two files:

  • scripts/handle_tmux_automatic_start/systemd_enable.sh
  • scripts/handle_tmux_automatic_start/systemd_disable.sh

I was thinking we put the following header at the top of these files:

Maintainer: Sven Vowe @nuclearglow
Contact maintainer for any change to this file.

Let me know what you think?

@nuclearglow
Copy link
Author

@bruno- alright, that should work. I will try to help! Regarding the DISPLAY variable, I am not a 100% sure. I know that my tmux systemd service works fine right now, on different Ubuntu systems with different gpus (AMD Vega, NVidia RTX, Intel built-in) and with different monitor configurations. But it might be that if we wait for the graphical target anyway, it might not be needed. I think I will need to test that. I'll update the PR after I checked that.

@bruno-
Copy link
Member

bruno- commented Jun 3, 2022

alright, that should work. I will try to help!

Thanks. I have added you as a maintainer to systemd files via 9329afa

Regarding the DISPLAY variable, I am not a 100% sure. ... I think I will need to test that. I'll update the PR after I checked that

I propose you just open a new PR for DISPLAY variable, if you think a change is necessary.
Let's leave this PR focused on the documentation.

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