PICARD-1685 : Add ~filesize variable

[nullHawk]

Summary

Add ~filesize variable

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Problem

Provide the file size in bytes as a hidden file variable ~filesize (similar to the existing file variables like e.g. ~bitrate and ~length).

• JIRA ticket (optional): PICARD-1685

Solution

I have added the "~filename" variable which stores the size of the file in bytes using "os.path.getsize" and passing the "self.filename" in it. also I have used this variable in infodialog.py to show Size. Modified test cases for '~filsize'. Add '~filesize' to metadata in wav.py since it doesn't uses mutagen.

Action

I don't know if you want to also add size option in itemviews.py (since it was mention in the old discussion "https://community.metabrainz.org/t/current-file-size/453338/5"). If yes, then please specifiy how to add it in UI, sicne the size is in bytes and in itemviews.py values are directly added from the list ("columns").

[phw]

This looks really good, thanks a lot for for looking over the code with so much detail. Just some minor comments.

If you feel up to it you could also update the documentation at https://picard-docs.musicbrainz.org/en/variables/variables_file.html to have the _filesize variable listed there. The docs are managed in https://github.com/metabrainz/picard-docs

[nullHawk]

I will soon add the rest of the requirements along with the documentations and code the formated according to the PEP 8 standards

[nullHawk]

Added checks to other formates for '~filesize', also added '~filsize' to metadata in wav.py

[nullHawk]

Also raised PR "metabrainz/picard-docs#218" for updating the _filesize variable as mentioned by @phw :)

[phw]

Thanks, this looks good. LGTM

[phw]

Not fully sure why https://github.com/metabrainz/picard/actions/runs/7540649415/job/20539127340?pr=2361 is failing. I first assumed it was because of mutagen==1.3.7, but testing with this mutagen version locally shows no errors.

One cause might be that the randomization of tests (with pytest-randomly) makes this issue happen only sometimes. Does some other test maybe write to the source file?

[nullHawk]

Not fully sure why https://github.com/metabrainz/picard/actions/runs/7540649415/job/20539127340?pr=2361 is failing. I first assumed it was because of mutagen==1.3.7, but testing with this mutagen version locally shows no errors.

One cause might be that the randomization of tests (with pytest-randomly) makes this issue happen only sometimes. Does some other test maybe write to the source file?

Yes, I have confirmed this. When I tested with mutagen version 1.47 and python 3.11, all the tests worked fine. However, upon revisiting the requirements file and installing the correct version of mutagen, that is mutagen 1.37 with python 3.8, that particular test case failed. So I modified the file size in the test case accordingly.

[phw]

However, upon revisiting the requirements file and installing the correct version of mutagen, that is mutagen 1.37 with python 3.8, that particular test case failed. So I modified the file size in the test case accordingly.

Now it fails in other cases :( Something is going on here, but I don't yet understand what. mutagen 1.37 is an old version without WAVE support, so we are in the alternative code paths

picard/test/formats/test_wav.py

Line 148 in 94250ff

class WAVTest(CommonTests.SimpleFormatsTestCase):

and

picard/picard/formats/wav.py

Line 226 in 94250ff

class WAVFile(File):

. But it only seems to happen in the combination of both Python 3.8 and old mutagen. Reading the file size however is not involving mutagen.

I don't see what the issue is right now. Maybe some Python 3.8 bug?

But I'd be happy with just disabling the filesize check for WAVE for now and merge this change.

@zas any ideas?

[nullHawk]

I carefully checked the failed tests and found the following issues:

1. mutagen 1.37 is not compatible with Python 3.11. When attempting to install mutagen 1.37 with python 3.11 as specified in requirements.txt, it automatically installs mutagen 1.47 instead of mutagen 1.37
2. mutagen 1.37 is compatible with python 3.8
3. and we have 2 cases now

• mutagen 1.37 with python 3.8 expects the filesize to be 14652
• while mutagen 1.47 with python 3.11 expects the filesize to be 15706.

I am not sure why it's behaving like this.

[nullHawk]

I don't see what the issue is right now. Maybe some Python 3.8 bug?

As python 3.8 support is set to end in 2024 and the current version is 3.12, are we planning to migrate from python 3.8 to a higher version? If so, I am happy to contribute to the migration process.

[phw]

As python 3.8 support is set to end in 2024 and the current version is 3.12, are we planning to migrate from python 3.8 to a higher version? If so, I am happy to contribute to the migration process.

We'll drop 3.8 out of the CI builds and set 3.9 as min. supported version some time shortly after it has become EOL. No big migration really as there is full compatibility with newer versions.

The only thing that will be impacted is drop for Windows 7 support, but we have this with the Qt6 migration anyway. That's why the Windows builds in the master branch do build with Python 3.12 already.

[phw]

@nullHawk I found the issue, that was tricky. The issue was that the test_info test case was saving the data before performing the test, and hence as a side effect changing the actual test file. This resulted in different file size than the actual test files had. I only noticed because I compared file size on disk to file size in test.

See 0332f29#diff-7d25cea7df3c99710f9ba35f5231579dcd76cebc8effb6fb0ea01dfa531e2b57L238

I took the freedom to commit this change directly to the PR in 0332f29

[phw]

Now only some style issues remain. Can you check this locally? We use isort to organize the import statements, see https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md

[nullHawk]

@nullHawk I found the issue, that was tricky. The issue was that the test_info test case was saving the data before performing the test, and hence as a side effect changing the actual test file. This resulted in different file size than the actual test files had. I only noticed because I compared file size on disk to file size in test.

See 0332f29#diff-7d25cea7df3c99710f9ba35f5231579dcd76cebc8effb6fb0ea01dfa531e2b57L238

I took the freedom to commit this change directly to the PR in 0332f29

Oh I see now...Thanks a lot!

[nullHawk]

Now only some style issues remain. Can you check this locally? We use isort to organize the import statements, see https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md

Sure :)

[nullHawk]

Now only some style issues remain. Can you check this locally? We use isort to organize the import statements, see https://github.com/metabrainz/picard/blob/master/CONTRIBUTING.md

As a first-time contributor, I forgot to set up the pre-commit, which is why I couldn't catch the import error earlier while committing my changes. I believe that if it had been configured beforehand as mentioned here, it would have been easier for me to just install the pre-commit and use it directly, rather than creating the file .git/hooks/pre-commit and copying the codes as mentioned in the contributing file.

If you're considering making the readme more beginner-friendly, I'd be happy to submit a pull request for that. I can also list and add all the details I missed to ensure that any other first-time contributor won't overlook the same things.

[zas]

LGTM