PICARD-1325: Allow disabling new version update checking for packagers #1024

virusMac · 2018-11-02T11:52:42Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other

When user uses --disable-autoupdate flag when installing or building Picard, it automatically hide "Update checking" feature from user and disable update checking code itself

Problem

Recently the ability to automatically and manually check for program updates was added to MusicBrainz Picard. Picard release packagers should be able to completely hide this functionality and configuration settings from the user for the packages they produce.

JIRA ticket: PICARD-1325

Solution

added if statements to determine showing all update-checking tabs and update-checking itself
added description to flag --disable-autoupdate in picard_install and picard_build (setup.py)
changed variable name from _autoupdate to autoupdate_enabled (tagger.py)

rdswift · 2018-11-02T15:29:42Z

picard/tagger.py

@@ -146,7 +146,7 @@ def __init__(self, picard_args, unparsed_args, localedir, autoupdate):
        )

        self._cmdline_files = picard_args.FILE
-        self._autoupdate = autoupdate
+        self.autoupdate_enabled = True


Is this True setting left over from your testing? Should it be self.autoupdate_enabled = autoupdate instead?

Yeah, it was only for testing. Thank you.

rdswift · 2018-11-02T15:30:12Z

picard/tagger.py

@@ -251,7 +251,8 @@ def __init__(self, picard_args, unparsed_args, localedir, autoupdate):
        self.stopping = False

        # Load release version information
-        self.updatecheckmanager = UpdateCheckManager(parent=self.window)
+        if QtWidgets.QApplication.instance().autoupdate_enabled:


Can't you simply use the self.autoupdate_enabled object here rather than the instance() object?

rdswift · 2018-11-02T15:30:21Z

picard/ui/mainwindow.py

@@ -189,7 +189,8 @@ def keyPressEvent(self, event):
    def show(self):
        self.restoreWindowState()
        super().show()
-        self.auto_update_check()
+        if QtWidgets.QApplication.instance().autoupdate_enabled:


I believe that the value is exposed as self.tagger.autoupdate_enabled so the instance() object shouldn't be required. Same throughout the file.

Thanks, I didn't notice that.

rdswift · 2018-11-02T15:30:32Z

picard/ui/options/general.py

@@ -17,8 +17,7 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.

-from PyQt5 import QtCore
-from PyQt5.QtWidgets import QInputDialog
+from PyQt5 import QtCore,QtWidgets


If you use self.tagger.autoupdate_enabled this likely isn't required. If it is required, there should be a space after the comma.

rdswift · 2018-11-02T15:30:44Z

picard/ui/options/general.py

-            self.ui.update_level.addItem(_(description['title']), level)
-        self.ui.update_level.setCurrentIndex(self.ui.update_level.findData(config.setting["update_level"]))
-        self.ui.update_check_days.setValue(config.setting["update_check_days"])
+        if QtWidgets.QApplication.instance().autoupdate_enabled:


I believe that the value is exposed as self.tagger.autoupdate_enabled so the instance() object shouldn't be required. Same throughout the file.

rdswift · 2018-11-02T15:35:38Z

picard/ui/ui_options_general.py

-        self.verticalLayout_2.addLayout(self.gridLayout_2)
-        self.vboxlayout.addWidget(self.groupBox_3)
+
+        if QtWidgets.QApplication.instance().autoupdate_enabled:


This file is automatically generated by python setup.py build_ui command and should not be edited manually. I suggest that you simply hide the elements associated with the update checking settings when the ui is displayed in picard/ui/options/general.py.

Is it ok to add that hiding feature to load() function?
I used setEnabled() - please tell me if it's better to hide it completely

Yes, I would add it to the load() method. I think it would be better to hide it completely, that way users of a packaged version don't see something that they can't change. It's less likely to generate questions that way. ;-)

rdswift · 2018-11-02T15:36:02Z

picard/ui/ui_options_general.py

-        self.vboxlayout.addWidget(self.groupBox_3)
+
+        if QtWidgets.QApplication.instance().autoupdate_enabled:
+            self.groupBox_3 = QtWidgets.QGroupBox(GeneralOptionsPage)


Since we're likely to be referring to this to hide it, the name should probably be changed from groupbox_3 to something more appropriate (e.g. update_check_groupbox).

rdswift · 2018-11-02T18:03:29Z

picard/ui/options/general.py

@@ -18,7 +18,6 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.

 from PyQt5 import QtCore
-from PyQt5.QtWidgets import QInputDialog


QImportDialog still needs to be imported because it is used in the login(self) method.

Sorry, misclick :)

I'm actually surprised that wasn't caught with the automatic checks. Then again, I would have expected codacy to have caught it and it seems the codacy checking didn't run.

phw · 2018-11-04T10:08:20Z

setup.py

@@ -181,7 +181,7 @@ class picard_build(build):
    user_options = build.user_options + [
        ('build-locales=', 'd', "build directory for locale files"),
        ('localedir=', None, ''),
-        ('disable-autoupdate', None, ''),
+        ('disable-autoupdate', None, 'disable update checking and hide settings for it'),


Well done adding a comment for this!

Thank you :)

zas · 2018-11-05T08:32:32Z

picard/ui/options/general.py

@@ -19,7 +19,6 @@

 from PyQt5 import QtCore
 from PyQt5.QtWidgets import QInputDialog
-


This whiteline should stay, can you just re-add it and squash again ?

Done, thanks :)

rdswift · 2018-11-05T19:26:34Z

@virusMac

Just so you know, the reason @zas was asking you to put the blank line back in is because the Picard developers are trying to standardize on sorting the import lines using the isort utility.

re metabrainz/picard#1024

rdswift reviewed Nov 2, 2018

View reviewed changes

virusMac force-pushed the PICARD-1325 branch from da466aa to ae79a8f Compare November 2, 2018 16:18

rdswift requested changes Nov 2, 2018

View reviewed changes

virusMac force-pushed the PICARD-1325 branch from 4ab8530 to 3d7ad23 Compare November 2, 2018 18:37

phw reviewed Nov 4, 2018

View reviewed changes

phw previously approved these changes Nov 4, 2018

View reviewed changes

zas requested changes Nov 5, 2018

View reviewed changes

virusMac dismissed phw’s stale review via c820f4c November 5, 2018 19:12

Allow disabling new version update checking

ff0df0a

virusMac force-pushed the PICARD-1325 branch from c820f4c to ff0df0a Compare November 5, 2018 19:14

zas approved these changes Nov 5, 2018

View reviewed changes

phw merged commit 985c56c into metabrainz:master Nov 5, 2018

phw mentioned this pull request Nov 13, 2018

Picard 2.1.0 metabrainz/picard-website#137

Merged

17 tasks

AndrewKvalheim mentioned this pull request Sep 6, 2023

picard: disable autoupdate NixOS/nixpkgs#253669

Merged

12 tasks

wegank pushed a commit to NixOS/nixpkgs that referenced this pull request Sep 7, 2023

picard: disable autoupdate

efb0b2f

re metabrainz/picard#1024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PICARD-1325: Allow disabling new version update checking for packagers #1024

PICARD-1325: Allow disabling new version update checking for packagers #1024

virusMac commented Nov 2, 2018

rdswift Nov 2, 2018

virusMac Nov 2, 2018

rdswift Nov 2, 2018

rdswift Nov 2, 2018

virusMac Nov 2, 2018

rdswift Nov 2, 2018

rdswift Nov 2, 2018

rdswift Nov 2, 2018

virusMac Nov 2, 2018

rdswift Nov 2, 2018

rdswift Nov 2, 2018

rdswift Nov 2, 2018

virusMac Nov 2, 2018 •

edited

Loading

rdswift Nov 2, 2018

phw Nov 4, 2018

virusMac Nov 4, 2018

zas Nov 5, 2018

virusMac Nov 5, 2018

rdswift commented Nov 5, 2018

		@@ -19,7 +19,6 @@

		from PyQt5 import QtCore
		from PyQt5.QtWidgets import QInputDialog

PICARD-1325: Allow disabling new version update checking for packagers #1024

PICARD-1325: Allow disabling new version update checking for packagers #1024

Conversation

virusMac commented Nov 2, 2018

Summary

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virusMac Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdswift commented Nov 5, 2018

virusMac Nov 2, 2018 •

edited

Loading