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

Add support for QtMultimedia #50

Merged
merged 1 commit into from Aug 16, 2016
Merged

Conversation

davvid
Copy link
Contributor

@davvid davvid commented Aug 2, 2016

Add a wrapper around QtMultimedia to provide access to QAudio, QSound
and friends.

Closes #49
Signed-off-by: David Aguilar davvid@gmail.com

@goanpeca
Copy link
Member

goanpeca commented Aug 2, 2016

Hi @davvid, thanks for the PR

I just made some changes, could you rebase or merge with latests master?

@davvid
Copy link
Contributor Author

davvid commented Aug 2, 2016

Thanks, just rebased.

@goanpeca
Copy link
Member

goanpeca commented Aug 2, 2016

@davvid the tests folder is now inside the qtpy module, can you check this is the case?

@davvid
Copy link
Contributor Author

davvid commented Aug 2, 2016

Oops, fixed it to go into the qtpy/tests/ folder. Thanks!

@goanpeca
Copy link
Member

goanpeca commented Aug 2, 2016

Awesome, thanks :-)

The failing tests on appveyor are my fault. fixing now, will ping you for a rebase :-|

@goanpeca
Copy link
Member

goanpeca commented Aug 3, 2016

Hey @davvid, I fixed the CI, could you please rebase once more?


@ccordoba12 I did some clean up of the codebase:

  • Moved tests to the qtpy folder so they can be run when installed (could be useful for finding problems)
  • I added circle-ci (for linux and qt5)
  • I adapted the travis and appveyor tests to use the qttesting channel so now qt5 is also being checked.
  • Added coverage checks, QC and Scrutinizer.

@davvid davvid force-pushed the qtmultimedia branch 2 times, most recently from 8fffb97 to 98dfb07 Compare August 3, 2016 00:30
@davvid
Copy link
Contributor Author

davvid commented Aug 3, 2016

Thanks @goanpeca I've rebased and all are passing except QuantifiedCode which seems to be tripping up over the import * that was added. All good?

@goanpeca
Copy link
Member

goanpeca commented Aug 3, 2016

Thanks @goanpeca I've rebased and all are passing except QuantifiedCode which seems to be tripping up over the import * that was added. All good?

Yes, it looks good, just waiting for @ccordoba12 to double check.

For now the import * is ok, but I am opening new PRs to explicitely import the contents and avoid *, which will also help in autocompletion.

@ccordoba12 ccordoba12 modified the milestones: v1.1.2, v1.2 Aug 3, 2016
@ccordoba12
Copy link
Member

@davvid, thanks a lot for this!

@goanpeca, please don't merge this one yet. Since this is a new feature, and not just a bugfix, it needs to go in 1.2, instead of 1.1.2.

To preserve semantic versioning, I'm going to release 1.1.2 tomorrow, and 1.2 will come after that :-)

@ccordoba12
Copy link
Member

@goanpeca, another thing: could we remove the QuantifiedCode integration until you remove our star imports?

@goanpeca
Copy link
Member

goanpeca commented Aug 3, 2016

I added a protected branch and required checks before merging. Quantified code is an optional check now. So tests will appear as passing (no need to disable it)

@goanpeca
Copy link
Member

goanpeca commented Aug 3, 2016

@davvid could you rebase again :-) ?

@ccordoba12
Copy link
Member

Thanks @goanpeca!

@davvid
Copy link
Contributor Author

davvid commented Aug 3, 2016

Rebased. Thanks y'all, sounds great.

@goanpeca goanpeca closed this Aug 3, 2016
@goanpeca goanpeca reopened this Aug 3, 2016
@ccordoba12
Copy link
Member

@davvid, I just released 1.1.2 and now it seems this needs another rebase :-)

@goanpeca
Copy link
Member

goanpeca commented Aug 8, 2016

@ccordoba12 I had not yet made the fix PR to remove the problem that would otherwise break navigator.

Did you fix it? otherwise if that new release makes it the repo.continuum.io is going to break navigator.

@ccordoba12
Copy link
Member

Sorry, I thought you had done it already :-/ We discussed about it several days, and since this is a very simple fix, I assumed it was done :-)

@goanpeca
Copy link
Member

goanpeca commented Aug 9, 2016

Sorry, I thought you had done it already :-/ We discussed about it several days, and since this is a very simple fix, I assumed it was done :-)

Well you just broke navigator...

@davvid
Copy link
Contributor Author

davvid commented Aug 9, 2016

Thanks @ccordoba12 just rebased onto the latest origin/master and re-pushed.

@ccordoba12
Copy link
Member

@davvid, an error in our CircleCI configuration was making your PR to fail. This was fixed in PR #59, so a new rebase should fix it.

Sorry for all the hassles, by the way ;-)

Add a wrapper around QtMultimedia to provide access to QAudio, QSound
and friends.

Closes spyder-ide#49
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid
Copy link
Contributor Author

davvid commented Aug 16, 2016

Sweet, just rebased for the fix. Thanks @ccordoba12

@ccordoba12
Copy link
Member

Thanks, merging (at last :-)

@ccordoba12 ccordoba12 changed the title qtpy: add support for qtpy.QtMultimedia Add support for QtMultimedia Aug 16, 2016
@ccordoba12 ccordoba12 merged commit 16e444d into spyder-ide:master Aug 16, 2016
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