Skip to content

Conversation

WillB97
Copy link

@WillB97 WillB97 commented Mar 19, 2023

Uses systemd directly to start and restart the browser. Adds options for running livestream and using chromium on low-end devices.

Drops support for Firefox.

@PeterJCLaw
Copy link
Owner

Thanks for this contribution, however in future I would strongly advise discussing changes of this scale before starting development. While some of the changes here may be applicable, there are also a number of changes here which I don't think are desirable.

  • I don't think we want a default remote_ssh_port. I suspect this may have been an oversight originally, however the lack of it being set triggering an error is probably a good thing given the properties that it must have (I've added a comment with details in 7efed35).
  • I don't think we should drop support for Firefox
  • this seems to have lost the running of unclutter, which is useful to hide the mouse pointer (unless that's somehow handled by something else here?)
  • silencing the graphical under-voltage warnings without any mechanism to pass the issue back to the user -- it feels like this could easily create more issues than it solves

I'm guessing that with moving systemd you're hoping to address some of the process management issues that have been observed (#21, #8)? Those would indeed be great to resolve, so if that's the case it would be great to know if they are addressed here.

However, I anticipate that moving to systemd has a number of other knock-on effects. It's not immediately clear what those are nor whether or not they are desirable. (Forcing the control of the service to require sudo being one thing that comes to mind).

There are some useful things here though:

  • using configuration files for xscreensaver over the xset commands
  • the more detailed browser options (though it's not clear to me why these need to be contingent on the url being a livestream, rather than just always passing them)
  • knowing how to silence the undervoltage warnings

I would be happy to discuss these changes, however I would rather not merge them as a single PR.

I would also encourage in future that you self-review the commits before submitting them -- many of the commits here are missing details regarding the intent of their changes and/or contain extraneous changes unrelated to those described. This makes it both harder to review the changes and makes the history much less useful for future developers.

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