desbma / r128gain

Fast audio loudness scanner & tagger (ReplayGain v2 / R128)
GNU Lesser General Public License v2.1
171 stars 9 forks source link

Added options to preserve modification times when tagging files. #8

Closed m-reiter closed 5 years ago

m-reiter commented 5 years ago

See #7

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-2.1%) to 75.381% when pulling 89d3bfeec983a6666071dd3b23b99ee32e783564 on m-reiter:master into 2864814c6d9881b04a04cc794f54939d3a93b762 on desbma:master.

desbma commented 5 years ago

Thanks.

I don't understand the need to restore the original mtime, but add an offset to it. Could you explain a use case for this?

I left a review in the code for other (minor) stuff.

m-reiter commented 5 years ago

I don't understand the need to restore the original mtime, but add an offset to it. Could you explain a use case for this?

That's an idea I didn't come up with myself but stumbled across online while researching tools to add gain information. I see two potential use cases:

Unfortunately, I can no longer find where I saw the first one. The second one was mentioned in the Squeezebox community forum

desbma commented 5 years ago
  • If you keep your music collection in more than one place and sync it from the master copy, some syncing tools might not pick up the file changes if the timestamp is preserved exactly.

  • Adding some seconds makes e.g. Logitech Media Server pick up the changes in a "changed or new" scan.

Both can be solved simply by doing nothing, and letting the modified time be updated (the current behavior). Or am I missing something?

m-reiter commented 5 years ago
  • If you keep your music collection in more than one place and sync it from the master copy, some syncing tools might not pick up the file changes if the timestamp is preserved exactly.
  • Adding some seconds makes e.g. Logitech Media Server pick up the changes in a "changed or new" scan.

Both can be solved simply by doing nothing, and letting the modified time be updated (the current behavior). Or am I missing something?

Yes: Logitech Media Server (that's what I'm using, other software might behave similarly) treats the file's modification time as the time the music was added to the library. So if I just update all mtimes to the time of tagging, that information gets lost. As a result, when I'm browsing newly added music in my library, old stuff will show up instead. That's why I wanted the option in the first place. I admit that adding some seconds is a clutch, and probably one I can live without, but seeing that others see the need to do just that, I thought I'd add it in as well since it's basically no effort and I don't see it doing any harm. Whoever doesn't need it will just not use it.

desbma commented 5 years ago

OK, I see the use now, and I agree with you there is not real drawback to implement that.

However I think the option should simply have two possible choices "preverse file mtime" or "preserve file mtime + add 1 second". There is no real need to let the user add a custom number of seconds.

The best way to do that is probably to use choices=(0,1), nargs="?", default=None, const=0, for the argparse setup.

That way, the argument value is interpreted as such:

m-reiter commented 5 years ago

I like your original suggestion without fixed choices better for several reasons:

I've pushed a version with choices commented out now. If you don't agree that the help output looks odd, feel free to comment it back in. I'm happy with both versions even if I'm slightly leaning towards the free form one.

desbma commented 5 years ago

a numeric parameter with just one valid option (besides the default) just feels odd to me ;)

Ideally we should just add the smallest amount of time so that the file is detected as changed by the tools you mentioned, but that is not portable across filesystems and OSs, adding a full second is safer.

Another alternative for the option is to have something a bit more verbose like -p original (equivalent to -p) and -p offset which adds 1, but I'm not sure this is better (subjective I guess).

desbma commented 5 years ago

I have merged this with a few other minor cosmetic changes.

Thanks for the contribution.

m-reiter commented 5 years ago

Thank you for the interesting discussion!