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

PICARD-817, PICARD-960: Port Picard to Qt5(Continued) #503

Merged
merged 8 commits into from
Apr 3, 2017

Conversation

sambhav
Copy link
Collaborator

@sambhav sambhav commented Dec 21, 2016

Continues and finishes #385

@sambhav sambhav changed the title Port Picard to PyQt5 PICARD-817: Port Picard to PyQt5 Dec 21, 2016
@mwiencek
Copy link
Member

I think the commits should be named "Port Picard to Qt5" and "Port Picard to Qt5 (continued)", where the commit message for the second commit explains that it's a continuation of e41fb3d.

# XXX QPyNullVariant does not exist in all PyQt versions, and was
# removed entirely in PyQt5. See:
# http://pyqt.sourceforge.net/Docs/PyQt5/pyqt_qvariant.html
if str(type(value)) == "<class 'PyQt4.QtCore.QPyNullVariant'>":
Copy link
Member

Choose a reason for hiding this comment

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

If someone had such a value in their configuration and upgraded Picard, what would str(type(value)) return? We should figure that out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

http://pyqt.sourceforge.net/Docs/PyQt5/pyqt_qvariant.html

According to this it will just convert it to a python object. So I suppose it will just return a str type?

except AttributeError:
_fromUtf8 = lambda s: s

class ArtworkTable(QtGui.QTableWidget):
Copy link
Member

Choose a reason for hiding this comment

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

Huh, this shouldn't be removed. Are you sure you rebased things against latest master?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, the file was edited manually in 7451602 (not good). So we'll have to move the custom code to somewhere else, without changing any behavior.

@sambhav sambhav changed the title PICARD-817: Port Picard to PyQt5 PICARD-817: Port Picard to Qt5(Continued) Dec 22, 2016
@sambhav
Copy link
Collaborator Author

sambhav commented Feb 10, 2017

Updated the Qt5 port. (All of picard till 1.4 is ported now) Although all of my commits got merged into @mwiencek 's original commit while rebasing.

@mineo
Copy link
Member

mineo commented Feb 11, 2017

At the moment, the settings can't be opened:

Traceback (most recent call last):
  File "./picard/ui/mainwindow.py", line 746, in show_options
    dialog = OptionsDialog(page, self)
  File "./picard/ui/options/dialog.py", line 121, in __init__
    page.load()
  File "./picard/ui/options/scripting.py", line 402, in load
    self.ui.script_list.setItemSelected(last_selected_script, True)
AttributeError: 'QListWidget' object has no attribute 'setItemSelected'

@@ -512,7 +512,7 @@ def browse_releases(self, handler, priority=True, important=True, **kwargs):
def submit_ratings(self, ratings, handler):
host = config.setting['server_host']
port = config.setting['server_port']
path = '/ws/2/rating/'
path = '/ws/2/rating/?client=' + CLIENT_STRING
Copy link
Member

Choose a reason for hiding this comment

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

See 2113a13, this is not correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did correct it. No idea why it didn't update O.o.

@mineo
Copy link
Member

mineo commented Feb 12, 2017

We should probably decide which minimum version of (Py)Qt5 we want to require. Most of the HighDPI things seem to require Qt 5.6 (or at a minimum 5.4), redirection (I think this came up somewhere recently, but don't remember where) support in QNetworkAccessManager is 5.6+ as well.

@sambhav sambhav changed the title PICARD-817: Port Picard to Qt5(Continued) PICARD-817, PICARD-960: Port Picard to Qt5(Continued) Feb 12, 2017
@Freso
Copy link
Member

Freso commented Feb 13, 2017

We should probably decide which minimum version of (Py)Qt5 we want to require.

Given the amount of things we want to change, Picard 2.0 probably won't be out for a while yet, so my vote is for not putting the bar on a too old version. 5.6, or even 5.7, would get my vote. (Qt 5.8 was released in January, so PyQt 5.8 will likely get released soon. While I would be happy to go for this version as well, that might be a tad too recent for even non-Debian systems. :))

@zas
Copy link
Collaborator

zas commented Feb 13, 2017

Given the amount of things we want to change, Picard 2.0 probably won't be out for a while yet, so my vote is for not putting the bar on a too old version. 5.6, or even 5.7, would get my vote. (Qt 5.8 was released in January, so PyQt 5.8 will likely get released soon. While I would be happy to go for this version as well, that might be a tad too recent for even non-Debian systems. :))

Qt5.4 is the first version with HiDPI support, but it was greatly reworked in 5.6. But 5.6 was released in March 2016, it is a LTS version. 5.4 supports for HiDPI was implemented differently, it was released back 2014. Ubuntu 16.04 comes with Qt 5.5.
So i think we should target the lowest version for now, perhaps adding tests for more recent versions (especially because HiDPI-related features are quite different among 5.x versions).
It also means we can't use 5.6+-only features for now.

@Sophist-UK
Copy link
Contributor

Sounds to me like we should go with 5.6.

@sambhav
Copy link
Collaborator Author

sambhav commented Feb 13, 2017

Ditto with 5.6 given the above points in its favor.

@Freso
Copy link
Member

Freso commented Feb 13, 2017

Ubuntu 16.04

If people with old Ubuntu's want to run Picard, Picard 1.4.x should continue to work fine for them for some time to come. What does Ubuntu 16.10 ship? Or what will Ubuntu 17.04 and 17.10 have? I doubt Picard 2.0 will be out before October, so non-hardcore-LTS'ers should be able to have switched to a Ubuntu version with a more recent version.

Also, this is one of the reasons I really dislike non-rolling release distros: they hamper software development. Let's not let the "most outdated denominator" determine what library to use, esp. if it means more work 6–12+ months from now. If @samj1912 suddenly gets caught up with something (e.g., paid work, studies, …), then we can wait 2+ years for someone else to pick up the mantle and move us onwards from (Py)Qt 5.4. Let's try and not stay too far behind the curve, please?

(See also: The GoPHP5 mission.)

@mineo
Copy link
Member

mineo commented Feb 17, 2017

Trying to use the OAuth authentication only gets me

Traceback (most recent call last):
  File "./picard/ui/options/general.py", line 91, in login
    authorization_code, scopes, self.on_authorization_finished)
  File "./picard/oauth.py", line 138, in exchange_authorization_code
    url.setEncodedQuery(url_query)
AttributeError: 'QUrl' object has no attribute 'setEncodedQuery'
zsh: abort (core dumped)  python2 tagger.py

@sambhav
Copy link
Collaborator Author

sambhav commented Feb 20, 2017

@mineo fixed :)

Dockerfile Outdated
RUN python setup.py build_locales -i
CMD python setup.py test
RUN python setup.py build_ext -i
RUN python setup.py build_locales -i
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still extra whitespace at end of line (it was worse before though).

@@ -63,17 +63,22 @@ def mouseReleaseEvent(self, event):
self.clicked.emit()

def dragEnterEvent(self, event):
for url in event.mimeData().urls():
if url.scheme() in ('https', 'http', 'file'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test about scheme is gone ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, it is checked again in dropEvent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, just noticed. Thanks.

@@ -297,7 +297,7 @@ def _display_info_tab(self):
if album.errors:
tabWidget.setTabText(tab_index, _("&Errors"))
text = '<br />'.join(map(lambda s: '<font color="darkred">%s</font>' %
'<br />'.join(unicode(QtCore.Qt.escape(s))
'<br />'.join(unicode(cgi.escape(s))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to create picard.util.htmlescape() so we can change it to whatever fits the best.

def fetch_remote_image(self, url):
return self.parent().fetch_remote_image(url)
def fetch_remote_image(self, url, dropped_data):
return self.parent().fetch_remote_image(url, dropped_data)

Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing this I noticed a problem in master, so I created #665 . Can you review that PR and make the corresponding rebase here? (note that the PR removes fetch_remote_image from CoverArtThumbnail)

@@ -67,6 +56,7 @@ def setupUi(self, InfoDialog):
InfoDialog.setTabOrder(self.tabWidget, self.buttonBox)

def retranslateUi(self, InfoDialog):
_translate = QtCore.QCoreApplication.translate
Copy link
Contributor

Choose a reason for hiding this comment

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

is _translate used somewhere?

self.vboxlayout.addWidget(self.buttonbox)

self.retranslateUi(Dialog)
QtCore.QMetaObject.connectSlotsByName(Dialog)

def retranslateUi(self, Dialog):
_translate = QtCore.QCoreApplication.translate
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, maybe you wanted to set _ ?

self.vboxlayout.addItem(spacerItem)

self.retranslateUi(AdvancedOptionsPage)
QtCore.QMetaObject.connectSlotsByName(AdvancedOptionsPage)

def retranslateUi(self, AdvancedOptionsPage):
_translate = QtCore.QCoreApplication.translate
Copy link
Contributor

Choose a reason for hiding this comment

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

And also here and all other retranslateUi methods in other classes

@@ -173,7 +160,7 @@ def retranslateUi(self, RenamingOptionsPage):
"<html><head><meta name=\"qrichtext\" content=\"1\" /><style type=\"text/css\">\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to have the only call to _translate in all picard's code (at least from a quick grep I did). Maybe it can be changed to _ and remove the _translate entry from _translate_re in setup.py ?

@zas zas merged commit 5022d5d into metabrainz:master Apr 3, 2017
@mwiencek
Copy link
Member

mwiencek commented Apr 3, 2017

You removed my authorship from the initial commit. :(

@sambhav
Copy link
Collaborator Author

sambhav commented Apr 3, 2017

You removed my authorship from the initial commit. :(

Sorry, it had too many merge conflicts, I had to checkout every file from the current qt5 branch.
Umm, can you rebase the master and fix it?

@mwiencek
Copy link
Member

mwiencek commented Apr 3, 2017

It's okay, I don't think it's worth rewriting history. :)

@sambhav sambhav deleted the qt5 branch February 9, 2018 22:04
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.

7 participants