Cover Art Copy Worker #10902
Cover Art Copy Worker #10902
Conversation
|
Thank you for the review. |
|
There are some clazy complains. |
fatihemreyildiz
force-pushed
the
coverartworker
branch
from
78de51b
to
8a81580
Compare
fatihemreyildiz
force-pushed
the
coverartworker
branch
from
e36f7ad
to
c63be19
Compare
src/widget/wcoverartmenu.cpp
Outdated
| return; | ||
| } | ||
|
|
||
| m_worker.reset(new CoverArtCopyWorker(image, coverArtCopyFilePath)); |
There was a problem hiding this comment.
This smart pointer will delete the worker even though it might still be running.
We need to wait for the task to be finished in the destructor of this class. Check out other places in the code for a template.
There was a problem hiding this comment.
I've added a quit() in the last method of the worker, and this triggers a bool member in WCoverArtMenu, I've added a VERIFY_OR_DEBUG_ASSERT(m_isWorkerRunning == false), IDK if that solves this issue.
|
With the last commit, I've moved the disc access in WCoverArtMenu to another thread. |
| this, | ||
| &WCoverArtMenu::slotStarted); | ||
|
|
||
| m_worker->start(); |
There was a problem hiding this comment.
the waiting code in the destructor is missing.
There was a problem hiding this comment.
I couldn't understand it could you please explain it a bit more please? Thanks in advance.
There was a problem hiding this comment.
Something like this:
There was a problem hiding this comment.
Oh, destructor should have this in order to not terminate the working thread, right?
| auto coverArtFileInfo = mixxx::FileInfo(m_selectedCoverArtFilePath); | ||
| auto selectedCoverFileAccess = mixxx::FileAccess(coverArtFileInfo); | ||
| if (!selectedCoverFileAccess.isReadable()) { | ||
| if (Sandbox::askForAccess(&coverArtFileInfo)) { |
There was a problem hiding this comment.
This will issue a message box from this thread. I am not an apply guy. Is this necessary at all?
There was a problem hiding this comment.
Oh, I didn't notice that the askForAccess populates message box. That will cause a SEG fault according to my experience with this worker. So I just check if it is readable, if it is not readable emitting fail signal would work IMHO. What do you think?
| auto selectedCoverFileAccess = mixxx::FileAccess(coverArtFileInfo); | ||
| if (!selectedCoverFileAccess.isReadable()) { | ||
| if (Sandbox::askForAccess(&coverArtFileInfo)) { | ||
| selectedCoverFileAccess = mixxx::FileAccess(coverArtFileInfo); |
There was a problem hiding this comment.
We need also check for failure in the second path. if this code remains.
There was a problem hiding this comment.
So double check isReadable for both of the paths would work?
There was a problem hiding this comment.
We need the isReadable() == true state at least. I have no clue about the implications on macOS.
fatihemreyildiz
force-pushed
the
coverartworker
branch
from
f087465
to
388fe59
Compare
|
As we have discussed for macOS FileAccess needs to be tested. |
|
I now receive |
Could you please tell how to produce this? I tried many times and I've never had this before. |
As I mentioned earlier on #4887 , this is the basic of the cover art copy worker.
We need it for to have a cover art fetcher #4851 .
Technical details of this PR is, it only works for 'JPG' extension for now and the copying process is default. The rest detailed version of this worker will be improved on #4887 . Such as preferences and comparison page.
For now it looks like this: