QtMultimedia availability check added

[timur-enikeev]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Problem

After internal music player has been added, picard crashes if PyQt5.QtMultimedia module is not available. I have added availability check for QtMultimedia module.

• JIRA ticket (optional): PICARD-1488

Solution

I have added handling of ImportError exception when we try to import PyQt5.QtMultimedia. If the import fails, qt_multimedia_available variable will be False. If the import was successful, this variable will be True.

Action

[timur-enikeev]

@samj1912

This should be an except ImportError instead of a broad except.

But what if there will be another exception type? I don't know entrails of pyqt. Maybe it can raise another exception type.

[zas]

No need to pre-format with log.warning() and we usually place the detailed reason after the main message.
Also import will raise ImportError

try:
    from PyQt5 import QtMultimedia
except ImportError as e:
    qt_multimedia_available = False
    log.warning('Internal player disabled: %s', e.msg)
else:
    qt_multimedia_available = True

[zas]

We could also try to import directly here and only set player property if a player is actually available.

self.player = None
try:
    from PyQt5 import QtMultimedia
    player = QtMultimedia.QMediaPlayer(self)
    if player.availability() != QtMultimedia.QMultimedia.ServiceMissing:
        self.player = player
        self.player.error.connect(self._on_player_error)
except ImportError as e:
    log.warning('Internal player disabled: %s', e.msg)

It would simplify tests:

if self.player:
   do_something_with(self.player)

And ensure an exception is raised if self.player is used without being really available (like using self.player.volume()).

@timur-enikeev @phw : what do you think ?

[phw]

I am not a big fan of importing inline, I think this is clearer if done at the top. Apart from that I agree, so I would change your example to:

self.player = None
player = QtMultimedia.QMediaPlayer(self)
if qt_multimedia_available and player.availability() != QtMultimedia.QMultimedia.ServiceMissing:
    self.player = player
    self.player.error.connect(self._on_player_error)

[zas]

I implemented this, with extra error logging in #1207

[zas]

But what if there will be another exception type? I don't know entrails of pyqt. Maybe it can raise another exception type.

If another exception is raised, then it shouldn't be converted to a warning, because it'd be unexpected.
@samj1912 is right here, we should only catch ImportError.

But testing lack of exception on module import isn't sufficient to ensure the player is available, see #1203 (comment)

We have 3 cases:

• module cannot be imported at all
• module can be imported but player isn't available because of missing dependencies (typically gstreamer on linux)
• module can be imported and player is available

[zas]

Closing this pull request as #1207 includes it.