Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows files/folders to be copied. #6014

Merged
merged 6 commits into from
Sep 18, 2017
Merged

Allows files/folders to be copied. #6014

merged 6 commits into from
Sep 18, 2017

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Aug 7, 2017

Adds a Copy action in the file action menu.
This also enables copy and move actions on selected files.

I know this is not how @jancborchardt designed it here, and that the files_clipboard app is being developed but hey, this works for me, so it might be worth a look. :)
Ref. #3206

Needs :

  • Tests
  • Copy action icon
  • Tests on federated shares, remote storage, ...
  • Tests for regressions on filepicker UI

The move feature has been moved at #6273

@mention-bot
Copy link

@tcitworld, thanks for your PR! By analyzing the history of the files in this pull request, we identified @noveens, @PVince81 and @butonic to be potential reviewers.

@@ -2000,10 +2072,94 @@
self.showFileBusyState($tr, false);
});
});
if (callback) {
callback();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dirty.

@MorrisJobke
Copy link
Member

cc @danxuliu

@tcitworld Could I ask you to split the "add copy" and the "move for selected files" into two PRs? Then we could work on them in parallel and one does not block the other feature.

@tcitworld
Copy link
Member Author

Yup, will do that.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! However I would really like an addition like that to only come in with the design as specified at #3206 (comment)

Could you adjust it to that? :)

@rullzer rullzer added this to the Nextcloud 13 milestone Aug 25, 2017
@tcitworld tcitworld mentioned this pull request Aug 27, 2017
@codecov
Copy link

codecov bot commented Aug 27, 2017

Codecov Report

Merging #6014 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master   #6014      +/-   ##
===========================================
- Coverage     53.43%   53.4%   -0.03%     
  Complexity    22545   22545              
===========================================
  Files          1408    1408              
  Lines         87210   87260      +50     
  Branches       1328    1340      +12     
===========================================
+ Hits          46601   46604       +3     
- Misses        40609   40656      +47
Impacted Files Coverage Δ Complexity Δ
core/js/oc-dialogs.js 0.43% <0%> (-0.03%) 0 <0> (ø)
apps/files/templates/list.php 0% <0%> (ø) 0 <0> (ø) ⬇️
core/js/files/client.js 82.86% <0%> (-4.64%) 0 <0> (ø)
core/js/jquery.ocdialog.js 1.53% <0%> (-0.04%) 0 <0> (ø)
lib/private/Server.php 84.37% <0%> (+0.12%) 123% <0%> (ø) ⬇️
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

@tcitworld
Copy link
Member Author

tcitworld commented Aug 27, 2017

Looks like this @jancborchardt
selection_102

@jancborchardt
Copy link
Member

@tcitworld nice! :) Was just mentioning it in the design lightning talk today. ;) Is the menu also changed to say »Move or Copy«? (Will test the whole flow later)

name: 'Move',
displayName: t('files', 'Move'),
name: 'CopyMove',
displayName: t('files', 'Copy or Move'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny one: First move, then copy. So »Move or copy« :) (with small _c_opy)

<span class="icon icon-external"></span>
<span><?php p($l->t('Copy'))?></span>
<span><?php p($l->t('Copy or Move'))?></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, »Move or copy« :)

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise

};

var copyCallback = function () {
console.log('copy callback');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover debug statement

@@ -47,6 +47,10 @@
</label>
<a class="name sort columntitle" data-sort="name"><span><?php p($l->t( 'Name' )); ?></span><span class="sort-indicator"></span></a>
<span id="selectedActionsList" class="selectedActions">
<a href="" class="copy-move">
<span class="icon icon-external"></span>
<span><?php p($l->t('Copy or Move'))?></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here as @jancborchardt said

@tcitworld tcitworld dismissed stale reviews from jancborchardt and icewind1991 August 28, 2017 08:57

Changed move and copy

@tcitworld
Copy link
Member Author

Rebased now that #6273 is merged.

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works great 👍

@jancborchardt
Copy link
Member

👍 from my side. What about the failing tests @tcitworld @MorrisJobke?

@MorrisJobke
Copy link
Member

👍 from my side. What about the failing tests @tcitworld @MorrisJobke?

Those are timeouts, that are solved in current master/stable12 and this is simply from an older version of them. In the future they should not happen at all (or at least not that often anymore).

@MorrisJobke
Copy link
Member

Tests
Tests on federated shares, remote storage, ...
Tests for regressions on filepicker UI

What about those pending items? @tcitworld Could you add at least some tests for this?

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
…older sizes

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld
Copy link
Member Author

I added tests and fixed a behaviour where failed copy actions were also counted as successful. Please review again.

@tcitworld tcitworld added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 15, 2017
@jancborchardt
Copy link
Member

@tcitworld looks like the commits should be squashed into one? :) Also, there’s some Drone failures

@danxuliu
Copy link
Member

there’s some Drone failures

The failure was in an acceptance test... that should not have failed (I have run the test in local and it worked fine). Restarting the build will hopefully make the failure to go away (although I will have to study the failure in detail anyway; I hope it does not appear too frequently in Drone until then :-S ).

@MorrisJobke MorrisJobke merged commit f9dc6c4 into master Sep 18, 2017
@MorrisJobke MorrisJobke deleted the add-copy-action branch September 18, 2017 12:19
@agoat
Copy link

agoat commented Sep 18, 2017

Nice work @tcitworld. Thanks.

@jancborchardt
Copy link
Member

Yup, great work @tcitworld :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. feature: files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants