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

Fix OAuthenticator reply parsing. #5603

Closed
wants to merge 2 commits into from
Closed

Fix OAuthenticator reply parsing. #5603

wants to merge 2 commits into from

Conversation

gooseensky
Copy link
Contributor

Provide QByteArray instead of QIODevice to QJson parser.
Bug link: flavio/qjson#52

Provide QByteArray instead of QIODevice to QJson parser.
Bug link: flavio/qjson#52
@gooseensky
Copy link
Contributor Author

Probably will fix #5457

@hatstand
Copy link
Contributor

If this really fixes things, then I'd like to know why because we have this pattern everywhere.

@gooseensky
Copy link
Contributor Author

gooseensky commented Jan 19, 2017

This pattern is correct, but there is a regression in libqjson, which was introduced between 0.8.1 and 0.9.0 releases (flavio/qjson@774c778 - break commit, if someone is interested).

@gooseensky
Copy link
Contributor Author

I've updated all problem places in code. It resolves issues with loggin into Google Drive and Dropbox, for example.

@hatstand
Copy link
Contributor

We should rely on the upstream fix I think and perhaps check for QJson > 0.9.0 at compile time.

@gooseensky
Copy link
Contributor Author

gooseensky commented Jan 30, 2017

@hatstand But clementine use shared version of qjson, so compile time check wouldn't solve the problem.
Could we temporary add qjson to project's repo as 3rd party and build it as static library with correct behavior (I've already proposed a pull request with fix to qjson project, so we could take it or previously non-broken version).
When qjson version with fixed behavior will be released, we will drop dependency from repo.

@hatstand
Copy link
Contributor

I realise it's not perfect but it's best if we can avoid adding more things to 3rdparty. Perhaps we can add a wrapper function for now that we use everywhere.

The best thing to do would be to harass distros into updating their qjson for the bugfix.

@abika
Copy link
Contributor

abika commented Apr 7, 2017

This bug in QJson also breaks fetching tags from Musicbrainz.
How about using the JSON support build-in in Qt? It came with 5.0 which is 4 years old now. We could remove QJson everywhere.

@abika
Copy link
Contributor

abika commented Apr 7, 2017

Nevermind, Clementine is still using Qt4.

Well, I can help if somebody decides how to solve this (globally).

abika added a commit to abika/Clementine that referenced this pull request May 31, 2017
abika added a commit to abika/Clementine that referenced this pull request Aug 1, 2021
@gooseensky gooseensky closed this by deleting the head repository Mar 30, 2024
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.

None yet

3 participants