Create albums_statistics.py

[Echelon666]

Forum threads about this plugin:

[](https://community.metabrainz.org/t/new-plugin-statistics/717507)

[Sophist-UK]

1. This PR needs a description which links to the two forum discussions about this plugin.

2. See my previous review comments in the forums. However, even though I made the time to give it a really decent yet overall positive review, and despite the changes I requested being pretty simple to implement, I have no idea why the creator of this PR has chosen to submit the PR without making any changes I suggested, however...

Until my review comments are properly handled, my personal recommendation (for the little it is worth since I am not someone who takes decisions about whether this gets merged) is that the code in this plugin is of insufficient quality to be merged.

[Sophist-UK]

Forum discussion: https://community.metabrainz.org/t/new-plugin-statistics/717507

[Sophist-UK]

My review comments from the forum:

• Currently called Statistics - IMO Albums Statistics is a better name.
• “Counts the types of albums” would be better described as “Counts the quality or status of albums”. I also wonder whether an example might be useful in the description e.g. “Unchanged: 5, Modified: 2, Incomplete: 2, Errored: 1, Total: 10.”
• IMO icons should either be to the left of the descriptions or between the descriptions and the counts.
• I think there should be a description line before the stats that says something like “The status of the selected Albums is as follows:”.
• In the descriptions Album should IMO be either Albums or Album(s), or even omitted as it is implied by the Window Title “Albums Statistics”.
• I would personally prefer to see the descriptions as e.g. “Albums incomplete & unchanged / saved”, “Albums complete and changed / unsaved” etc.
• You have created the statwindow as a global variable and IMO it would be better being inside the main class as a class variable self.statwindow and initialised created in the class constructor.
• Class name should IMO be AlbumStats.
• Variable names needs a bit of work e.g. statwindow → stats_popup, A/B/C/D/E/TOT/text* need to have names that reflect their purpose. T does not need to be initialised because it is a calculated value, the others can be initialised in a single statement A = B = C = D = E = 0. The text* variables can probably be eliminated e.g. grid.addWidget(QLabel(str(A)), 0, 1)
• There is a LOT of repeated code for adding the widget - IMO better to create a small class method that does the several actions for the 3 widgets in each row (e.g. create_row_widgets, and then call this function once for each row with the relevant values passed as parameters.
• I do wonder whether there is a better style for the nested if statements.
• You are clearing widgets at the start and recreating them. It might be better to create them in the class constructor, saving a pointer to each of the text widgets for later use and simply change the values of the text widgets before you show the window.
• Are there any issues with this when it is displayed on either old low-resolution monitors or modern, expensive ultra-high resolution monitors?
• I have no idea how I18n (language translation) is handled for plugins, but all string constants should IMO be wrapped in a translation call e.g. _("string"). @phw Philipp: Can you give any indicator here on how people can add translations to a plugin? Is it possible in Picard v2? Will it be better / easier in Picard v3

[Sophist-UK]

In the forum, @Echelon666 said he wasn't sure how to implement my review comments, so here is my untested version:

PLUGIN_NAME = "Albums Statistics"
PLUGIN_AUTHOR = "Echelon"
PLUGIN_DESCRIPTION = "Summarises the status of selected albums e.g. Changed?, Complete? Error?"
PLUGIN_VERSION = '0.1'
PLUGIN_API_VERSIONS = ['2.2']
PLUGIN_LICENSE = "GPL-2.0-or-later"
PLUGIN_LICENSE_URL = "https://www.gnu.org/licenses/gpl-2.0.html"

from PyQt5 import QtGui
from PyQt5.QtWidgets import QLabel, QGridLayout, QWidget
from PyQt5.QtGui import QPixmap, QIcon

from picard.ui.itemviews import BaseAction, register_album_action

class AlbumsStats(BaseAction):
    NAME = "Albums Statistics"

    def __init__(self):
        # Create grid hidden
        self.grid = QGridLayout()
        self.grid.addWidget(QLabel(_("The status of the selected Albums is as follows:")), 0, 0, 1, 3)

        self.addGridRow(1, ":/images/22x22/media-optical.png",
            _("Incomplete & unchanged"))
        self.addGridRow(2, ":/images/22x22/media-optical-modified.png",
            _("Incomplete & modified"))
        self.addGridRow(3, ":/images/22x22/media-optical-saved.png",
            _("Complete & unchanged"))
        self.addGridRow(4, ":/images/22x22/media-optical-saved-modified.png",
            _("Complete & modified"))
        self.addGridRow(5, ":/images/22x22/media-optical-error.png",
            _("Errored"))
        self.addGridRow(6, "",
            _("Total"))

        self.grid.addWidget(QLabel("Total"), 6, 2)

        self.window = QWidget()
        self.window.setLayout(self.grid)
        self.window.setGeometry(100, 100, 400, 200)
        self.window.setWindowTitle(_("Albums Statistics"))
        self.window.setWindowIcon(QIcon(":/images/16x16/org.musicbrainz.Picard.png"))
        self.window.setStyleSheet("font-size:12pt;")

    def addGridRow(self, row, icon_location, description):
        icon = QLabel()
        if icon_location:
            icon.setPixmap(QPixmap(icon_location))
        self.grid.addWidget(icon, row, 0)

        self.grid.addWidget(QLabel(""), row, 1)

        self.grid.addWidget(QLabel(description), row, 2)

    def setCounter(self, row, count):
        counter = self.grid.itemAtPosition(row, 1)
        counter.setText(str(count))

    def callback(self, objs):
        incomplete_unchanged = incomplete_modified = complete_unchanged = complete_modified = errored = 0

        for album in objs:
            if album.errors:
                errored += 1
            elif album.is_complete():
                if album.is_modified():
                    complete_modified += 1
                else:
                    complete_unchanged += 1
            else:
                if album.is_modified():
                    incomplete_modified += 1
                else:
                    incomplete_unchanged += 1

        total = incomplete_unchanged + incomplete_modified + complete_unchanged + complete_modified + errored

        self.setCounter(1, incomplete_unchanged)
        self.setCounter(2, incomplete_modified)
        self.setCounter(3, complete_unchanged)
        self.setCounter(4, complete_modified)
        self.setCounter(5, errored)
        self.setCounter(6, total)

        self.window.show()

register_album_action(AlbumsStats())

[Echelon666]

“I have no idea why the creator of this PR has chosen to submit the PR without making any changes I suggested, however…”

ANY ???

I’ve already implemented 12 of your corrections the first time.

Only without “selfstatwindow” and clearing the results.

[Sophist-UK]

In the online forum @Echelon666 (as @Peter69) said:

@Sophist

Too many variables, classes, objects, calls for me.

It’s all getting mixed up.

I’d like to help myself and you, but I honestly say “I can’t do it”.

It’s a waste of your time and mine.

In essence you said that you weren’t able to implement my review comments because of your inexperience.

I wanted to help you and so I spent more of my own time showing you what my review comments were about in the hope that the code would be more readable and maintainable and so more likely to get approved and made generally available.

Then you said:

I’ve already implemented 12 of your corrections the first time.

I made some generally positive comments about your work, and made a total of 12 bullets in my review comments, and the bulk of these were not implemented, so it is difficult to see how you can realistically claim that you implemented all 12 of them - and indeed you stated yourself that you were unable to implement them. Your original code, my review comments, your revised code and my revised version are all available above for people to review for themselves and form their own judgements.

I was only trying to help get your vision approved - and now I just wish I hadn’t bothered to try to help.

[Sophist-UK]

@Echelon666

This is your PR and it is entirely up to you whether you stick with your poor quality code or improve it so that it can be recommended and approved. If you want to use my rewritten code as the basis for achieving this, or rewrite it yourself - entirely your choice.

But if you want to use my rewritten code, all you need to do to get my recommendation for approval for this PR is to take this code and test it to make sure that A) it fits what you are trying to achieve and B) it works.

Or rewrite it yourself - or worst case abandon this PR (in which case please close it).

(This comment was prompted by @Echelon666's new attempt today to promote a second plugin here.)

[Echelon666]

@Sophist-UK

I downloaded the code, made a ZIP, installed it.

I added a few directories to Picard. The results appeared in the right panel.

Then I select the albums, right-click, and Picard closes.

Here's the debug:

D: 22:12:42,961 tagger.__init__:315: Starting Picard from 'C:\\Program Files\\MusicBrainz Picard\\picard\\tagger.pyc' D: 22:12:42,961 tagger.__init__:316: Platform: Windows-10-10.0.22631-SP0 CPython 3.8.10 D: 22:12:42,961 tagger.__init__:318: Versions: Picard 2.12.3, Python 3.8.10, PyQt 5.15.10, Qt 5.15.2, Mutagen 1.47.0, Discid discid 1.2.0, libdiscid 0.6.4, astrcmp C, SSL OpenSSL 1.1.1b 26 Feb 2019 D: 22:12:42,961 tagger.__init__:319: Configuration file path: 'C:/Users/Piotr/AppData/Roaming/MusicBrainz/Picard.ini' D: 22:12:42,961 tagger.__init__:321: User directory: 'C:\\Users\\Piotr\\AppData\\Local\\MusicBrainz\\Picard' D: 22:12:42,961 tagger.__init__:322: System long path support: True D: 22:12:42,961 tagger.__init__:325: Qt Env.: QT_PLUGIN_PATH='C:\\Program Files\\MusicBrainz Picard\\PyQt5\\Qt5\\plugins' D: 22:12:42,961 i18n.setup_gettext:153: UI language: system D: 22:12:42,961 i18n._log_lang_env_vars:138: Env vars: D: 22:12:42,961 i18n.setup_gettext:161: Trying locales: ['pl_PL'] D: 22:12:42,977 i18n.setup_gettext:167: Set locale to: 'pl_PL' D: 22:12:42,977 i18n.setup_gettext:178: Using locale: 'pl_PL' D: 22:12:42,977 i18n._load_translation:125: Loading gettext translation for picard, localedir='C:\\Program Files\\MusicBrainz Picard\\locale', language='pl_PL' D: 22:12:42,977 i18n._load_translation:125: Loading gettext translation for picard-attributes, localedir='C:\\Program Files\\MusicBrainz Picard\\locale', language='pl_PL' D: 22:12:42,977 i18n._load_translation:125: Loading gettext translation for picard-constants, localedir='C:\\Program Files\\MusicBrainz Picard\\locale', language='pl_PL' D: 22:12:42,977 i18n._load_translation:125: Loading gettext translation for picard-countries, localedir='C:\\Program Files\\MusicBrainz Picard\\locale', language='pl_PL' D: 22:12:42,977 i18n.setup_gettext:201: _ = <bound method GNUTranslations.gettext of <gettext.GNUTranslations object at 0x000001FFAC4979D0>> D: 22:12:42,993 i18n.setup_gettext:202: N_ = <function <lambda> at 0x000001FFAA0D9EE0> D: 22:12:42,993 i18n.setup_gettext:203: ngettext = <bound method GNUTranslations.ngettext of <gettext.GNUTranslations object at 0x000001FFAC4979D0>> D: 22:12:42,993 i18n.setup_gettext:204: gettext_countries = <bound method GNUTranslations.gettext of <gettext.GNUTranslations object at 0x000001FFAC497AF0>> D: 22:12:42,993 i18n.setup_gettext:205: gettext_attributes = <bound method GNUTranslations.gettext of <gettext.GNUTranslations object at 0x000001FFAC4977C0>> D: 22:12:42,993 i18n.setup_gettext:206: pgettext_attributes = <bound method GNUTranslations.pgettext of <gettext.GNUTranslations object at 0x000001FFAC4977C0>> D: 22:12:42,993 webservice._network_accessible_changed:388: Network accessible requested: 1, actual: 1 D: 22:12:43,024 webservice.set_cache:410: NetworkDiskCache dir: 'C:/Users/Piotr/AppData/Local/MusicBrainz/Picard/cache/network/' current size: 90.0 MB max size: 100 MB D: 22:12:43,024 pluginmanager.load_plugins_from_directory:264: Looking for plugins in directory 'C:\\Users\\Piotr\\AppData\\Local\\MusicBrainz\\Picard\\plugins', 1 names found D: 22:12:43,024 plugin.register:82: ExtensionPoint: album_actions register <- plugin='stat' item=<picard.plugins.stat.AlbumsStats object at 0x000001FFAC519160> D: 22:12:43,024 pluginmanager._load_plugin:337: Loading plugin 'Albums Statistics' version 0.1.0.final0, compatible with API: 2.2 I: 22:12:43,024 pluginmanager.load_plugins_from_directory:252: Plugin directory 'C:\\Program Files\\MusicBrainz Picard\\plugins' doesn't exist D: 22:12:43,024 ui/playertoolbar.__init__:91: Internal player: QtMultimedia available, initializing QMediaPlayer D: 22:12:43,052 ui/playertoolbar.__init__:98: Internal player: available, QMediaPlayer set up D: 22:12:43,316 tagger.main:1576: Looking for Qt locale pl_PL in C:/Program Files/MusicBrainz Picard/PyQt5/Qt5/translations I: 22:12:43,320 browser/browser.start:121: Starting the browser integration (127.0.0.1:8000) D: 22:12:43,363 config.event:261: Config file update requested on thread 3032 D: 22:12:45,225 ui/mainwindow.auto_update_check:1786: Skipping startup check for program updates. Today: 2024-11-06, Last check: 2024-11-04 (Check interval: 7 days), Update level: 0 (stable) D: 22:12:45,225 config.event:261: Config file update requested on thread 3032 D: 22:13:19,726 config.event:261: Config file update requested on thread 3032

[outsidecontext]

Can someone remove me from this thread please? I've been added in error and it has nothing to do with me. Thanks.

On Thu, 7 Nov 2024, 10:22 Echelon666, @.***> wrote:

@.**** commented on this pull request.

In plugins/albums_statistics/albums_statistics.py https://github.com/metabrainz/picard-plugins/pull/384#discussion_r1832428069 :

+from picard.ui.itemviews import BaseAction, register_album_action

+statwindow = QWidget() +grid = QGridLayout() + +statwindow.setLayout(grid) +statwindow.setGeometry(100, 100, 400, 200) +statwindow.setWindowTitle("Albums Statistics") +statwindow.setWindowIcon(QIcon(":/images/16x16/org.musicbrainz.Picard.png")) +statwindow.setStyleSheet("font-size:12pt;") + +class AlbumStats(BaseAction):

• NAME = "Statistics"
• def callback(self, objs):

•    A = B = C = D = E = 0
  

But now we are testing the @Sophist-UK https://github.com/Sophist-UK code

— Reply to this email directly, view it on GitHub https://github.com/metabrainz/picard-plugins/pull/384#discussion_r1832428069, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHQBLF7FPZAK6TXS3SAN3TZ7M5QHAVCNFSM6AAAAABPOVNVF2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRQGU2DGMZZGA . You are receiving this because you were mentioned.Message ID: @.***>

[phw]

@outsidecontext You got mentioned above by mistake, which makes Github to automatically subscribe you. But you should be able to unsubscribe with the "Unsubscribe" button on the right side bar.

I don't think it is possible for other users to unsubscribe someone else from a thread.

[Sophist-UK]

@outsidecontext Ooops - sorry - it was me who mentioned you instead of @phw. Apologies.

[Sophist-UK]

@Sophist-UK

I downloaded the code, made a ZIP, installed it.

I added a few directories to Picard. The results appeared in the right panel.

Then I select the albums, right-click, and Picard closes.

Here's the debug: ...

The debug tells us nothing. (Picard shouldn't crash though regardless - it should instead complain about you having zipped it up.)

I am unclear why you zipped it rather than just replacing the code in your existing .py file. So my advice, delete the zip file and put my code in the .py file, and then push it as a commit to Github.

(I have a feeling that zip files are just for downloaded plugins, not for locally developed ones. Also, there are multiple internal formats for zips - so it is also possible that you used an incompatible one.)

AND PLEASE STOP ASKING FOR MY HELP BY NAME!!! I have tried to help you before and only got abuse as a response, and I have made it clear that as a consequence I am not prepared to provide you any further detailed help. If you want further help from me then you need to apologise for your past behaviour and promise to start to listen to and act upon the advice I give (or provide reasoned responses why you think I am wrong).

[Echelon666]

Overwrite new code here or create a new PR from scratch?

[Sophist-UK]

Overwrite the code here - who wants to start this whole painful process over from scratch with a new PR???!!!!!!

[Echelon666]

I don't see the albums_statistics plugin in my Picard clone on my drive.

[Sophist-UK]

Insufficient information to help. Additionally, we cannot continue to hand-hold you for every little thing. You need to be more self-sufficient and work things out for yourself.

[Echelon666]

Miracle, it appeared. ;)

[Sophist-UK]

Yes it did. So now @phw has to spend more of his time re-reviewing my version of the code. And I haven't yet seen a single apology from you e.g. for having wasted his time.

[phw]

• I have no idea how I18n (language translation) is handled for plugins, but all string constants should IMO be wrapped in a translation call e.g. _("string"). @phw Philipp: Can you give any indicator here on how people can add translations to a plugin? Is it possible in Picard v2? Will it be better / easier in Picard v3

@Sophist-UK Translations won't work for plugins, or at least there is no defined way on how to do this. For the new plugin system I'd like to provide a translation function that is usable by plugins (essentially plugins will be able to provide their own translation domain and ship their own translation files). Not yet sure whether this will be part of the initial version or we will provide this later. But the plan is that the plugin API will provide a translation function the plugin can use (and which will work transparently).

[Sophist-UK]

Thanks Philipp. Heartening to know.

[Echelon666]

I didn't succeed with Github Desktop, so I'll post the code here.

I added the line "super().init()"

and I detected that setCounter causes Picard to crash.

`PLUGIN_NAME = "Albums Statistics" PLUGIN_AUTHOR = "Echelon" PLUGIN_DESCRIPTION = "Summarises the status of selected albums e.g. Changed?, Complete? Error?" PLUGIN_VERSION = '0.1' PLUGIN_API_VERSIONS = ['2.2'] PLUGIN_LICENSE = "GPL-2.0-or-later" PLUGIN_LICENSE_URL = "https://www.gnu.org/licenses/gpl-2.0.html"

from PyQt5 import QtGui from PyQt5.QtWidgets import QLabel, QGridLayout, QWidget from PyQt5.QtGui import QPixmap, QIcon

from picard.ui.itemviews import BaseAction, register_album_action

class AlbumsStats(BaseAction): NAME = "Albums Statistics"

def __init__(self):
    super().__init__()

    # Create grid hidden
    self.grid = QGridLayout()
    self.grid.addWidget(QLabel(_("The status of the selected Albums is as follows:")), 0, 0, 1, 3)

    self.addGridRow(1, ":/images/22x22/media-optical.png",
        _("Incomplete & unchanged"))
    self.addGridRow(2, ":/images/22x22/media-optical-modified.png",
        _("Incomplete & modified"))
    self.addGridRow(3, ":/images/22x22/media-optical-saved.png",
        _("Complete & unchanged"))
    self.addGridRow(4, ":/images/22x22/media-optical-saved-modified.png",
        _("Complete & modified"))
    self.addGridRow(5, ":/images/22x22/media-optical-error.png",
        _("Errored"))
    self.addGridRow(6, "",
        _("Total"))

    self.grid.addWidget(QLabel("Total"), 6, 2)

    self.window = QWidget()
    self.window.setLayout(self.grid)
    self.window.setGeometry(100, 100, 400, 200)
    self.window.setWindowTitle(_("Albums Statistics"))
    self.window.setWindowIcon(QIcon(":/images/16x16/org.musicbrainz.Picard.png"))
    self.window.setStyleSheet("font-size:12pt;")

def addGridRow(self, row, icon_location, description):
    icon = QLabel()
    if icon_location:
        icon.setPixmap(QPixmap(icon_location))
    self.grid.addWidget(icon, row, 0)

    self.grid.addWidget(QLabel(""), row, 1)

    self.grid.addWidget(QLabel(description), row, 2)

def setCounter(self, row, count):
    counter = self.grid.itemAtPosition(row, 1)
    counter.setText(str(count))

def callback(self, objs):
    incomplete_unchanged = incomplete_modified = complete_unchanged = complete_modified = errored = 0

    for album in objs:
        if album.errors:
            errored += 1
        elif album.is_complete():
            if album.is_modified():
                complete_modified += 1
            else:
                complete_unchanged += 1
        else:
            if album.is_modified():
                incomplete_modified += 1
            else:
                incomplete_unchanged += 1

    total = incomplete_unchanged + incomplete_modified + complete_unchanged + complete_modified + errored
    '''
    self.setCounter(1, incomplete_unchanged)
    self.setCounter(2, incomplete_modified)
    self.setCounter(3, complete_unchanged)
    self.setCounter(4, complete_modified)
    self.setCounter(5, errored)
    self.setCounter(6, total)
    '''
    self.window.show()

register_album_action(AlbumsStats())`

[Sophist-UK]

The addition of super().__init__() was a good call IMO - I missed it from the code and since BaseAction has an __init__ method, this line is indeed needed.

I now think I know why the code as it stands is failing - but given OP's previous attitude and his most recent post in the forums "You too. In short U2." (where the f-word appears to be silent but implied), I am not inclined to help him any further.

If anyone else wants to help him that is of course your choice, but based on his response to my giving him extensive help and then being insulted you may equally wish to pass on this "opportunity".

[Echelon666]

@phw

Could you please take a look at the current code?

[phw]

I'll see that I can run and review this in the next days

[Echelon666]

Before using the right panel "Select all", you need to collapse all albums "Collapse all".

If a single track is visible and selected somewhere, Picard will crash and turn off.

This is probably because the plugin counts albums, not songs.

[yoobi]

Any chances to be merged ?

[Sophist-UK]

@yoobi This PR is awaiting the author responding to the review comments by myself and others.

Since this code was in part (or largely) based on code I wrote as an example (and is therefore my copyright), as per my comments above I have asked for credit to be given, and until that change is made (amongst all the other changes requested) I will object to this PR being merged.

[yoobi]

As I saw the author made a request for review I thought it was done, having credits for it make sense.

Also to note I have copied the python code and installed it locally on my Picard Version 2.12 macOS arm m1 and it crashed I have around 400 albums