search

sonic2kk / steamtinkerlaunch

Linux wrapper tool for use with the Steam client for custom launch options and 3rd party programs
GNU General Public License v3.0
2.03k stars 69 forks source link

Improve installation clarity in README per discussion in #1118 #1127

Open OrderedSet86 opened 2 weeks ago

OrderedSet86 commented 2 weeks ago

Changes:

sonic2kk commented 2 weeks ago

Thanks!

I think the note about Repology is unnecessary as this is a standard embed, it is not maintained by us. Issues with green being confusing should be taken to upstream Repology, although it has never been an issue for any other project, including ones where using the master branch is expected (such as Valve's GameScope, which also includes a Repology section).

I have a couple of notes on the changes but apart from thinking the note about Repology is unnecessary, the changes overall are good from a glance. I'll take a closer look at how the Markdown parses out and leave some comments accordingly, but the spirit of this is all good :+1: Thanks for taking the time to look at it.

sonic2kk commented 2 weeks ago

Left a couple of comments, they aren't major blockers but just outline my own thoughts on wording and potential changes, but I'd also like you hear your thoughts.

An unrelated note: You've pushed this change to the master branch of your fork. I typically squash-merge changes, meaning changes get condensed into a single commit into master that won't include your original commit. This can make keeping your fork up-to-date potentially tricky because the history is technically out-of-sync (the changes are the same but the commits are not). It is possible, one may might be to rollback to the last commit before yours (effectively removing your commits), and then pull in master from this repo (pulling in the changes created by the merge commit of this PR). But Git is a funny tool, there may be other ways I'm unaware of, or maybe GitHub can resolve this when updating forks with some magic!

I've committed directly to my fork's master branch on KDE Invent before and didn't realise or have any idea how to fix it, so hopefully this is a helpful heads up!

OrderedSet86 commented 2 weeks ago

An unrelated note: You've pushed this change to the master branch of your fork. I typically squash-merge changes, meaning changes get condensed into a single commit into master that won't include your original commit. This can make keeping your fork up-to-date potentially tricky because the history is technically out-of-sync (the changes are the same but the commits are not). It is possible, one may might be to rollback to the last commit before yours (effectively removing your commits), and then pull in master from this repo (pulling in the changes created by the merge commit of this PR). But Git is a funny tool, there may be other ways I'm unaware of, or maybe GitHub can resolve this when updating forks with some magic!

The way I handle this on my repository is... well, just being lazy and merging without squashing using the Github Web UI, but if I wanted to do your workflow (squashing):

  1. I use the Github CLI (gh checkout pr 1127) to check out PR branches I want to make changes on (for some reason vanilla Git makes this very difficult)
  2. After I am doing making changes, I would git rebase master (with the PR candidate checked out), then checkout master and merge the candidate branch. Since a linear commit history is created (thanks to rebase) there is no merge message.

You could also squash on master and --force-with-lease push, but assume history overwriting like that will probably break something if you have any CI. Or perhaps one of the downstream tools. I wouldn't really recommend this method. But it would work for having a clean commit history!

I think the note about Repology is unnecessary as this is a standard embed, it is not maintained by us. Issues with green being confusing should be taken to upstream Repology, although it has never been an issue for any other project, including ones where using the master branch is expected (such as Valve's GameScope, which also includes a Repology section).

This is another situation where I think it's not "technically" correct to say something about Repology, sure, but it impacts user experience if they are mistakenly mislead into thinking that green = good (like it would in any other status UI). I would recommend keeping it (perhaps a weaker warning if you really want), but you are the maintainer and ultimately get to decide. The rest of the changes are fine by me.

sonic2kk commented 2 weeks ago

However you want to resolve the conflicts is totally fine, it was just a heads up in case when this gets merged it causes any confusion. I would rather not rewrite any master history (I know of at least two downstream tools using SteamTinkerLaunch), so either of the other two methods is probably fine (rebasing on master would probably be what I would do, to avoid the GitHub commandline tools as much as possible).

This is another situation where I think it's not "technically" correct to say something about Repology, sure, but it impacts user experience if they are mistakenly mislead into thinking that green = good (like it would in any other status UI).

The reason I think it is unnecessary is because this has never came up before; for this project or for any others that I have seen. Repology is widely used, surely someone would've said something if it was such a major issue. All it does is display which packages are using the latest tagged release. If there is a way to alter this of change the colour I would accept a PR for that, but to my understanding, there is not.

I get what you're saying, and if SteamTinkerLaunch were a tool designed with the average user in mind who might not have visited GitHub before, I might agree! But SteamTinkerLaunch is not, so we should look at this through the lens of a Linux "power user".

Criticisms about the Replogy UI should be brought up upstream, I'm not sure if this was ever discussed upstream before or if there is customisation available, but again, this has never seemed to be a problem before for any other project I've see, and Repology is quite widely used.

perhaps a weaker warning if you really want

I think warning a user that their repos packages may be out of date is a bit unnecessary, since that is all this Repology table shows; distribution package status.

I think the changes for the package statuses are unnecessary. The rest of the changes are good though, just a few style/wording nits.

I'm happy to merge this PR whenever those changes are applied. :slightly_smiling_face: