Merge lp:~nikwen/ubuntu-filemanager-app/cancel-deletion-fix into lp:ubuntu-filemanager-app

[Niklas Wenzel]

When cancelling folder deletion, future deletions will work now

When cancelling folder deletion, future deletions will work now

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:355
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/439/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-vivid/531
        deb: http://91.189.93.70:8080/job/generic-mediumtests-vivid/531/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/61

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/439/rebuild

review: Approve (continuous-integration)

[Carlos Jose Mazieri (carlos-mazieri)]

I am not sure it solves the problem.

In the current code processAction() is called anyway (and it needs to be), it sets m_busy to true it there is more actions to perform or false if there is no action to perform.

In fact processAction() is called to when an action starts and when an action finishes to start another action if the queue is not empty.

Can you describe how did you find it would the problem?

There is a compiler option DEBUG_MESSAGES that activates verbose in the plugin, it may helps.

[lp:~nikwen/ubuntu-filemanager-app/cancel-deletion-fix]

356. By Niklas Wenzel on 2014-12-29

Call endActionEntry() instead of invoking processAction() directly

[Niklas Wenzel (nikwen)]

Thank you for looking into this, Carlos. :)

You're right. processAction() needs to be called and it is when an action finishes properly. However, when you cancel an action, it isn't.
To verify this, you can add the following line to the beginning of the processAction() method:

qDebug() << "processAction() called";

When cancelling a deletion, the log will then still look like this, which shows that processAction() isn't called:

[...]
qml: On progress 1090 5667 19
qml: On progress 1095 5667 19
qml: On progress 1100 5667 19
qml: Cancelling file progress action

Looking at processActionEntry() you'll see that endActionEntry(), which later calls processAction(), is only called if m_cancelCurrentAction is false. Since there is no else in trunk, processAction() won't be called if m_cancelCurrentAction is true. This can be fixed by adding an appropriate else clause.

    if (!m_cancelCurrentAction)
    {
        switch(curEntry->type)
        {
           case ActionRemove:
           case ActionHardMoveRemove:
                removeEntry(curEntry);
                endActionEntry();
                break;
           case ActionCopy:
           case ActionHardMoveCopy:
                processCopyEntry(); // specially: this is a slot
                break;
          case ActionMove:
                moveEntry(curEntry);
                endActionEntry();
                break;
          default:
                break;
        }
    }

I did, however, notice now that it would be better to call endActionEntry() in the else statement as well because that method already contains code for handling a cancelled action (it's pretty much the same which I added previously). So thank you for your comment. :)

If you need any further information, feel free to ask. ;)

[lp:~nikwen/ubuntu-filemanager-app/cancel-deletion-fix]

357. By Niklas Wenzel on 2014-12-29

Revert changes to pot files

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:357
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/440/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-vivid/535
        deb: http://91.189.93.70:8080/job/generic-mediumtests-vivid/535/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/62

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/440/rebuild

review: Approve (continuous-integration)

[Carlos Jose Mazieri (carlos-mazieri)]

Hi Niklas.

First I did not understand where you were put the else statement, I thought it was inside the function FileSystemAction::endActionEntry(), I am sorry because I did not lookt at it right.

You may be right, an else statement is necessary in the funtion FileSystemAction::processActionEntry().

The test case I mentioned before worked, I will review this test case to understand why it worked.

I should approve this, please wait some time, I cannot test this right now, will do later today.

Many Thanks.

[Niklas Wenzel (nikwen)]

Hi Carlos,

Well, no problem. Your comment was actually helpful since it helped my find out that I should be calling another function in the else clause.

Take the time you need. ;)

[Carlos Jose Mazieri (carlos-mazieri)]

Hi Niklas,

I tried to make my test case fail but it did not, I tried to reproduce the scenario you described and I should have failed in some point.

I agree that "if" should have an "else", I ran all the tests with your change and all passed.
So I am OK with it.

I added another test case using lots of files instead of one directory with a deep tree, but in test we delete all files using selection (not single remove in the parent directory) then we cancel, this test failed and it seems had a problem in the external watcher. I will keep looking at this.

Thank you very much.

review: Approve

[Niklas Wenzel (nikwen)]

Hi Carlos,

Thank you for approving this. Would you mind doing a top-approval as well so that it gets merged?

[Niklas Wenzel (nikwen)]

Thanks for merging. :)

Am Dienstag, 30. Dezember 2014 schrieb :
> The proposal to merge
lp:~nikwen/ubuntu-filemanager-app/cancel-deletion-fix into
lp:ubuntu-filemanager-app has been updated.
>
> Status: Approved => Merged
>
> For more details, see:
>
https://code.launchpad.net/~nikwen/ubuntu-filemanager-app/cancel-deletion-fix/+merge/245402
> --
> You are the owner of
lp:~nikwen/ubuntu-filemanager-app/cancel-deletion-fix.
>

=== modified file 'src/plugin/folderlistmodel/filesystemaction.cpp'
--- src/plugin/folderlistmodel/filesystemaction.cpp	2014-06-17 20:42:35 +0000
+++ src/plugin/folderlistmodel/filesystemaction.cpp	2014-12-29 11:32:21 +0000
@@ -422,6 +422,11 @@
                 break;
         }
     }
+    else
+    {
+        //Needed to set m_busy to false. Otherwise, further actions will never be executed.
+        endActionEntry();
+    }
 }
 
 //===============================================================================================