Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Add PySide6 support for qta-browser #171

Merged
merged 4 commits into from
Oct 15, 2021

Conversation

kumattau
Copy link
Contributor

@kumattau kumattau commented Oct 13, 2021

This PR is for that qta-browser supports PySide6.
I tested the PR on Linux(x11) and multi-screen.

Please note that qta-browser cannot show correct icons until PySide6's PYSIDE-1685 is released.
By the following workaround, qta-browser works with current PySide6 6.2.0,
but I does not apply the workaround because I think it is not responsibility of qta-browser.

diff --git a/qtawesome/iconic_font.py b/qtawesome/iconic_font.py
index a89a088..cc9caec 100644
--- a/qtawesome/iconic_font.py
+++ b/qtawesome/iconic_font.py
@@ -405,7 +405,7 @@ class IconicFont(QObject):

     def font(self, prefix, size):
         """Return a QFont corresponding to the given prefix and size."""
-        font = QFont(self.fontname[prefix])
+        font = QFont([self.fontname[prefix]])
         font.setPixelSize(round(size))
         if prefix[-1] == 's':  # solid style
             font.setStyleName('Solid')

Fixes #170

@5yutan5
Copy link

5yutan5 commented Oct 13, 2021

Hi.
How about using setFamily() instead of the qfont constructor?
Maybe it can be used with both qt5 and qt6.

diff --git a/qtawesome/iconic_font.py b/qtawesome/iconic_font.py
index a89a088..ca3ca1b 100644
--- a/qtawesome/iconic_font.py
+++ b/qtawesome/iconic_font.py
@@ -405,7 +405,8 @@ class IconicFont(QObject):
 
     def font(self, prefix, size):
         """Return a QFont corresponding to the given prefix and size."""
-        font = QFont(self.fontname[prefix])
+        font = QFont()
+        font.setFamily(self.fontname[prefix])
         font.setPixelSize(round(size))
         if prefix[-1] == 's':  # solid style
             font.setStyleName('Solid')

@kumattau
Copy link
Contributor Author

@5yutan5, Thank you for the comment.

How about using setFamily() instead of the qfont constructor?

I checked it worked well on pyqt5 and pyside6.
I added the commit.

@5yutan5
Copy link

5yutan5 commented Oct 13, 2021

@kumattau, No problem. Thanks for this work👍

@ccordoba12 ccordoba12 changed the title PR: qta-browser supports PySide6 PR: Add PySide6 support for qta-browser Oct 13, 2021
@ccordoba12 ccordoba12 added this to the v1.1.0 milestone Oct 13, 2021
@ccordoba12 ccordoba12 requested a review from dalthviz October 13, 2021 19:21
@5yutan5
Copy link

5yutan5 commented Oct 13, 2021

About QApplication.desktop() problem, if you use QGuiApplication.primaryScreen() does not require exception handling.
But I don't understand the difference between QGuiApplication.primaryScreen() and QGuiApplication.screenAt().
Perhaps this suggestion isn't really needed.

diff --git a/qtawesome/icon_browser.py b/qtawesome/icon_browser.py
index ea11916..7eb0e63 100644
--- a/qtawesome/icon_browser.py
+++ b/qtawesome/icon_browser.py
@@ -95,14 +95,8 @@ class IconBrowser(QtWidgets.QMainWindow):
 
         # QApplication.desktop() has been removed in Qt 6.
         # Instead, QGuiApplication.screenAt is supported in Qt 5.10 or later.
-        try:
-            desktop = QtWidgets.QApplication.desktop()
-            screen = desktop.screenNumber(desktop.cursor().pos())
-            centerPoint = desktop.screenGeometry(screen).center()
-        except:
-            screen = QtGui.QGuiApplication.screenAt(QtGui.QCursor.pos())
-            centerPoint = screen.geometry().center()
-
+        geo = self.geometry()
+        centerPoint = QtGui.QGuiApplication.primaryScreen().availableGeometry().center()
         geo.moveCenter(centerPoint)
         self.setGeometry(geo)

@kumattau
Copy link
Contributor Author

kumattau commented Oct 14, 2021

Hi, @5yutan5

But I don't understand the difference between QGuiApplication.primaryScreen() and QGuiApplication.screenAt().

There is the following difference if screen#0 (primary) and screen#1 (secondary) exist.

  • QGuiApplication.primaryScreen() always returns screen#0 (primary).
  • QGuiApplication.screenAt(cursor's position) returns the screen where there is a cursor.

The original code and my fix show a window on the screen where there is a cursor,
but your suggestion always shows a window on the primary screen regardless of a cursor position.

@5yutan5
Copy link

5yutan5 commented Oct 14, 2021

@kumattau, ok.
Is this mean that using QGuiApplication.primaryScreen() in multiscreen may open the window in unexpected size or screen?
I test only one screen so I couldn't find the difference😅

@kumattau
Copy link
Contributor Author

Is this mean that using QGuiApplication.primaryScreen() in multiscreen may open the window in unexpected size or screen?

@5yutan5, yes that's right.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @kumattau for noticing this and submitting the corresponding fixes! I left some suggestions for the except blocks but otherwise this LGTM 👍

qtawesome/icon_browser.py Outdated Show resolved Hide resolved
qtawesome/icon_browser.py Outdated Show resolved Hide resolved
@kumattau
Copy link
Contributor Author

@dalthviz thank you for the review.
I applied all your suggestions and updated comment.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kumattau ! This LGTM 👍

Also, thank you @5yutan5 for suggesting improvements and checking the work being done here!

@dalthviz dalthviz merged commit 3dc321d into spyder-ide:master Oct 15, 2021
@kumattau kumattau deleted the qta-browser-pyside6 branch October 15, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

qta-browser with PySide6
4 participants