Fix reading the Spinny cover on Windows #12103
Conversation
|
Great work, @daschuer !
Isn't that something that could have been solved with a simple mutex? Not saying that that would have been a better solution (I am all for reducing needless threading complexity), just trying to understand the changes so I can do the review.
Can you point out the where in the code this change is implemented? |
Yes, a quick hack solution with a mutex would have been possible. The downside would be that only one thread would be have file excess and that multiple access of the same file would have been chained. Now we still use a new concurrent thread for every new file. Simultaneous accesses to the same file, that happens when loadingva track, are performed by a single thread. |
src/library/coverartcache.cpp
Outdated
| // to avoid loading the same picture again while we are loading it | ||
| QPair<const QObject*, mixxx::cache_key_t> requestId = qMakePair(pRequester, requestedCacheKey); | ||
| if (m_runningRequests.contains(requestId)) { | ||
| bool requestPending = m_runningRequests.contains(cacheKey); |
There was a problem hiding this comment.
Here we collect requests to a same file.
Whe the first look up is done m_runningRequests is used to signall all requesters.
src/library/basetracktablemodel.cpp
Outdated
| this, | ||
| coverInfo, | ||
| absoluteHeightOfCoverartToolTip); | ||
| return tr("Fetching image ..."); |
There was a problem hiding this comment.
What is the string used for? Can you add a comment?
There was a problem hiding this comment.
This text is shown as tooltip for the spit of a second before the actual cover is shown. I will add a comment.
There was a problem hiding this comment.
This text is shown as tooltip for the spit of a second before the actual cover is shown.
Than it doesn't work, I see the text, but it's no replaced by the cover art. If I don't move the mouse, it remains there for some seconds, and than the tooltip disappears.
src/library/basetracktablemodel.cpp
Outdated
| getTrackLocation(m_toolTipIndex) != coverInfo.trackLocation) { | ||
| return; | ||
| } | ||
| emit dataChanged(m_toolTipIndex, m_toolTipIndex, {Qt::ToolTipRole}); |
There was a problem hiding this comment.
What has the tooltip to do with the cover?
There was a problem hiding this comment.
If library tooltips are enabled and you move the mouse over a library cover, than the tooltip shows a bigger picture of this cover (mouse pointer is not captured by screenshot-tool):
There was a problem hiding this comment.
Got it!t Thanks! Still discovering mixxx features I never use :-)
src/library/coverart.h
Outdated
| bool hasImage() const { | ||
| return cacheKey() != CoverImageUtils::defaultCacheKey(); | ||
| return type != NONE; |
There was a problem hiding this comment.
might be a good moment to prefix this m_type?
|
The deck spinnies are now loaded, but there is a new issue: If I start with initial library table column size everything works: |
|
Can you allso have a look to that mature #12009 |
daschuer
force-pushed
the
gh11131
branch
3 times, most recently
from
519ab18
to
5724857
Compare
|
I was able to reproduce the column size issue. I have amended the last commit with a fix. |
|
This doesn't compile: |
|
Now it builds again. |
|
I could build it now, and it fixes the issue with the missing cover art in the decks. But the cover art tooltips in the library only work, if the cover art is already cached. |
|
Strange, I am not able to confirm it natively on Ubuntu, but on my windows 10 virtual machine. |
daschuer
force-pushed
the
gh11131
branch
2 times, most recently
from
7cbdfd1
to
a1c7e56
Compare
|
Fixed here by rebasing on #12087 |
|
Rebased on top of the branches this one is based on. |
daschuer
force-pushed
the
gh11131
branch
2 times, most recently
from
9be98cd
to
4db2609
Compare
|
I think this needs to be rebased to latest 2.4, before we can be merge this - or can Git handle this merge without rebase? |
|
Rebased. |
|
Thank you! Still LGTM! |
This finally fixes #11131
This patch awoid to access the same file simultaneously form two threads.
Instead it used the same thread to redponds to both requests.
@JoergAtGithub Please confirm if this fixes your issue.
This PR is on top of #12087 which is on top of #12009