Move/copy file picker: Remeber last destination and start in current folder #12381

rummatee · 2018-11-09T15:19:52Z

This my first time contributing to Nextcloud and using Pull Requests, so please tell me, if I did something wrong.
This Pull Request intends to solve Issue #11314 .
I changed the signature of the filepicker, but made sure that the behavior doesn't change in case it gets called without the additional argument.

rummatee · 2018-12-20T14:00:46Z

please review @jancborchardt

jancborchardt · 2018-12-20T21:27:20Z

Aaaalright @rummatee, sorry for the belated reply but welcome to Nextcloud! 👋 🎉

Will get right to reviewing, also cc other @nextcloud/designers, and this definitely also needs a JS review from @nextcloud/javascript.

jancborchardt · 2018-12-20T21:45:52Z

@rummatee also, could you rebase onto latest master, as there were some changes? Let me know if you have questions how to do that – I generally also search and find the answer on StackOverflow 😅

jancborchardt · 2018-12-20T21:52:03Z

Hmm, just tested it and it doesn’t seem to work? Still starts in the root folder every time. Do I need to compile anything? :)

rummatee · 2018-12-20T22:08:01Z

@rummatee also, could you rebase onto latest master, as there were some changes? Let me know if you have questions how to do that – I generally also search and find the answer on StackOverflow sweat_smile

indeed I would appreciate some help on this. I see that there is a button to resolve conflicts, but when I do that, it shows me a warning that I am committing to master and I was afraid to confirm that.

Hmm, just tested it and it doesn’t seem to work? Still starts in the root folder every time. Do I need to compile anything? :)

I don't think it should require to compile anything. I just tested it again and for me it works. I created a folder called "test" and a file in this folder. When I want to move this file the dialog starts in folder "test".

jancborchardt · 2018-12-21T00:42:11Z

indeed I would appreciate some help on this. I see that there is a button to resolve conflicts, but when I do that, it shows me a warning that I am committing to master and I was afraid to confirm that.

It’s fine – that is referring to your master branch. It’s not possible for anyone to commit directly to master or any stable branches as we have branch protection enabled. :)

kesselb · 2018-12-22T15:28:59Z

A: Select one file, use the three dot menu on the right, pick copy or move ✅

B: Select two ore more files, use the top three dot menu, pick copy or move ❌

A is working fine for me but B does not work.

kesselb

Great improvement 👍

core/js/oc-dialogs.js

rummatee · 2018-12-23T13:51:04Z

A is working fine for me but B does not work.

okay thank you for clarifying that there are 2 different menus that need this feature. The new commit should add it also to the multiselect menu.

jancborchardt · 2018-12-23T14:34:57Z

Thanks @rummatee!

We definitely need a code review on this from someone in @nextcloud/javascript :)

kesselb · 2018-12-23T15:55:23Z

Select two or more files, use top three dot menu and pick "move or copy". Please note the two buttons "move" and "copy".

When you go a directory higher or lower the button label is changed like below even when you are in the same folder again.

I think the button labels should be the same @jancborchardt

jancborchardt · 2018-12-23T18:06:38Z

I think the button labels should be the same

Yes, true. Is it possible to make it so that in the folder where the files are right now, they are simply "Copy" and "Move"? Ideally they should also be disabled there, and a tooltip (also visible on click) on the buttons should say "This folder is the current place of the files."

rummatee · 2018-12-23T19:37:51Z

I think the button labels should be the same

Yes, true. Is it possible to make it so that in the folder where the files are right now, they are simply "Copy" and "Move"? Ideally they should also be disabled there, and a tooltip (also visible on click) on the buttons should say "This folder is the current place of the files."

Well that would be related to issue #11313 #9931 and PR #10825 so I am not completely sure how these would interact. Of course it would be possible to change the label or grey out the buttons, but maybe it is better to leave that out of this PR to avoid conflicts with other PRs?

jancborchardt · 2018-12-23T23:39:59Z

maybe it is better to leave that out of this PR to avoid conflicts with other PRs?

Good point, yes let’s do it separately!

Still waiting for a code review from @nextcloud/javascript :) @juliushaertl @danxuliu @kevgathuku @ChristophWurst anyone?

ChristophWurst

:)

apps/files/js/filelist.js

core/js/oc-dialogs.js

ChristophWurst · 2019-01-08T07:00:13Z

apps/files/js/filelist.js


 			var actions = this.isSelectedMovable() ? OC.dialogs.FILEPICKER_TYPE_COPY_MOVE : OC.dialogs.FILEPICKER_TYPE_COPY;
+			var dialogDir = self.getCurrentDirectory();
+			if (typeof self.dirInfo.dirLastCopiedTo !== undefined) {


Suggested change

if (typeof self.dirInfo.dirLastCopiedTo !== undefined) {

if (typeof self.dirInfo.dirLastCopiedTo !== 'undefined') {

ChristophWurst

Code looks good!

jancborchardt · 2019-01-09T01:53:05Z

Here too @rullzer @MorrisJobke what about the Drone failure? Is it just unrelated?

MorrisJobke · 2019-01-09T08:54:13Z

The failures are fixed on master already (issue with the PHP 7.0 to 7.3 compatibility). Either rebase or we can just assume that they will work and this is then ready for review and merge.

MorrisJobke · 2019-01-09T17:48:34Z

@rummatee It seems that your force push closed this PR 🙈 Could you open a new one?

rummatee · 2019-01-09T17:49:50Z

yeah, I didn't see the rebase conflicts and accidentally pushed to remove all my commits

rummatee · 2019-01-09T17:56:51Z

hmm does it show my commits again when I click on reopen?

rummatee · 2019-01-09T18:13:08Z

@rummatee It seems that your force push closed this PR see_no_evil Could you open a new one?

now that I reopened it, it doesn't seem to run the checks. Do I have to create a new PR?

MorrisJobke · 2019-01-09T18:20:21Z

now that I reopened it, it doesn't seem to run the checks. Do I have to create a new PR?

Or rebase again 🙈

core/js/oc-dialogs.js

juliusknorr

Small nitpick. Otherwise works as expected and the code looks fine 👍

Signed-off-by: Florian Schunk <[email protected]>

juliusknorr · 2019-01-18T13:32:40Z

Signed-off messages are fine. DCO bot seems to have some issues. 😉

ChristophWurst added this to the Nextcloud 16 milestone Nov 12, 2018

ChristophWurst added enhancement 3. to review Waiting for reviews labels Nov 12, 2018

jancborchardt requested review from blizzz, juliusknorr, kesselb, kevgathuku, skjnldsv and violoncelloCH December 20, 2018 21:27

kesselb reviewed Dec 22, 2018

View reviewed changes

core/js/oc-dialogs.js Show resolved Hide resolved

rummatee force-pushed the master branch from c0247f3 to 0e45ede Compare December 23, 2018 13:44

kesselb approved these changes Dec 23, 2018

View reviewed changes

rummatee force-pushed the master branch 2 times, most recently from 02dda9c to 00032b2 Compare January 5, 2019 17:40

ChristophWurst requested changes Jan 7, 2019

View reviewed changes

apps/files/js/filelist.js Outdated Show resolved Hide resolved

core/js/oc-dialogs.js Show resolved Hide resolved

core/js/oc-dialogs.js Outdated Show resolved Hide resolved

rummatee force-pushed the master branch from 236b3ed to 6eabf0b Compare January 7, 2019 21:23

ChristophWurst requested changes Jan 8, 2019

View reviewed changes

rummatee force-pushed the master branch 3 times, most recently from 17d9b9a to 6aa505b Compare January 8, 2019 14:30

ChristophWurst approved these changes Jan 8, 2019

View reviewed changes

rummatee force-pushed the master branch from 6aa505b to 4babce9 Compare January 9, 2019 10:27

rummatee closed this Jan 9, 2019

rummatee force-pushed the master branch from 4babce9 to 62b4f96 Compare January 9, 2019 17:42

rummatee reopened this Jan 9, 2019

rummatee force-pushed the master branch 2 times, most recently from db81573 to cc8d539 Compare January 10, 2019 12:31

juliusknorr reviewed Jan 14, 2019

View reviewed changes

core/js/oc-dialogs.js Outdated Show resolved Hide resolved

juliusknorr requested changes Jan 14, 2019

View reviewed changes

rummatee added 4 commits January 16, 2019 22:54

copy Dialog starts in current directory

528964e

Signed-off-by: Florian Schunk <[email protected]>

remember last copied to directory

37270fc

Signed-off-by: Florian Schunk <[email protected]>

also remember folder for multiselect actions

ecb9364

Signed-off-by: Florian Schunk <[email protected]>

fix testing for undefined

332b4ae

Signed-off-by: Florian Schunk <[email protected]>

rummatee force-pushed the master branch from cc8d539 to 332b4ae Compare January 16, 2019 21:54

juliusknorr approved these changes Jan 18, 2019

View reviewed changes

juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 18, 2019

MorrisJobke merged commit 743323e into nextcloud:master Jan 18, 2019

MorrisJobke mentioned this pull request Jan 18, 2019

Move/copy file picker: Should start out in current folder, not root #11314

Closed

	if (typeof self.dirInfo.dirLastCopiedTo !== undefined) {
	if (typeof self.dirInfo.dirLastCopiedTo !== 'undefined') {

Uh oh!

Move/copy file picker: Remeber last destination and start in current folder #12381

Move/copy file picker: Remeber last destination and start in current folder #12381

Uh oh!

Conversation

rummatee commented Nov 9, 2018

Uh oh!

rummatee commented Dec 20, 2018

Uh oh!

jancborchardt commented Dec 20, 2018

Uh oh!

jancborchardt commented Dec 20, 2018

Uh oh!

jancborchardt commented Dec 20, 2018

Uh oh!

rummatee commented Dec 20, 2018

Uh oh!

jancborchardt commented Dec 21, 2018

Uh oh!

kesselb commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kesselb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rummatee commented Dec 23, 2018

Uh oh!

jancborchardt commented Dec 23, 2018

Uh oh!

kesselb commented Dec 23, 2018

Uh oh!

jancborchardt commented Dec 23, 2018

Uh oh!

rummatee commented Dec 23, 2018

Uh oh!

jancborchardt commented Dec 23, 2018

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChristophWurst Jan 8, 2019

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

jancborchardt commented Jan 9, 2019

Uh oh!

MorrisJobke commented Jan 9, 2019

Uh oh!

MorrisJobke commented Jan 9, 2019

Uh oh!

rummatee commented Jan 9, 2019

Uh oh!

rummatee commented Jan 9, 2019

Uh oh!

rummatee commented Jan 9, 2019

Uh oh!

MorrisJobke commented Jan 9, 2019

Uh oh!

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kesselb commented Dec 22, 2018 •

edited

Loading