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

Re-add tangoinfo plugin #323

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Re-add tangoinfo plugin #323

merged 1 commit into from
Feb 14, 2022

Conversation

ix5
Copy link
Contributor

@ix5 ix5 commented Jan 29, 2022

This plugin has been ported to Picard 2.x.

It currently needs a workaround for a server misconfiguration for tango.info

Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

Thanks a lot for digging into this and figuring out this odd behavior caused by the explicit port.

Sophist's note about using a comment for the example data should be addressed, but otherwise this looks good to merge to me. Including the workaround. We can remove this anytime should this be fixed on tango.info.

my_url = QUrl(self.url().url())
# HACK: Setting port=defaultPort will cause Host header sent to
# tango.info to change from `tango.info:443` to `tango.info`
my_url.setPort(-1)
Copy link
Member

Choose a reason for hiding this comment

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

We could also omit default ports 80 / 433 from HTTP / HTTPS URLs in Picard already.

So in https://github.com/metabrainz/picard/blob/master/picard/util/__init__.py#L583-L589 just set the scheme based on the port, and only explicitly set the port if it differs from the default port.

@zas @Sophist-UK Do you see any downside doing this?

Apart from that the workaround here should stay for now if the plugin shall be compatible with existing released Picard versions, unless this gets fixed server side. It's a bit of a hack, but it is small code and cleanly done :)

Copy link
Contributor

@Sophist-UK Sophist-UK Jan 30, 2022

Choose a reason for hiding this comment

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

The code in Picard that Philipp referred to is:

def build_qurl(host, port=80, path=None, queryargs=None):
    """
    Builds and returns a QUrl object from `host`, `port` and `path` and
    automatically enables HTTPS if necessary.
    Encoded query arguments can be provided in `queryargs`, a
    dictionary mapping field names to values.
    """
    url = QtCore.QUrl()
    url.setHost(host)
    url.setPort(port)

    if host in MUSICBRAINZ_SERVERS or port == 443:
        url.setScheme("https")
        url.setPort(443)
    else:
        url.setScheme("http")

    if path is not None:
        url.setPath(path)
    if queryargs is not None:
        url_query = QtCore.QUrlQuery()
        for k, v in queryargs.items():
            url_query.addQueryItem(k, str(v))
        url.setQuery(url_query)
    return url

I would argue that it is this code that is causing this problem. http / https implicitly define the port to be used and I don't think we need to set the port explicitly if they are standard. So if it was me I would:

  1. Make the port an optional value.
  2. Either only call setport(port) if port is not None (or if this causes problems setport(-1) if port is None.

My quick take on the Picard code would therefore be:

def build_qurl(host, port=None, path=None, queryargs=None):
    """
    Builds and returns a QUrl object from `host`, `port` and `path` and
    automatically enables HTTPS if necessary.
    Encoded query arguments can be provided in `queryargs`, a
    dictionary mapping field names to values.
    """
    url = QtCore.QUrl()
    url.setHost(host)
    if port and port not in (80, 443):
        url.setPort(port)
    if host in MUSICBRAINZ_SERVERS or port == 443:
        url.setScheme("https")
    else:
        url.setScheme("http")
    if path is not None:
        url.setPath(path)
    if queryargs is not None:
        url_query = QtCore.QUrlQuery()
        for k, v in queryargs.items():
            url_query.addQueryItem(k, str(v))
        url.setQuery(url_query)
    return url

I think that this would avoid the need for a hack completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy with whatever you decide. But note that for backward compatibility, the "hack" would need to stay in (behind a guard).
I'll try to verify which versions of Picard this plugin can run on successfully and adjust accordingly.

One question, probably not all too important: Are there any services that will be accessed encrypted (https), but not on port 443?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. AFAIK, you have to use 443 for SSL but if there is a server that requires https on a different port, there is no way with the current code or my proposed alternative to achieve that.

Copy link
Member

Choose a reason for hiding this comment

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

See metabrainz/picard#2057 for the changes in port handling in Picard's build_qurl

One question, probably not all too important: Are there any services that will be accessed encrypted (https), but not on port 443?

As Sophist said this would be currently unsupported. This would require actually refactoring the picard.webservice API. Currently everything works by passing a hostname + port, but you cannot specify the scheme. Port 443 is assumed to be https, everything else gets http. But we should definitely refactor the API here at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could only verify as far back as version release-2.6.4, everything else crashes on startup for me (Arch Linux, current as of 2022-01-30). So I've put API compatibilty to 2.6-2.7.

Tested with metabrainz/picard#2057 and without (overriding version to .dev0), both handle fine.

Crash:

D: 16:50:51,470 /user/foo/picard/picard/ui/playertoolbar.__init__:90: Internal player: QtMultimedia available, initializing QMediaPlayer
D: 16:50:51,549 /user/foo/picard/picard/ui/playertoolbar.__init__:97: Internal player: available, QMediaPlayer set up
Traceback (most recent call last):
  File "/user/foo/picard/scripts/picard", line 5, in <module>
    main('/user/foo/picard/.venv-picard/share/locale', True)
  File "/user/foo/picard/picard/tagger.py", line 996, in main
    tagger = Tagger(picard_args, unparsed_args, localedir, autoupdate)
  File "/user/foo/picard/picard/tagger.py", line 285, in __init__
    self.window = MainWindow(disable_player=picard_args.no_player)
  File "/user/foo/picard/picard/ui/mainwindow.py", line 198, in __init__
    self.setupUi()
  File "/user/foo/picard/picard/ui/mainwindow.py", line 233, in setupUi
    self.cover_art_box = CoverArtBox(self)
  File "/user/foo/picard/picard/ui/coverartbox.py", line 315, in __init__
    self.cover_art = CoverArtThumbnail(False, True, self.pixmap_cache, parent)
  File "/user/foo/picard/picard/ui/coverartbox.py", line 74, in __init__
    self.shadow = self.shadow.scaled(w, h, QtCore.Qt.KeepAspectRatio, QtCore.Qt.SmoothTransformation)
TypeError: arguments did not match any overloaded call:
  scaled(self, int, int, aspectRatioMode: Qt.AspectRatioMode = Qt.IgnoreAspectRatio, transformMode: Qt.TransformationMode = Qt.FastTransformation): argument 1 has unexpected type 'float'
  scaled(self, QSize, aspectRatioMode: Qt.AspectRatioMode = Qt.IgnoreAspectRatio, transformMode: Qt.TransformationMode = Qt.FastTransformation): argument 1 has unexpected type 'float'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/user/foo/picard/.venv-picard/bin/picard", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/user/foo/picard/scripts/picard", line 9, in <module>
    from picard import crash_handler
ImportError: cannot import name 'crash_handler' from 'picard' (/user/foo/picard/picard/__init__.py)

@ix5
Copy link
Contributor Author

ix5 commented Jan 30, 2022

@phw would you mind cherry-picking the Github CI commit disabling py3.5 (or take whatever other action you deem appropriate)?
I don't think it should be part of this PR.

plugins/tangoinfo/tangoinfo.py Outdated Show resolved Hide resolved
plugins/tangoinfo/tangoinfo.py Show resolved Hide resolved
plugins/tangoinfo/tangoinfo.py Show resolved Hide resolved
plugins/tangoinfo/tangoinfo.py Outdated Show resolved Hide resolved
my_url = QUrl(self.url().url())
# HACK: Setting port=defaultPort will cause Host header sent to
# tango.info to change from `tango.info:443` to `tango.info`
my_url.setPort(-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. AFAIK, you have to use 443 for SSL but if there is a server that requires https on a different port, there is no way with the current code or my proposed alternative to achieve that.

Copy link
Contributor

@Sophist-UK Sophist-UK left a comment

Choose a reason for hiding this comment

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

If you could tweak the comments again...

plugins/tangoinfo/tangoinfo.py Outdated Show resolved Hide resolved
@phw
Copy link
Member

phw commented Jan 30, 2022

would you mind cherry-picking the Github CI commit disabling py3.5
@ix5 I did this, and also enabled testing with Python 3.9 + 3.10. After rebasing your branch should be mergable again.

@ix5 ix5 changed the title [DRAFT] Re-add tangoinfo plugin Re-add tangoinfo plugin Jan 30, 2022
@ix5 ix5 marked this pull request as ready for review January 30, 2022 15:48
@ix5
Copy link
Contributor Author

ix5 commented Jan 30, 2022

If you could tweak the comments again...

Done

@ix5 I did this, and also enabled testing with Python 3.9 + 3.10. After rebasing your branch should be mergable again.

Rebased.

Also tested with metabrainz/picard#2057 applied.

Thank you both for your review!

@ix5
Copy link
Contributor Author

ix5 commented Feb 1, 2022

Fixed a condition (introduced by my "too-clever-by-half" re-writing of the data-extract handling) where if either genre, date or vocal was missing there'd be a key error.


Just FYI, the CI check seems to still want to wait for a python 3.5 build ("some checks haven't completed yet"). I suspect that's something you'd have to turn off inside the GH UI somewhere.

Also, is there any effort to have (unit) tests for plugins I should be aware of? I've added a basic test (with a lot of assumptions) to my own plugin repo, see tests.py

@ix5
Copy link
Contributor Author

ix5 commented Feb 13, 2022

Hey @phw, is there anything else you'd like changed? I see metabrainz/picard#2057 got merged.

plugins/tangoinfo/tangoinfo.py Outdated Show resolved Hide resolved
plugins/tangoinfo/tangoinfo.py Outdated Show resolved Hide resolved
plugins/tangoinfo/tangoinfo.py Outdated Show resolved Hide resolved
The plugin had been removed in
6fc6f94 because of a server
issue from tango.info, see [1].

This plugin has been modified to work with current versions
of Picard.

It currently needs a workaround for a server
misconfiguration for tango.info. Once this issue is
resolved on tango.info's side, the workaround can be
removed.

Versions 2.8+ of Picard will handle URL construction and the
Host header differently [2], so the workaround is guarded by
a version check.

[1]: #172
[2]: metabrainz/picard#2057
@ix5 ix5 requested review from zas and phw February 13, 2022 20:17
Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

@ix5 No, all good. I just did not get around to look at this again. Thanks for the reminder. I'll merge this, the plugin should than show up on the Picard website (and inside Picard) soon.

@zas Anything to add before I merge?

@ix5
Copy link
Contributor Author

ix5 commented Feb 13, 2022

Excellent! Thank you again for the review, and @zas for spotting the optimizations I overlooked.

@zas
Copy link
Collaborator

zas commented Feb 13, 2022

@zas Anything to add before I merge?

Nope, LGTM.

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.

4 participants