Create albums_statistics.py

Echelon666 commented 1 month ago

Forum threads about this plugin:

Sophist-UK commented 1 month ago

This PR needs a description which links to the two forum discussions about this plugin.
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 commented 1 month ago

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

Sophist-UK commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

“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 commented 1 month ago

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 commented 2 days ago

@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 commented 1 day ago

@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 commented 1 day ago

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 commented 1 day ago

@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 commented 1 day ago

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

Sophist-UK commented 1 day ago

@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 commented 1 day ago

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

Sophist-UK commented 1 day ago

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

Echelon666 commented 1 day ago

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

Sophist-UK commented 1 day ago

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 commented 1 day ago

Miracle, it appeared. ;)

Sophist-UK commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

Thanks Philipp. Heartening to know.

metabrainz / picard-plugins

Create albums_statistics.py #384

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