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

Add an option to revert changes made by scripts [IMPORTANT] #384

Closed
wants to merge 22 commits into from

Conversation

adamperkowski
Copy link
Collaborator

@adamperkowski adamperkowski commented Sep 15, 2024

Add an option to revert changes made by scripts

Merge #335 #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

  • New feature
  • Refactoring
  • 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

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no errors/warnings/merge conflicts.

Credits

@adamperkowski

This comment was marked as resolved.

@nnyyxxxx
Copy link
Contributor

I'll be working on this with Adam.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

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.

@nnyyxxxx

This comment was marked as off-topic.

@nnyyxxxx nnyyxxxx mentioned this pull request Sep 15, 2024
6 tasks
@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

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"

@nnyyxxxx

This comment was marked as off-topic.

@nnyyxxxx

This comment was marked as off-topic.

@nnyyxxxx

This comment was marked as off-topic.

@nnyyxxxx

This comment was marked as resolved.

@nnyyxxxx

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

I think this conversation should stop right here.

I agree. I don't agree with the added complexity and performant hurting suggestions from LJ, this debate has gone for too long.

I have repeatedly shown how your argument regarding 'performance' is not consistent whatsoever, since you misinterpreted one of my suggestions to mean something which would have a far greater impact on performance, and still agreed it would be a good idea.

@adamperkowski

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

Trying to stop discussion on implementation details is not reasonable behaviour when you're proposing major changes.

Like I said, comments like "clearly performance isn't your top concern" or "153 must be reverted then" have nothing to do with implementation details.

I absolutely won't accept someone arguing that my suggestion should be rejected solely because of performance impacts, when they support several other changes that similarly improve the program while hurting performance. You can't pick whichever argument is convenient at the time.

The one thing you're correct about is that my suggestion would involve more complexity. I've been very transparent in that. I just think the improvement in UX is more important than some complexity in the codebase.

@nnyyxxxx

This comment was marked as off-topic.

@nnyyxxxx

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

some users would actually like to have the option to reinstall instead of the entry being replaced with a revert entry

Can you please read my suggestions? I proposed making the 'r' key invert which entries are shown in the UI, which would very much allow users to choose whether they want to run the initial script again.

#338 also poses quite a challenge for your implementation; I'm building mine directly upon it.

@adamperkowski

This comment was marked as off-topic.

@nnyyxxxx

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

If you're going to do that then don't steal any of my script implementation

Theft, clearly an accurate way to describe using permissively licensed code, especially when released publicly under the same license while even maintaining the original commits & authorship.

when you could just use R to execute the revert function immediately

One of the major gripes I had with that was #338. I expect that PR to be merged. Someone will have to deal with the issues that causes.

then change one functionality to invert entries

I'm adding a lot of complexity, but I'm also apparently "stealing" "your" very simple implementation. Which one is it now?

This is a ridiculous debate, there is no value in it when it's entirely comprised of unfounded accusations.

@nnyyxxxx

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

And what issue will that cause?

You'll need to be able to select both items to be reverted & those to be ran normally, and be able to show those differently.
It's easy to say that it's simple when you aren't the one writing any part of it.

@nnyyxxxx
Copy link
Contributor

And what issue will that cause?

It's easy to say that it's simple when you aren't the one writing any part of it.

Never said that #338 was simple but I understand what you mean in:

You'll need to be able to select both items to be reverted & those to be ran normally

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

https://github.com/lj3954/linutil/tree/add_revertable_commands

Please critique my proposed changes. Specifically, lj3954@78aee43, where all the major changes are made. Other than that, changes to scripts are from this branch, other refactoring in core is from #247, and multi selection is from #338. I've based my changes on those branches because I expect them to be merged regardless, and I don't want to cause any conflicts since they all make major changes.

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Sep 15, 2024

https://github.com/lj3954/linutil/tree/add_revertable_commands

Please critique my proposed changes. Specifically, lj3954@78aee43, where all the major changes are made. Other than that, changes to scripts are from this branch, other refactoring in core is from #247, and multi selection is from #338. I've based my changes on those branches because I expect them to be merged regardless, and I don't want to cause any conflicts since they all make major changes.

Yeah I'm good @lj3954 you're just taking my work & Adams work and adding unneeded functionality onto it; I hope chris does not end up merging that... Where is the morality?

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

you're just taking my work

Yes, by merging your commits onto my branch. Chris is also "taking" your work whenever he merges your PRs, right?

& Adams work

Our implementations are completely unique; I'd like for him to read what I've written and make appropriate comments. I'd of course like to hear input from others as well, such as yourself, but you're of course a third party to the changes within the UI, having not made any within this PR.

Where is the morality?

It is unbelievable to me that you think there's any sort of moral issue with what I'm doing. The only thing I've written is a competing UI/UX implementation to that written by @adamperkowski. Your script changes are independent of that, both UIs run them (nearly) identically. I've dropped just one commit of yours (9a5839e), since scripts not supporting reversion are handled within the UI instead.

@lj3954 lj3954 mentioned this pull request Sep 15, 2024
7 tasks
@adamperkowski

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

still i didn't see "yeah sure you can use my work" from nyx

The MIT license the code is contributed under explicitly gives me all permissions I need to include the code. I'm sorry, I don't need written permission to use someone's work with the rights the license grants me, that is absurd and would slow the development of any open source project to a complete halt.

It's true he doesn't agree with my changes- well, purports to. Perhaps he just dislikes how I've disagreed with him on certain things. Whatever the case, writing code does not give one complete control over the implementation details relating to how the code is used. Would there be a difference if I made my PR after this one was merged, only removing your code and adding mine? I would doubt that.

... just to ignore it again?

I'm looking for input on what I've written, not nonsense about how I've "stolen" his work by bringing his commits to my branch with full credit given.

Again, we're not here to compete.

We're here to offer competing solutions to the same problem. Rejecting all competition doesn't help anyone, a project is only going to improve more if multiple people are able to suggest different ideas, that the merits of are then discussed. This discussion has led to improvements suggested to both solutions. For example, dynamically displaying 'r' in the keyboard shortcuts depending on whether or not the script supports reversion. That idea wouldn't have come up without this discussion.

@nnyyxxxx
Copy link
Contributor

still i didn't see "yeah sure you can use my work" from nyx

The MIT license the code is contributed under explicitly gives me all permissions I need to include the code. I'm sorry, I don't need written permission to use someone's work with the rights the license grants me, that is absurd and would slow the development of any open source project to a complete halt.

It's true he doesn't agree with my changes- well, purports to. Perhaps he just dislikes how I've disagreed with him on certain things. Whatever the case, writing code does not give one complete control over the implementation details relating to how the code is used. Would there be a difference if I made my PR after this one was merged, only removing your code and adding mine? I would doubt that.

... just to ignore it again?

I'm looking for input on what I've written, not nonsense about how I've "stolen" his work by bringing his commits to my branch with full credit given.

Again, we're not here to compete.

We're here to offer competing solutions to the same problem. Rejecting all competition doesn't help anyone, a project is only going to improve more if multiple people are able to suggest different ideas, that the merits of are then discussed. This discussion has led to improvements suggested to both solutions. For example, dynamically displaying 'r' in the keyboard shortcuts depending on whether or not the script supports reversion. That idea wouldn't have come up without this discussion.

@lj3954 Again morality != law , and if you're going to make a separate PR at least have the morality to rewrite the revert functions I made.. Don't just take my work without explicit permission, this is disrespectful especially when it's a "competition between 2 PRs" and once again morality != law.

@adamperkowski

This comment was marked as off-topic.

@lj3954
Copy link
Contributor

lj3954 commented Sep 15, 2024

I'd invite everyone to read #401 (comment) for the end of this discussion about 'theft'.

@adamperkowski

This comment was marked as off-topic.

@adamperkowski
Copy link
Collaborator Author

adamperkowski commented Sep 16, 2024

To be reworked

@nnyyxxxx
Copy link
Contributor

Closed for now, I'm going to keep working on this and maintain it with adam. We'll make a new PR in a month or so.

@adamperkowski adamperkowski deleted the revert branch October 3, 2024 17:02
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.

uninstall? undo the changes ? Revert changes
4 participants