New plugin "Statistics"

MusicBrainz Picard
3

Well done on making this work.

  1. You need to log into Github and clone the Picard-Plugins repo, and in your cloned copy…
  2. Add a branch called album_statistics, then add a file called /plugins/album_statistics/album_statistics.py, then copy and paste your code into this and save / commit the file.
  3. Github should then offer you the option of creating a PR and you click the button to do so.

When you submit this as a PR, you will get some review comments, and to help you along, here are mine. Please don’t consider the quantity of these review comments to be a negative response - this seems like a useful plugin and you have successfully written it and made it work.

  1. Currently called Statistics - IMO Albums Statistics is a better name.
  2. “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.”
  3. IMO icons should either be to the left of the descriptions or between the descriptions and the counts.
  4. I think there should be a description line before the stats that says something like “The status of the selected Albums is as follows:”.
  5. 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”.
  6. I would personally prefer to see the descriptions as e.g. “Albums incomplete & unchanged / saved”, “Albums complete and changed / unsaved” etc.
  7. 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.
  8. Class name should IMO be AlbumStats.
  9. 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)
  10. 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.
  11. I do wonder whether there is a better style for the nested if statements.
  12. 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.
  13. Are there any issues with this when it is displayed on either old low-resolution monitors or modern, expensive ultra-high resolution monitors?
  14. 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"). @outsidecontext 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

Sorry to give so many comments; they may perhaps look daunting but are quite minor and really quite easy to change.

1 Like