Differential D1394

Possibly fixed krarc writing mode
ClosedPublic
Actions

Authored by martinkostolny on Apr 12 2016, 5:25 PM.

Details

Reviewers
asensi
gengisdave
Group Reviewers
Krusader
Maniphest Tasks
T1930: Krarc with Write Support freezes
Summary

Removing the whole copy function is a daring step I know :). Consider this proposition as a question if we can even consider to removing copy function in order to fix the problem. Because write mode started working after this change.

When an implemented KIO::SlaveBase function is missing or calls error(...), KIO tries to call another one to achieve what was needed. In our case - reading a file is firstly tried with "copy" function and if it fails, KIO tries to read the file using "get" method. Accidentally this was exactly the scenario when write mode was disabled. Copy function ended with error(...) call so "get" was performed instead and file was successfully opened. When write mode was enabled copy function didn't end up with error, was successfully performed. But file was not opened for some case and "get" function was not called.

EDIT: Diff updated according Davide's guidance. Patch works as expected.

Test Plan

Write mode enabled:

  • reading files (opening them in editor), copying them outside
  • writing to files, overwriting from outside, copying files to archive, creating a directory

Write mode disabled:

  • reading files (opening them in editor), copying them outside

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
martinkostolny updated this revision to Diff 3301.Apr 12 2016, 5:25 PM
martinkostolny retitled this revision from to Possibly fixed krarc writing mode.
martinkostolny updated this object.
martinkostolny edited the test plan for this revision. (Show Details)
martinkostolny added a reviewer: Krusader.
martinkostolny set the repository for this revision to R167 Krusader.
martinkostolny added a project: Krusader.
martinkostolny added a task: T1930: Krarc with Write Support freezes.
gengisdave added a subscriber: gengisdave.EditedApr 12 2016, 7:50 PM

Once upon a time... I have broken krArc :(

You don't need to remove the put method, just uncomment to line 764 and change CMD_COPY with 74, according to https://quickgit.kde.org/?p=kio.git&a=blob&h=a8ddeba2e4467aecfc4d809f557c34daec3a954f&hb=4a51dd6ee5ca902119ddc2c4481d63ee0ca1a58f&f=src%2Fcore%2Fcommands_p.h. When copy fails, it automatically invokes get.

martinkostolny updated this revision to Diff 3303.Apr 12 2016, 9:05 PM
martinkostolny updated this object.
martinkostolny added a comment.Apr 12 2016, 9:08 PM

Thanks a lot, Davide! It works!

Doing more tests I realized that zip files open with zip:/ protocol (instead of krarc:/) so we are still unable to write into them but that is another issue and should probably be addressed in different commit.

gengisdave accepted this revision.Apr 13 2016, 5:46 AM
gengisdave added a reviewer: gengisdave.

Ok, remove the comment, I've added it as a memo for the future https://quickgit.kde.org/?p=krusader.git&a=commit&h=97cc44da126ddf67e561a0449756f8ef71b07df5.

This revision is now accepted and ready to land.Apr 13 2016, 5:46 AM
asensi accepted this revision.EditedApr 13 2016, 8:28 PM
asensi added a reviewer: asensi.
asensi added a subscriber: asensi.

Ok!

On a related issue, I am using some new code to perform more complete and intuitive traces of the execution of the krarc kioslave, so the traces include when the execution of a function starts, and (automatically) when it's going to finish. That is a screenshot of a comparison between two traces:

  • On the left part of the screenshot it can be seen:
    • The trace of the execution of the krarc kioslave when: the D1394 patch is not applied, the "enable write support" option is disabled, the user enters a 7z file ("/home/user/compressed_file.7z") and copies one file ("é.txt") outside. Note: The name of the file of the trace is "krdebug-unpatched-noWriteSupport" in the screenshot.
  • On the right part of the screenshot it can be seen:
    • The same case as before, but the "enable write support" option is enabled, and so the name of the file of the trace is "krdebug-unpatched-writeSupport" in the screenshot.

I've also compared the traces of the execution of the krarc kioslave when the D1394 patch is applied and the same operations are done; then the traces are the same whether if the "enable write support" option is enabled or not. After all, I also accept the D1394 patch :-)

martinkostolny closed this revision.EditedApr 14 2016, 10:39 PM

Davide, thanks a lot for your guidance!
Toni, cool, thanks! I'll definitely check out the code for traces you mentioned.

Code shipped.

Revision Contents
Changeset List

PathPackages
MkrArc/krarc.cpp (4 lines)

Diff 3303

View Options

krArc/krarc.cpp

Loading...