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

Fixing broken bug cover dialog in case no cover is avaiable #2073

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Apr 3, 2019

@daschuer daschuer added this to the 2.2.1 milestone Apr 3, 2019
@uklotzde
Copy link
Contributor

uklotzde commented Apr 6, 2019

I'm not able to reproduce the bug that has been flagged as critical. How do I open the cover art window for tracks with no cover art?

@ronso0
Copy link
Member

ronso0 commented Apr 6, 2019

works as it should.

interesting: I never noticed the issue as the (empty) cover window wouldn't open every time. Sometimes it took 3-4 attempts to open it, and on other occassions I could see a window being opened for a fraction of a second but it disappeared right away. Also the (empty) cover would open with completely different sizes.. strange. but fixed now

@uklotzde
Copy link
Contributor

uklotzde commented Apr 6, 2019

I'm not qualified to verify this fix without being able to reproduce the reported issue.

@uklotzde
Copy link
Contributor

uklotzde commented Apr 9, 2019

Is this the intended behavior?

  • The cover art window keeps displaying the last available cover when switching to next/prev track, even if those tracks have no cover art
  • A small, empty window is opened for tracks without cover art

This is behavior is actually inconsistent and the content of the window does not always reflect the properties of the selected track.

@daschuer
Copy link
Member Author

daschuer commented Apr 9, 2019

This PR is only a quick fix to restore the 2.1 state. There is not working code fragments that changes the cover when selecting a new track. But this looks more like a copy and paste left over from the cover widget. We have already a bug for your described behavior: https://bugs.launchpad.net/mixxx/+bug/1807450 I think we can do it later.

@uklotzde
Copy link
Contributor

uklotzde commented Apr 9, 2019

The behavior was more inconsistent before the change and I'm not able to reproduce any bug. I have no objections against the code, but I'm still waiting for someone to explain to me why we should merge this minor change???

@uklotzde
Copy link
Contributor

uklotzde commented Apr 9, 2019

Does it depend on the Qt version?

@daschuer
Copy link
Member Author

The issue here is a regression from the Qt5 dead lock fix #1849

@uklotzde
Copy link
Contributor

One last try: How can I reproduce the bug to to verify this fix??

@daschuer
Copy link
Member Author

Use an effected Os (Ubuntu Xenial is known), click on the empty cover cover art widget and see the GUI skin shine through the cover window. Click on close (x) but nothing happens.

@uklotzde
Copy link
Contributor

uklotzde commented Apr 16, 2019

I'm not able to open the cover art window for tracks without cover art on Fedora 29 / Qt 5.11 The window is automatically closed as soon as trying to select the cover of a track without cover art. The status quo works as expected and this fix makes it worse. Sorry, but I can't make an informed decision on how to handle this PR Retested with 2.2: The cover art window is not updated when changing the track selection. But the window is closed as soon as selecting clicking on empty cover art.

@uklotzde
Copy link
Contributor

Retested again: I'm now able to open multiple (at least 2) cover art windows from the deck and the properties dialog at once? Only a minor issue.

The cover art window opened from a deck is automatically updated when loading another track, but not when changing the track selection within the property dialog. This (inconsistent) behavior seems to be unchanged after testing various use cases with different versions.

Since the behavior already seems to be slightly inconsistent it's ok to merge this PR if it fixes a serious issue.

@daschuer
Copy link
Member Author

Yes, this cover window really need some love. Let's track the remaining issues in this bug:
https://bugs.launchpad.net/bugs/1807450

@daschuer
Copy link
Member Author

Since you have not discovered regressions we could merge this now.

@daschuer daschuer merged commit 168db70 into mixxxdj:master Apr 17, 2019
@daschuer
Copy link
Member Author

Ups ... this should go to 2.2.1 as well. I will cherry pick this commit upstream.

@uklotzde
Copy link
Contributor

Done.

@daschuer
Copy link
Member Author

Thank you :-)

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.

3 participants