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

mp3tag: Enhance pre_install script, add pre_uninstall script, and change persist #9840

Merged
merged 4 commits into from
Dec 8, 2022

Conversation

FlawlessCasual17
Copy link
Contributor

@FlawlessCasual17 FlawlessCasual17 commented Nov 25, 2022

This allows for mp3tag to be added as a shell extension when the user installs this app.

@FlawlessCasual17
Copy link
Contributor Author

/verify

@FlawlessCasual17 FlawlessCasual17 changed the title mp3tag: Enhance pre_install script, add pre_uninstall, and change persist mp3tag: Enhance pre_install script, add pre_uninstall script, and change persist Nov 25, 2022
@github-actions
Copy link
Contributor

All changes look good.

Wait for review from human collaborators.

mp3tag

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate

Copy link
Member

@HUMORCE HUMORCE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exists restrict of installation

@FlawlessCasual17
Copy link
Contributor Author

@HUMORCE can you read my comment on the manifest when you have the time?

@HUMORCE
Copy link
Member

HUMORCE commented Nov 29, 2022

where is it?

@FlawlessCasual17
Copy link
Contributor Author

FlawlessCasual17 commented Nov 29, 2022

Ok since GitHub will not show comments for whatever stupid reason I'll just paste it here. This is for lines 12 to 19.

@HUMORCE This part of the script will not work without administrator privileges. Should I make this optional instead?

@HUMORCE
Copy link
Member

HUMORCE commented Nov 29, 2022

since GitHub will not show comments for whatever stupid reason.

you need to submit review after start a review.

make this optional

yes

@FlawlessCasual17
Copy link
Contributor Author

/verify

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

All changes look good.

Wait for review from human collaborators.

mp3tag

  • Description
  • License
  • Hashes
  • Checkver
  • Autoupdate

@HUMORCE
Copy link
Member

HUMORCE commented Dec 5, 2022

exists restrict of installation

applies to uninstallation also.

for all operations that require admin privileges: use notes or warn() | info() instead, let users execute them manully.

e.g. some items in scoop-checkup need to be executed manually by the user rather than requesting admin privileges to execute when installing Scoop.

Scoop installs programs from the command line with a minimal amount of friction.

src: https://github.com/ScoopInstaller/Scoop#what-does-scoop-do

@FlawlessCasual17
Copy link
Contributor Author

@HUMORCE I fixed the pre_uninstall script, now it will only ask for administrator privileges if the DLL file is registered.

@HUMORCE HUMORCE merged commit 3a5dcdb into ScoopInstaller:master Dec 8, 2022
@redactedscribe
Copy link
Contributor

@Zliced13 Thanks for the enhancements, but for what reason was persist limited to only a select few files? The manifest was correct to back up all of /data/ and /export/, as well as /mp3tag.cfg. These two directories and /mp3tag.cfg is what Mp3tag exports as a backup when you use the File menu "Save configuration" feature. Limiting persist to only a few files within /data/ misses some user-customised files and the same goes for omitting all of /export/.

There is also /Mp3tagSettings.zip which is automatically created after each termination of the application. If there's a good reason behind limiting persist as you have, at minimum /Mp3tagSettings.zip should also be persisted because this contains all of the user's settings (the three items I've mentioned).

Would it be possible that you fix this?

@FlawlessCasual17
Copy link
Contributor Author

@Zliced13 Thanks for the enhancements, but for what reason was persist limited to only a select few files? The manifest was correct to back up all of /data/ and /export/, as well as /mp3tag.cfg. These two directories and /mp3tag.cfg is what Mp3tag exports as a backup when you use the File menu "Save configuration" feature. Limiting persist to only a few files within /data/ misses some user-customised files and the same goes for omitting all of /export/.

There is also /Mp3tagSettings.zip which is automatically created after each termination of the application. If there's a good reason behind limiting persist as you have, at minimum /Mp3tagSettings.zip should also be persisted because this contains all of the user's settings (the three items I've mentioned).

Would it be possible that you fix this?

If you read my previous comments, you would know why I had to use pre_(un)install scripts to manually copy the files instead of the using the built in persist property.

@redactedscribe
Copy link
Contributor

I see now that you are actually copying the /export/ directory in the pre inst/uninstall lines, but I'm still confused about why you're only persisting a select number of files from within /data/ instead of all of them.

@FlawlessCasual17
Copy link
Contributor Author

FlawlessCasual17 commented Jan 30, 2023

Because not all the files in the /data/ directory need to be copied to the persist folder.

@redactedscribe
Copy link
Contributor

Because not all the files in the /data/ directory need to be copied to the persist folder.

This is the contents of /data/ for me:
mp3tag-data

  • If actions is not persisted, all user created/modified tagging actions will be lost on uninstall.
  • If sources is not persisted, all user created/modified source scripts for fetching metadata will be lost on uninstall.
  • If genres.ini is not persisted, all user-defined genres tags will be lost on uninstall.
  • If tools.ini is not persisted, all user-defined external program paths and parameters will be lost on uninstall.
  • empty.mte appears to be a template which I haven't modified, however others may have.
  • freedb.src may also be of importance to anyone who may have edited it.

My /data/ folder may not contain all the potential files/folders that Mp3tag can create as user data, I don't know. What I do know is that I'd not want to lose the files which you have chosen to mark as expendable via their omission from the manifest. Backing up /data/ as a whole would avoid losing any of this user data, much of which is important.

@FlawlessCasual17
Copy link
Contributor Author

Next PR, I will try to add that feature to mp3tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants