Scrobbler: fix issue where if session key is not empty, a QNetworkAccessManager never gets created.
ClosedPublic

Authored by tanjoodo on Nov 24 2018, 7:53 PM.

Details

Summary

In the case where a user already logged in and has a session key created, their tracks are never scrobbled because m_networkAccessManager will never be initialized. The initialization is done inside getAuthToken which is not called if a session key is available.

Test Plan

With internet connection:

  1. Have JuK already configured for scrobbling with Settings -> Configure Scrobbling
  2. Play a track and observe the track being scrobbled on the last.fm website

With no internet connection:

  1. Have JuK already configured for scrobbling with Settings -> Configure Scrobbling
  2. Play a track and JuK does not crash

Starting with internet connection then disabling connection:

  1. Have internet connection enabled
  2. Have JuK already configured for scrobbling with Settings -> Configure Scrobbling
  3. Play a track and observe the track being scrobbled on the last.fm website
  4. Disconnect internet connection
  5. Play a track and JuK does not crash

Diff Detail

Repository
R344 Juk
Lint
Lint Skipped
Unit
Unit Tests Skipped
tanjoodo requested review of this revision.Nov 24 2018, 7:53 PM
tanjoodo created this revision.
mpyne added a comment.Dec 1 2018, 11:19 PM

Thanks for the patch! Since the m_networkAccessManager is now sure to be constructed once the Scrobbler constructor runs, I decided instead to just initialize that variable directly without making it conditional, and remove its creation in getAuthToken.

I've committed it to 18.12 and master.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 1 2018, 11:50 PM
This revision was automatically updated to reflect the committed changes.
tanjoodo added a comment.EditedDec 2 2018, 6:56 AM

Sounds good! Thanks!

However do bear in mind that in the original code, m_networkAccessManager would not be initialized if no username and password were provided. You can see that in the Scrobbler::getAuthToken() method. I'm not sure if constructing or retaining QNetworkAccessManager is actually expensive enough for us to care, but I was following the convention I found in the code.