Create albums_statistics.py #384
Conversation
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. |
|
Forum discussion: https://community.metabrainz.org/t/new-plugin-statistics/717507 |
|
My review comments from the forum:
|
|
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()) |
|
“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. |
|
In the online forum @Echelon666 (as @Peter69) said:
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 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. |
|
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.) |
|
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:
|
There was a problem hiding this comment.
I'm really not quite sure how useful this is as it currently. The display of some "A", "B", "C" values does not seem to be self explanatory. I think the UI in general would need some rework. Also the way a single grid gets created and updated every time the dialog is shown seems a bit error prone. It would be better to have some dialog class that gets instantiated.
The plugin is fine as a private plugin for personal use, but for inclusion in the plugin list it needs extra work.
| from picard.ui.itemviews import BaseAction, register_album_action | ||
|
|
||
| statwindow = QWidget() | ||
| grid = QGridLayout() |
There was a problem hiding this comment.
I think it would be better to put this into it's own class. Something like this:
class AlbumStatsDialog(QWidget):
def __init__(self, albums):
grid = QGridLayout()
self.setLayout(grid)
self.setGeometry(100, 100, 400, 200)
self.setWindowTitle("Albums Statistics")
# The full code to setup the UI goes hereThen this can be used in the callback function:
window = AlbumStatsDialog(objs)
window.show()
There was a problem hiding this comment.
@phw Philipp See my previous rewrite of this code that I made to try to help Peter - I put all the code inside the class, but I suspect that the above comment is still a useful enhancement to my version..
| statwindow.setGeometry(100, 100, 400, 200) | ||
| statwindow.setWindowTitle("Albums Statistics") | ||
| statwindow.setWindowIcon(QIcon(":/images/16x16/org.musicbrainz.Picard.png")) | ||
| statwindow.setStyleSheet("font-size:12pt;") |
There was a problem hiding this comment.
We usually avoid hard coding font sizes so Qt uses system defaults.
| NAME = "Statistics" | ||
|
|
||
| def callback(self, objs): | ||
| A = B = C = D = E = 0 |
There was a problem hiding this comment.
Those variables should have proper names
There was a problem hiding this comment.
But now we are testing the @Sophist-UK code
There was a problem hiding this comment.
No - this PR still has your code in it (regardless of what code you are personally working on) - and it the code you submitted (rather than what I posted in a comment) that @phw Philipp has just taken the time and effort to review.
As for my code, you are still welcome to use it even though I think you have made it abundantly clear in the forums that you do not appreciate my help. But if you do then:
- You need to push a commit to your PR with my code in it; and
- You needed to make it clear that you were doing so earlier so that @phw didn't waste his time reviewing your older and now obsolete code. IMO Philipp deserves an abject apology from you for wasting his time.
There was a problem hiding this comment.
Create a new PR from scratch?
How to name the plugin and branch?
There was a problem hiding this comment.
Create a new PR from scratch?
No
Overwrite new code here?
Yes
Which bit about "You need to push a commit to your PR with my code in it" was unclear? Did I mention creating a new PR?
|
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
<#384 (comment)>
:
> +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
<#384 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHQBLF7FPZAK6TXS3SAN3TZ7M5QHAVCNFSM6AAAAABPOVNVF2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRQGU2DGMZZGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@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. |
|
@outsidecontext Ooops - sorry - it was me who mentioned you instead of @phw. Apologies. |
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). |
|
Overwrite new code here or create a new PR from scratch? |
|
Overwrite the code here - who wants to start this whole painful process over from scratch with a new PR???!!!!!! |
|
I don't see the albums_statistics plugin in my Picard clone on my drive. |
|
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. |
|
Miracle, it appeared. ;) |
|
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. |
@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). |
|
Thanks Philipp. Heartening to know. |
Forum threads about this plugin:
[](https://community.metabrainz.org/t/new-plugin-statistics/717507)