Differential D19953

Fix progress info for CreateJob
ClosedPublic
Actions

Authored by rthomsen on Mar 21 2019, 7:30 PM.

Details

Reviewers
elvisangelaccio
Commits
R36:ce2f7f6690be: Fix progress info for CreateJob
Summary

Progress info is currently broken when creating archives using CreateJob. This happens for example when the user creates an archive through the KFileItemAction in Dolphin.

See bug 382599.

This happens because CreateJob uses a nested AddJob and the percent signal of the AddJob is not forwarded to CreateJob. It was necessary to create a new signal PercentFromNestedJob in the Job class because connecting to KJob::percent did not work. I suspect this is caused by KJob also having a public member function percent and Qt confusing the signal and the member function. If it is possible to somehow force Qt to connect to the signal, then the PercentFromNestedJob is not necessary.

Test Plan

Create a sufficiently large archive (e.g. rar archive) from Dolphin using the right-click menu. Observe that progress is correctly shown in system tray.

Alternatively use: ark --add-to <archive name> <files to add>

Diff Detail

Repository
R36 Ark
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rthomsen created this revision.Mar 21 2019, 7:30 PM
Restricted Application added subscribers: Ark, kde-utils-devel. · View Herald TranscriptMar 21 2019, 7:30 PM
rthomsen requested review of this revision.Mar 21 2019, 7:30 PM
elvisangelaccio added a comment.Mar 24 2019, 3:31 PM

If it is possible to somehow force Qt to connect to the signal, then the PercentFromNestedJob is not necessary.

Have you tried with the old-style connect?

This

connect(m_addJob, SIGNAL(percent(KJob*,ulong)), this, SIGNAL(percent(KJob*,ulong)));

compiles for me, but I haven't tested it.

rthomsen added a comment.Mar 24 2019, 4:48 PM
In D19953#437295, @elvisangelaccio wrote:

If it is possible to somehow force Qt to connect to the signal, then the PercentFromNestedJob is not necessary.

Have you tried with the old-style connect?

This

connect(m_addJob, SIGNAL(percent(KJob*,ulong)), this, SIGNAL(percent(KJob*,ulong)));

compiles for me, but I haven't tested it.

Yes, I did try with the old connect but the slot is never called. I imagine it connects with the member function instead.

rthomsen updated this revision to Diff 54787.EditedMar 25 2019, 5:13 PM

I managed to avoid adding another signal by using qOverload on the connect statement. This requires C++14 support.

elvisangelaccio added inline comments.Mar 25 2019, 8:50 PM
kerfuffle/jobs.cpp
484

Please use QOverload<KJob*,unsigned long>::of(&KJob::percent) which does not require C++ 14 ;)

rthomsen updated this revision to Diff 54828.Mar 26 2019, 5:37 AM

Use QOverload::of() so we dont require C++14.

rthomsen marked an inline comment as done.Mar 26 2019, 5:38 AM
elvisangelaccio accepted this revision.Mar 26 2019, 8:42 PM
This revision is now accepted and ready to land.Mar 26 2019, 8:42 PM
elvisangelaccio added a comment.Mar 26 2019, 8:42 PM

Please update the commit message before pushing, it's still referring to PercentFromNestedJob

Closed by commit R36:ce2f7f6690be: Fix progress info for CreateJob (authored by rthomsen). · Explain WhyMar 27 2019, 7:19 PM
This revision was automatically updated to reflect the committed changes.
rthomsen mentioned this in R36:ce2f7f6690be: Fix progress info for CreateJob.

Revision Contents
Changeset List

PathPackages
Mkerfuffle/jobs.cpp (3 lines)

Diff 54952

View Options

kerfuffle/jobs.cpp

Loading...