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

Modified Installer #459

Merged
merged 11 commits into from
Dec 5, 2022
Merged

Modified Installer #459

merged 11 commits into from
Dec 5, 2022

Conversation

abvee
Copy link
Contributor

@abvee abvee commented Nov 24, 2022

Fixed some formatting for the installer and lessened the number of echo calls with here docs, etc.

Now, seperators are done properly, to the width of the terminal and are proper box dashes (as per the Unicode specification):

image

Adjusting the size adjusts the bar:

image

doc.
Here docs are more efficient as they only call once, and formatting
doesn't need to be done with escape characters, thus making code more
readable
@AdnanHodzic
Copy link
Owner

Very nice, I actually wanted to do this myself but never got around it. Two things:

  1. Adjusting the size of terminal will result in breaking the lines, i.e:
    This is an image

As from what I understood this doesn't happen for you? If I re-run the same command on new re-sized terminal width then line will be adjusted.

  1. Why is this only done for install, as for example remove will not use any of this new functionality. I.e
------------------------- auto-cpufreq daemon removed -------------------------

But I guess it's related to what I proposed in 4 as this stuff is used outside of installer

  1. Let's ident "For list of options, run:" to the left, as now it looks bit off:
auto-cpufreq tool successfully installed.

  		For list of options, run:
auto-cpufreq --help
  1. After we merge this, it would be great if you could make these same changes across auto-cpufreq output where sep and seperator are being used.

@abvee
Copy link
Contributor Author

abvee commented Nov 25, 2022

As from what I understood this doesn't happen for you?

Well, lines that are already drawn will not resize, but yes, running the command again in a resized terminal will adjust to the size.

Why is this only done for install, as for example remove will not use any of this new functionality.

I uh... didn't see that. I'll make some commits and fix those, they will probably use the same logic as the install heading 😄

Let's ident "For list of options, run:" to the left, as now it looks bit off:

Will do

After we merge this, it would be great if you could make these same changes across auto-cpufreq output where sep and seperator are being used.

The only problem with that is that I don't know python very well.... I usually program in c and make small shell scripts. It shouldn't be too hard though. I'll keep you informed if I cannot do it

@AdnanHodzic
Copy link
Owner

If it helps, there's even a snippet that will take care of terminal sizing/padding but was never merged with master branch, reference: #325 (comment)

Moved $COLOUMN into seperator function, so if the terminal is resized
while the script is running, a new seperator should be resized as well.
multiple times.

Made local variables to allow for neat drawing when resizing the
terminal.
@abvee
Copy link
Contributor Author

abvee commented Nov 25, 2022

I'll probably make a new branch and PR to merge anything outside the installer.

I took the liberty to replace Hyphens in the headers to an "equals to" sign (=):

image

image

Let me know if you prefer one over the other, and if there are any other problems 🗡️

P.S: Thanks for being so nice on my first PRs :)

@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Nov 25, 2022

I'll probably make a new branch and PR to merge anything outside the installer.

Definitely a good idea, since there will be lot more changes and it'll be a lot bigger PR.

I took the liberty to replace Hyphens in the headers to an "equals to" sign (=):

Let's please return these to ---, I find the output to be "too bulky" with ===. Ideally these would be lines as they are for seperator and could also scale. See if you could make it like that, as that would be excellent.

Let me know if you prefer one over the other, and if there are any other problems dagger

Also let's add an empty line before "Welcome to ..."

P.S: Thanks for being so nice on my first PRs :)

Most welcome, I'm honored to make your first PR a pleasant experience and too many more! 🍻

@abvee
Copy link
Contributor Author

abvee commented Nov 26, 2022

Ideally these would be lines as they are for seperator and could also scale. See if you could make it like that, as that would be excellent.

Also let's add an empty line before "Welcome to ..."

Done

They also dynamically size just like the seperator:

image

image

Most welcome, I'm honored to make your first PR a pleasant experience and too many more! beers

Thanks again :D.
Let's hope I have many more

@AdnanHodzic
Copy link
Owner

I've just noticed a regression introduced as part of #460, so let's resolve this first before this PR is merged.

@AdnanHodzic AdnanHodzic mentioned this pull request Nov 27, 2022
@AdnanHodzic
Copy link
Owner

AdnanHodzic commented Nov 30, 2022

I was about to merge these changes, but then I found out what was missing as part of #462, I won't revert the changes so maybe it's easiest to make the changes as part of this PR?

Currently, if you do sudo ./auto-cpufreq-installer --install and do not run sudo auto-cpufreq --install in meantime, but go straight to sudo ./auto-cpufreq-installer --remove you'll end up getting the following error:

Couldn't remove the auto-cpufreq daemon
/usr/local/bin/auto-cpufreq-remove or /usr/bin/auto-cpufreq-remove do not exist.

@abvee
Copy link
Contributor Author

abvee commented Dec 1, 2022

so maybe it's easiest to make the changes as part of this PR?

Okay, will do.

you'll end up getting the following error:

Okay, I'll keep the error, but remove the other files....

@abvee
Copy link
Contributor Author

abvee commented Dec 1, 2022

the auto-cpufreq --remove command already prints an error, so I'm not going to reprint it.

I also added a check for the wrapper script, so it should just work now

@AdnanHodzic
Copy link
Owner

Perfect! Thank you for your contribution and I'm making to be making a v1.9.7 release where you'll get credit for all your work :)

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