Change Alignement Operator -- Make conversion QPixmap --> QIcon explicit -- Use lambda to connect() to functions behind a wrapper

[Nowa-Ammerlaan]

Hi,

I had to make the following changes to get this to run with PyQt5.

Mostly the changes are trivial:

• Import IsPyQt5 and use it in if statements
• Make conversion of QPixmap to QIcon explicit where needed, for some reason PyQt5 doesn't do this automatically while PySide2 does.
• Use | operator instead of & in alignment, as I understand it this is not exactly the same, but PyQt5 complains about the & operator, and the result is visually identical as far as I can tell. So I think it should be fine to change this.

And then there were two issues I ran into that I sort of managed to create an ugly workaround for. There probably is a better solution than my ugly workaround, but I'll need your help and expertise to find it because I could not. You can find my workarounds on line 1397 in GuiConfiguration.py and line 456 in gui.py, along with a To-Do: comment.

Please let me know what you think about this, it would be great to have both PyQt5 and PySide2 compatibility in my opinion.

[Et0h]

Hi @AndrewAmmerlaan, it is great to see that you want to contribute to the Syncplay project.

Sorry to be the bearer of bad news, but I believe that adding PyQt support would not be in the interests of the Syncplay project at this time based on the principles set out at #315.

Back in 2013 we decided to use PySide rather than PyQt because the license (LGPL rather than GPL) allowed us to integrate it with our existing Apache 2.0 code.

Our experience of maintaining PySide support is that getting it right can be hard, especially given the number of operating systems which Syncplay supports and the fact that at any time a new version can come out which breaks things in a way which might either cause Syncplay to not run or some minor glitch. Furthermore, supporting both PySide1 and PySide2 adds to the complexity of the code which we need to maintain.

Licensing issues aside, expanding our official support beyond PySide to also support PyQt would further add to the complexity of the code and our maintenance burden. It would mean we were was supporting something which we wouldn't be using for our bundles and that the core Syncplay developers would not be using on a regular basis.

As far as I know, there are no circumstances where a user could not use PySide or PySide 2. As such, I do not see us going beyond support for PySide in the future (although if the circumstances were right we might move to just PySide 2 to allow us to simplify the code and make it easier to maintain).

[Nowa-Ammerlaan]

Hi @Et0h

I completely understand your reluctance to support yet another QtForPython implementation, especially if you do not use PyQt5 yourself.

As far as I know, there are no circumstances where a user could not use PySide or PySide 2. As such, I do not see us going beyond support for PySide in the future (although if the circumstances were right we might move to just PySide 2 to allow us to simplify the code and make it easier to maintain).

The circumstance where this occurs is when a distribution has packaged either PyQt5 or PySide2 but not both (and the package maintainer (e.g. me) for the Qt Python application (e.g. Syncplay) does not want to go through the trouble of packaging the missing QtForPython implementation (e.g. PySide(2)), and would rather just port the application in question to the implementation that is present in the distribution (e.g. PyQt5)). In addition this makes it unnecessary for end-users to install both PySide2 and PyQt5 if they use multiple Qt Python applications.

That being said, I do understand that you do not want to officially support another QtForPython implementation. However, I do think we can find the middle ground here. I have identified what exactly is different between PySide2 and PyQt5, and found three things which are relevant to the Syncplay code:

1. setAlignment() does not support the & operator, the | operator is used instead. The result is visually identical though as far as I can tell. Furthermore, all the tutorials and examples that I've seen also use the | operator.
2. setIcon() and similar functions only accept arguments of type QIcon, PySide2 makes the conversion from QPixmap automatically wheras PyQt5 does not. Making this conversion explicit by adding QtGui.QIcon(QtGui.Pixmap( instead of QtGui.QPixmap( does not change anything functionally, but it does make the code compatible with PyQt5.
3. When using connect() on a function with a wrapper such as QtCore.Slot or needClient lambda: needs to be used to ensure the arguments are parsed correctly. Again this doesn't change anything functionally but it does make the code compatible with PyQt5.

I have changed this PR to only include these changes I listed here, without referencing PyQt5. This way the code is compatible with PyQt5 but the application still only runs with PySide2. Thus, there still would be no need to officially support PyQt5.

Would you please consider merging these changes as they are now? Nothing is changed in the functionality at all, all that is done is make some conversions that are done anyway explicit and change the alignment operator which doesn't change anything visually.

And if you do ever change your mind about PyQt5, the code would already be compatible with it, and all that is need is this patch:

diff --git a/syncplay/ui/ConfigurationGetter.py b/syncplay/ui/ConfigurationGetter.py
index dd1d8ec..6d83c65 100755
--- a/syncplay/ui/ConfigurationGetter.py
+++ b/syncplay/ui/ConfigurationGetter.py
@@ -513,10 +513,10 @@ class ConfigurationGetter(object):
         self._overrideConfigWithArgs(args)
         if not self._config['noGui']:
             try:
-                from syncplay.vendor.Qt import QtWidgets, IsPySide, IsPySide2
+                from syncplay.vendor.Qt import QtWidgets, IsPySide, IsPySide2, IsPyQt5
                 from syncplay.vendor.Qt.QtCore import QCoreApplication
                 from syncplay.vendor import qt5reactor
-                if not (IsPySide2 or IsPySide):
+                if not (IsPySide2 or IsPySide or IsPyQt5):
                     raise ImportError
                 if QCoreApplication.instance() is None:
                     self.app = QtWidgets.QApplication(sys.argv)
diff --git a/syncplay/ui/GuiConfiguration.py b/syncplay/ui/GuiConfiguration.py
index 9ce6a42..1ad5bd7 100755
--- a/syncplay/ui/GuiConfiguration.py
+++ b/syncplay/ui/GuiConfiguration.py
@@ -11,7 +11,7 @@ from syncplay.players.playerFactory import PlayerFactory
 from syncplay.utils import isBSD, isLinux, isMacOS, isWindows
 from syncplay.utils import resourcespath, posixresourcespath
 
-from syncplay.vendor.Qt import QtCore, QtWidgets, QtGui, __binding__, IsPySide, IsPySide2
+from syncplay.vendor.Qt import QtCore, QtWidgets, QtGui, __binding__, IsPySide, IsPySide2, IsPyQt5
 from syncplay.vendor.Qt.QtCore import Qt, QSettings, QCoreApplication, QSize, QPoint, QUrl, QLine, QEventLoop, Signal
 from syncplay.vendor.Qt.QtWidgets import QApplication, QLineEdit, QLabel, QCheckBox, QButtonGroup, QRadioButton, QDoubleSpinBox, QPlainTextEdit
 from syncplay.vendor.Qt.QtGui import QCursor, QIcon, QImage, QDesktopServices
@@ -445,7 +445,7 @@ class ConfigDialog(QtWidgets.QDialog):
                 defaultdirectory = QDesktopServices.storageLocation(QDesktopServices.HomeLocation)
             else:
                 defaultdirectory = ""
-        elif IsPySide2:
+        elif IsPySide2 or IsPyQt5:
             if self.config["mediaSearchDirectories"] and os.path.isdir(self.config["mediaSearchDirectories"][0]):
                 defaultdirectory = self.config["mediaSearchDirectories"][0]
             elif os.path.isdir(self.mediadirectory):
diff --git a/syncplay/ui/gui.py b/syncplay/ui/gui.py
index b7a9a9f..d2ad6e5 100755
--- a/syncplay/ui/gui.py
+++ b/syncplay/ui/gui.py
@@ -19,7 +19,7 @@ from syncplay.utils import resourcespath
 from syncplay.utils import isLinux, isWindows, isMacOS
 from syncplay.utils import formatTime, sameFilename, sameFilesize, sameFileduration, RoomPasswordProvider, formatSize, isURL
 from syncplay.vendor import Qt
-from syncplay.vendor.Qt import QtCore, QtWidgets, QtGui, __binding__, __binding_version__, __qt_version__, IsPySide, IsPySide2
+from syncplay.vendor.Qt import QtCore, QtWidgets, QtGui, __binding__, __binding_version__, __qt_version__, IsPySide, IsPySide2, IsPyQt5
 from syncplay.vendor.Qt.QtCore import Qt, QSettings, QSize, QPoint, QUrl, QLine, QDateTime
 applyDPIScaling = True
 if isLinux():
@@ -32,6 +32,8 @@ if hasattr(QtCore.Qt, 'AA_UseHighDpiPixmaps'):
     QtWidgets.QApplication.setAttribute(QtCore.Qt.AA_UseHighDpiPixmaps, applyDPIScaling)
 if IsPySide2:
     from PySide2.QtCore import QStandardPaths
+if IsPyQt5:
+    from PyQt5.QtCore import QStandardPaths
 if isMacOS() and IsPySide:
     from Foundation import NSURL
     from Cocoa import NSString, NSUTF8StringEncoding
@@ -808,7 +810,7 @@ class MainWindow(QtWidgets.QMainWindow):
                 self.listTreeView.setFirstColumnSpanned(roomtocheck, self.listTreeView.rootIndex(), True)
                 roomtocheck += 1
             self.listTreeView.header().setStretchLastSection(False)
-            if IsPySide2:
+            if IsPySide2 or IsPyQt5:
                 self.listTreeView.header().setSectionResizeMode(0, QtWidgets.QHeaderView.ResizeToContents)
                 self.listTreeView.header().setSectionResizeMode(1, QtWidgets.QHeaderView.ResizeToContents)
                 self.listTreeView.header().setSectionResizeMode(2, QtWidgets.QHeaderView.ResizeToContents)
@@ -822,7 +824,7 @@ class MainWindow(QtWidgets.QMainWindow):
             if self.listTreeView.header().width() < (NarrowTabsWidth+self.listTreeView.header().sectionSize(3)):
                 self.listTreeView.header().resizeSection(3, self.listTreeView.header().width()-NarrowTabsWidth)
             else:
-                if IsPySide2:
+                if IsPySide2 or IsPyQt5:
                     self.listTreeView.header().setSectionResizeMode(3, QtWidgets.QHeaderView.Stretch)
                 if IsPySide:
                     self.listTreeView.header().setResizeMode(3, QtWidgets.QHeaderView.Stretch)
@@ -1003,7 +1005,7 @@ class MainWindow(QtWidgets.QMainWindow):
                 defaultdirectory = QtGui.QDesktopServices.storageLocation(QtGui.QDesktopServices.HomeLocation)
             else:
                 defaultdirectory = ""
-        elif IsPySide2:
+        elif IsPySide2 or IsPyQt5:
             if self.config["mediaSearchDirectories"] and os.path.isdir(self.config["mediaSearchDirectories"][0]) and includeUserSpecifiedDirectories:
                 defaultdirectory = self.config["mediaSearchDirectories"][0]
             elif includeUserSpecifiedDirectories and os.path.isdir(self.mediadirectory):

[Nowa-Ammerlaan]

Would you please consider merging these changes as they are now? Nothing is changed in the functionality at all, all that is done is make some conversions that are done anyway explicit and change the alignment operator which doesn't change anything visually.

Pretty please?