linutil
linutil copied to clipboard
ChrisTitusTech
Add an option to revert changes made by scripts [IMPORTANT]
Add an option to revert changes made by scripts
Merge #380 #382 #387 before this so @nnyyxxxx can start working on the gaming-setup & aurhelper reverts.
#387 includes a fix for clipping in the preview.
DO NOT MERGE THIS PR UNTIL EVERYTHING ELSE RELATED TO THE SHELL SCRIPTS HAS BEEN MERGED / CLOSED.
AFTER MERGING / CLOSING EVERYTHING ELSE @nnyyxxxx WILL COME IN AND UPDATE THE EXISTING / NEW SCRIPTS TO INCORPORATE THIS NEW FUNCTIONALITY INTO THEM.
Type of Change
- [x] New feature
- [x] Refactoring
- [x] UI/UX improvement
Description
This PR refactors every shell script to have run and revert functions & adds TUI functionality for reverting - r keybind.
Impact
Every shell script refactored, somme internal command handling changes.
Issue related to PR
- Resolves #240
- Resolves #260
- Resolves #362
- Resolves #401
Checklist
- [x] My code adheres to the coding and style guidelines of the project.
- [x] I have performed a self-review of my own code.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [x] My changes generate no errors/warnings/merge conflicts.
Credits
- @nnyyxxxx - shell scripts
I think it would be better if we stored a file containing which scripts have been run (in ~/.local/state), and replacing the displayed scripts with an entry to revert them. An boolean flag should be added (most likely defaulting to true) as to whether any given script supports the feature.
I think it would be better if we stored a file containing which scripts have been run (in ~/.local/state), and replacing the displayed scripts with an entry to revert them. An boolean flag should be added (most likely defaulting to true) as to whether any given script supports the feature.
I like our current approach better. I don't agree with the "replacing the displayed scripts with an entry to revert them" I think the "R" Keybind is best suited for the revert functionality. #293
There is no real reason someone would need to run the same script multiple times, and the revert functions will have certain expectations (e.g. some .bak file must exist), which would lead to undefined behavior when run without the initial script being run beforehand. I think the out of the box experience should be prioritized, rather than cramming linutil full of features that will negatively impact UX.
However, there definitely could be cases that the opposite script might be needed, so they should be made available in some way. Perhaps 'r' or another keybind could be used to swap between displaying the default & opposite entries. For example, pressing, or even holding, a keybind could cause changes similar to this. "Revert Alacritty Setup" => "Alacritty Setup" "Bash Prompt" => "Revert Bash Prompt" "AUR Helper" => "Revert AUR Helper"
There is no real reason someone would need to run the same script multiple times, and the revert functions will have certain expectations (e.g. some .bak file must exist), which would lead to undefined behavior when run without the initial script being run beforehand. I think the out of the box experience should be prioritized, rather than cramming linutil full of features that will negatively impact UX.
However, there definitely could be cases that the opposite script might be needed, so they should be made available in some way. Perhaps 'r' or another keybind could be used to swap between displaying the default & opposite entries. For example, pressing, or even holding, a keybind could cause changes similar to this. "Revert Alacritty Setup" => "Alacritty Setup" "Bash Prompt" => "Revert Bash Prompt" "AUR Helper" => "Revert AUR Helper"
I still don't agree with your approach; I think the revert functionality inside of the scripts is fine, for the rust part, yes that needs improvement. I definitely see where you're coming from but, I think the "R" Keybind functionality is still better for this Revert approach.
rather than cramming linutil full of features that will negatively impact UX.
I disagree with this, I don't think this will negatively impact the UX at all. The revert functionality in the scripts is very minimal and is just a starting point; can be improved upon in the future.
certain expectations (e.g. some .bak file must exist), which would lead to undefined behavior when run without the initial script being run beforehand
I disagree with this as well, take a look:
@adamperkowski I just finished the last script, DO NOT squash these commits. Also leave this as a draft until you fix the preview handling.
Every script has been tested, no bugs found.
There are scripts which intentionally shouldn't be reverted that must be handled (e.g. system update; reverting updates should always be done manually since it can be extremely risky). Those need to be handled properly. I don't think running the script and printing a message is the best solution to that.
There are scripts which intentionally shouldn't be reverted that must be handled (e.g. system update; reverting updates should always be done manually since it can be extremely risky). Those need to be handled properly. I don't think running the script and printing a message is the best solution to that.
Why wouldn't it be the best option? That function is only called when "R" is pressed on the keyboard. Otherwise running it via "ENTER" will only call the update functions and the revert functions won't be called.
Nobody will know which scripts do and don't support reversion until they try them, which is poor UX. At least if you want to go by this concept, you should dynamically handle adding the 'r' keybind to the list depending on whether reversion is supported for the given script.
Also, for non-script command entries, it's an extremely poor idea to run the same command when the user expects to revert changes, since depending on the script, it could delete backups and render reversion impossible.
Also, for non-script command entries, it's an extremely poor idea to run the same command when the user expects to revert changes, since depending on the script, it could delete backups and render reversion impossible.
I disagree with that, I think our approach is good and when trying to revert via "R" only the functions inside of the revert function get called, I personally don't see a problem with it and I think this is the best approach. Also the backup situation was already fixed via #389
you should dynamically handle adding the 'r' keybind to the list depending on whether reversion is supported for the given script.
I agree with this partially, I think it should be dynamically updated as well, but I still think having an echo even if the user tries to do it and it ouputs "Not supported" Is the best approach to this.
I think it would be better if we stored a file containing which scripts have been run (in ~/.local/state), and replacing the displayed scripts with an entry to revert them.
- This would break a lot in linutil
- The tool is designed to be used kinda on-the-fly -ish, sto storing the files is not the best idea IMO.
Also, for non-script command entries
Nobody will know which scripts do and don't support reversion until they try them, which is poor UX. At least if you want to go by this concept, you should dynamically handle adding the 'r' keybind to the list depending on whether reversion is supported for the given script.
For now, all scripts are reafctored to have revert functionality. Linutil mainly depends on sh scripts. When that changes, a lot of linutil would have to be changed, including revert functionality.
for non-script command entries, it's an extremely poor idea to run the same command when the user expects to revert changes
Apart from that, how would you see "non-script commands" in linutil? Even a very very basic system update requires a script. And how would they have a revert functionality?
There are scripts which intentionally shouldn't be reverted that must be handled (e.g. system update; reverting updates should always be done manually since it can be extremely risky). Those need to be handled properly. I don't think running the script and printing a message is the best solution to that.
The error message is to mitigate the issue you're referring to and it is being handled properly, not sure what you mean.
There is no real reason someone would need to run the same script multiple times, and the revert functions will have certain expectations (e.g. some .bak file must exist), which would lead to undefined behavior when run without the initial script being run beforehand. I think the out of the box experience should be prioritized, rather than cramming linutil full of features that will negatively impact UX.
However, there definitely could be cases that the opposite script might be needed, so they should be made available in some way. Perhaps 'r' or another keybind could be used to swap between displaying the default & opposite entries. For example, pressing, or even holding, a keybind could cause changes similar to this. "Revert Alacritty Setup" => "Alacritty Setup" "Bash Prompt" => "Revert Bash Prompt" "AUR Helper" => "Revert AUR Helper"
That would bloat the dir up with multiple scripts, I don't think you're considering that Linutil as of right now is still in its infant stage, making a separate revert script for every single script is going to cost a lot, me and Adams approach is much simpler.
I think it would be better if we stored a file containing which scripts have been run (in ~/.local/state), and replacing the displayed scripts with an entry to revert them. An boolean flag should be added (most likely defaulting to true) as to whether any given script supports the feature.
Again that would cost a lot, making a separate script for every single script in Linutil is going to cost a lot.
There is no real reason someone would need to run the same script multiple times
Again only the Revert function is being called, the rest of the script is ignored. This is a much simpler, and more performant approach compared to making a separate script for every single script in Linutil for reverting changes.
making a separate script for every single script in Linutil is going to cost a lot.
I'm not sure what you think I'm proposing. If we add a boolean flag for whether a script is revertable or not, we can execute scripts normally if they don't support it (without needing to modify the script itself whatsoever), and use the revert & run function if they do. We don't need to create extra scripts, and I never said anything along those lines.
Again only the Revert function is being called, the rest of the script is ignored.
This comment was referring to running the main function of the script again, not the revert function.
I'm not sure what you think I'm proposing. If we add a boolean flag for whether a script is revertable or not, we can execute scripts normally if they don't support it (without needing to modify the script itself whatsoever), and use the revert & run function if they do. We don't need to create extra scripts, and I never said anything along those lines..
That is LITERALLY what we are already doing with the keybind logic, I don't understand why you are suggesting that we do the same exact thing we are already doing except with replacing the entries with a (Revert "Script_name") The keybind approach is much similar and better than that + its being displayed in the dynamic hotkey list which I explained to you earlier in this conversation.
This comment was referring to running the main function of the script again, not the revert function.
Not sure what do you mean by "main function" and "running it again" and how is that relevant.
Also LJ you're trying to suggest collecting telemetry on the user based on which scripts are run which is a no go in my book.
Also LJ you're trying to suggest collecting telemetry on the user based on which scripts are run which is a no go in my book.
That is an absurd suggestion. Locally storing a list of completed script filenames is not telemetry. Your text editor is not collecting telemetry just because it has a list of recently opened files stored on your system.
Also LJ you're trying to suggest collecting telemetry on the user based on which scripts are run which is a no go in my book.
That is an absurd suggestion. Locally storing a list of completed script filenames is not telemetry. Your text editor is not collecting telemetry just because it has a list of recently opened files stored on your system.
It's not absurd at all, It's what you suggested we do with script entries. Your approach is indeed collecting data and monitoring whether that data changes based on what the user does.
Yes, the 'remote collection and transmission of data'. None of which is occurring by locally storing a file which is never transmitted over the internet.
@lj3954 Here's my opinion on your approach, I think its unnecessary, collecting data and monitoring that data for changes based on which entries the user makes is not the best idea, also our approach is much simpler to this and only requires a keybind. If you could explain to me why this approach is bad maybe I'd understand but your explanations so far are just justifying our approach even more.
@lj3954 move this somewhere else. This is definitely not the place to debate over telemetry definitions.
@lj3954 move this somewhere else. This is definitely not the place to debate over telemetry definitions.
Please direct this to the one who brought up in the first place the ridiculous idea that I'm suggesting "telemetry" additions: https://github.com/ChrisTitusTech/linutil/pull/384#issuecomment-2351744318.