[Small optimization proposal] Eliminate saving remote playlist to tmp file

[diracsbracket]

Hi. Currently, remote playlists are handled by SongLoader::LoadRemotePlaylist() which saves the playlist to a tmp file in order to be parsed after which it is deleted.

This can be completely avoided by adding the following simple method:

//.h
void LoadPlaylistData(ParserBase* parser, QByteArray& data);

//.cpp
void SongLoader::LoadPlaylistData(ParserBase* parser, QByteArray& data) {
  QBuffer buffer(&data);
  buffer.open(QIODevice::ReadOnly);
  songs_ = parser->Load(&buffer, QString(), QString());
}

And using it as follows:

bool SongLoader::LoadRemotePlaylist(const QUrl& url) {
   ...

#if 0
  // Save them to a temporary file...
  QString playlist_filename =
      Utilities::SaveToTemporaryFile(data_reply->readAll());
  if (playlist_filename.isEmpty()) {
    qLog(Error) << url.toString()
                << "could not write contents to temporary file";
    return false;
  }

  qLog(Debug) << url.toString() << "with MIME" << mime_type << "loading from"
              << playlist_filename;

  // ...and load it.
  LoadPlaylist(parser, playlist_filename);

  QFile(playlist_filename).remove();

#else

    qLog(Debug) << url.toString() << "with MIME" << mime_type;

    // ...and load it.
    QByteArray data = data_reply->readAll();
    LoadPlaylistData(parser, data);
#endif

  return true;
}

[jbroadus]

Since QNetworkReply is a QIODevice, would it be possible to pass the reply to ParserBase::Load and let it read the data itself?

[diracsbracket]

@jbroadus

Since QNetworkReply is a QIODevice, would it be possible to pass the reply to ParserBase::Load and let it read the data itself?

Even better! Then the #else part reduces further to a one-liner and no additional method needs to be defined!

songs_ = parser->Load(data_reply, QString(), QString());

That works, excellent!

[diracsbracket]

In a similar vein then, SongLoader::Result SongLoader::LoadLocalAsync() could be simplified to

SongLoader::Result SongLoader::LoadLocalAsync(const QString& filename) {
  // First check to see if it's a directory - if so we will load all the songs
  // inside right away.
#if 0
  if (QFileInfo(filename).isDir()) {
#else
    QFileInfo info = QFileInfo(filename);
    if (info.isDir()) {
#endif
...
    // It's a playlist!
#if 0
    LoadPlaylist(parser, filename);
#else
    file.reset();
    songs_ = parser->Load(&file, filename, info.path());
#endif
    return Success;
  }

and SongLoader::LoadPlaylist() could be eliminated too then.

#if 0
  void LoadPlaylist(ParserBase* parser, const QString& filename);
#endif

Although I haven't tested this directly, it should work. Maybe you can confirm.

[jbroadus]

Yeah, it seems like that would work, too. Do you want to open a merge request?

[diracsbracket]

Do you want to open a merge request?

I've never done merge requests before, but am willing to try. I'll try the instructions on this page https://docs.gitlab.com/ee/user/project/merge_requests/creating_merge_requests.html

[jbroadus]

That's probably a good starting point. Happy to help if you have problems.

[diracsbracket]

@jbroadus Hi, I created a local branch named firstmerge

git clone https://github.com/clementine-player/Clementine.git
cd Clementine
git checkout -b firstmerge
<MADE THE CHANGES HERE>
git commit -a -m 'Eliminate saving remote playlist to tmp file [6991]'
git commit --amend --reset-author
<EDITED & SAVED NEW AUTHOR INFO + DID THE FOLLOWING TO AVOID HAVING TO REDO IT IN THE FUTURE>
git config --global user.name "diracsbracket"
git config --global user.email "diracsbracket@gmail.com"

Then I tried git push origin firstmerge, expecting to see a link to the merge request page as described here but I get the following error:

git push origin firstmerge
Username for 'https://github.com': diracsbracket
Password for 'https://diracsbracket@github.com': 
remote: Permission to clementine-player/Clementine.git denied to diracsbracket.
fatal: unable to access 'https://github.com/clementine-player/Clementine.git/': The requested URL returned error: 403

So, I get this Forbidden error. Any ideas why?

[jbroadus]

Yeah, those instructions assume that you have write access to the repository. You need to fork the repo on GitHub first. Look for the button at the top right of the page. After that, you can clone your fork or just add a second remote to the local repo that you already cloned.

[diracsbracket]

@jbraodus

You need to fork the repo on GitHub first.

I created a pull request. Thanks!