One Level file '- Copy' pattern by schinagl · Pull Request #313 · microsoft/winfile

schinagl · 2022-03-20T09:09:14Z

Motivation

One wants to create a copy of a file/directory in the very same directory.
Dragging a file/directory onto the empty space in the file-pane does nothing currently.

Improvements

With this change one can drag an already existing file/directory into the empty space of the file pane and an item with the ' - Copy' is created.
E.g. File.txt -> 'File - Copy.txt'

Lets have a live look

Comments on the Code

I starts in wfdirsr.c: 1049, where normally a drop on the same directory was blocked.
Enabling this only for 'copy' (fShowSourceBitmaps == TRUE) brings us to wfcopy.c, which is extended once for files and once for directories.

Known Limitations

This PR intentionally only works in the same directory.
There is no Copy (n) algorithm implemented if applied multiple times on the same file.

Dependencies

This one lives on its own, but if the Symlink/Hardlink one #312 will make it to the maintrunk, I will update this PR with a ' - Hardlink', '- Symlink' one.
I already have an aggregated version of all my changes under the _hermann branch.

schinagl · 2022-04-30T05:05:59Z

Let me give you a use-case:
One wants to make a temporary copy of a file easily, because you would like to try out things and be able to revert quickly.

Then simply drag the file onto the empty space, or on the directory itself in the left pane, and have a file with the ' - Copy' Prefix.

malxau-msft · 2022-05-05T03:38:26Z

+               lstrcpy(szDest, szDestAlt);
+            } else {
+               // If one already used this '- Copy' postfix, bail out. Just one level.
+               ret = DE_SAMEFILE;


It looks like this happens for two cases, the one I don't understand above ( ! FUNC_COPY ), or where the file name has been changed so it's not the same file but the file name is in use. Would DE_RENAMREPLACE be better here?

If the -copy Postfix didn't exist (GetFileAttributes() ...) we create it
If it already exists we bail out, as we did before

It bails out, telling the user that it's the same file - but it's not the same file. It's attempting to copy "foo" to "foo - Copy", finds "foo - Copy", and bails out. The question is what to tell the user.

That's correct it says the file is the same, but instead the file with '- copy' postfix is there.
We could create a new message, but for me it was good enough to see the current message

Did you look at the message for DE_RENAMREPLACE? See

winfile/src/lang/suggest_en-US.db

Line 50 in 8c0d62b

"A file or directory by this name already exists. Within a directory, file and directory names must be unique." )

. We could have a new message, but this one seems to say the right thing already.

malxau-msft · 2022-05-05T04:08:08Z

   if (hwndHolder == lpds->hwndSource) {
-      if ((dwSelSink == (DWORD)-1) || SendMessage(hwndLB, LB_GETSEL, dwSelSink, 0L))
-         return TRUE;
+      if ((dwSelSink == (DWORD)-1) || SendMessage(hwndLB, LB_GETSEL, dwSelSink, 0L)) {


The second part of this test means that if a user Ctrl+drags a file over itself, it will be copied; but Ctrl+drag over any other file will do nothing. Is this intended?

Note that because this is checking the list box hwnd, dragging between two windows to the same directory have different behavior here: Ctrl+drag to a different file in the same directory but a second window will copy (adding the new suffix.)

It looks like this section can't interpret dragging over other files as copies without breaking the code below which is attempting to launch a program with a file as an argument.

I think this check used to be at the top to just short circuit logic. Note the "NormalMoveCopy" case comments that it's checking for the same directory, but really doesn't - it's checking for the same window, so the previous code would attempt the operation when crossing windows and fail later.

I'm wondering if this check should really look at fShowSourceBitmaps to no-op moves, but allow copies to continue; and the "NormalMoveCopy" case should also look at fShowSourceBitmaps to avoid the same-window check for copies, which this PR is aiming to support.

Investigating ....
<I set the other comments where I thought it is fine to 'resolved'. Hope this is fine. I am not sure what the policy is. So feel free to reopen>

Did you try this or just guessed from the code?

I just tried this.
If I drag a file over an .exe it does start the .exe with the file as command line argument, regardless if it is in the same window or in a different one.
If I drag a file on the empty space it does a copy with the '- Copy' postfix.

Does this resolve your concern?
Do you have a use-case where you observed that it did not work?

I tried it. I saw the same behavior for both cases you listed. The issue is what happens when files are dragged over each other (ie., not the empty space.) The case that seemed interesting to me is Ctrl+drag a file over a different file, which is a no-op in the same window, but will generate a copy if dragged to a different window. The behavior difference is really an artifact though of having slightly different pieces of code processing what is effectively the same case (by checking the window for equality), and I think removing the duplication will result in a more consistent experience.

My current summary on this thread so far is

Copy files over 'ops' (.exe/.bat/...)

Works as before

Copy no-op over no-ops

Code of this PR
same pane: nothing happens
different pane: copy is created with '- Copy' postfix

New_NoOp2NoOp.mp4

Previous Code
same pane: nothing happens
different pane: Box pops up that files are the same <the video grabber didn't capture this>

Old_NoOp2NoOp.mp4

So I don't see a change of behaviour, except for the '- copy' postfix.
I don't want to change the same-window check in this PR. I just wanted to create postfixes.

Did I miss something? Any breaking behaviour change?

malxau-msft · 2022-05-07T19:53:35Z

I pushed a commit of suggestions: malxau@66759bb

(Obviously I'm no more familiar with this code than you, so please review and let me know if I'm making a horrible mistake.)

The "major" thing here is in wfdirsrc.c. In this PR, it seemed like there was code duplication, and I wanted to explore a bit more about how we could have a single path. I think the changes in this suggestion are cleaner and more consistent, but note they introduce one behavior change: in this PR, copying to the same window short-circuits the confirmation dialog. In my suggested change, copying to the same directory follows the same path as everything else, which results in a confirmation dialog. I think this is the right thing to do (if a user didn't want confirmations, they can turn them off), but it's worth calling out.

For wfcopy.c, I'm a bit frustrated at having two paths to add "- Copy" but after digging into it I understand why this happened. I tried to handle the directory case in the same way as the file case, but it didn't really seem "better" - it has to reconstruct the target path (one component) in OPER_MKDIR, so it felt like both approaches end up duplicating logic. Note these suggestions change the error message, check if the path is valid after adding "- Copy", and more explicitly check for FUNC_COPY when doing these appends.

Please let me know what you think.

schinagl · 2022-05-08T20:07:39Z

Thx for the input. I am taking things piece by piece from your branch in an extra commit and comment it there.
Some things are easy, somethings need some time to think. Will mark he last commit , so that you know I am done

In my next version for sym/hardlinks Winfile also supports '-hardlink' and '-symlink' postfixes. So as soon as hard/sylinks come in, I will have to revert things, because there will be FUNC_HARD, FUNC_LINK, FUNC_JUNC, and then the switch statement ist back This was the reason why in all those places the odd looking switch() was there Anyhow for now lets take it.

schinagl · 2022-05-08T20:15:42Z

This was an easy one. The code is shorter, but .. :-)
In my next version for sym/hardlinks Winfile also supports '-hardlink' and '-symlink' postfixes.
So as soon as hard/sylinks come in, I will have to revert things, because there will be FUNC_HARD, FUNC_LINK, FUNC_JUNC, and then the switch statement ist back
This was the reason why in all those places the odd looking switch() was there

Anyhow for now lets take it.

Variable declaration in C style, needed for branch targeting W2K & WXP Change message to more a appropriate

schinagl · 2022-05-08T20:20:54Z

Also an easy one
Variable declaration in C style, needed for branch targeting W2K & WXP
Change message to more a appropriate

The intention to reach DirMoveCopy: is the same Fine, thx

schinagl · 2022-05-08T20:42:41Z

The flow of lines is untwisted much better.
The intention to reach DirMoveCopy: is the same
Fine, thx

bDoMoveRename is really used that late, and since we have the check for FUNC_COPY this is more logical. // if (bSameFile && pCopyInfo->dwFunc == FUNC_COPY Checking the path is also good for all pathes // IsInvalidPath ... I was a bit struggling with the removal of the switch, because as mentiond earlier there might be FUNC_HARD, FUNC_LINK ... and so on Anyhow the 'switch' will then return, also the 'if (bSameFile ....' will become a little more clumsy, because all affected FUNC_ are mentioned I guess this was the last commit. Thx for digging into and providing leaner sections!

schinagl · 2022-05-08T21:01:21Z

Took over this one too. Took me the longest to check.
bDoMoveRename is really used that late, and since we have the check for FUNC_COPY this is more logical. // if (bSameFile && pCopyInfo->dwFunc == FUNC_COPY
Checking the path is also good for all pathes // IsInvalidPath ...

I was a bit struggling with the removal of the switch, because as mentiond earlier there might be FUNC_HARD, FUNC_LINK ... and so on
Anyhow the 'switch' will then return, also the 'if (bSameFile ....' will become a little more clumsy, because all affected FUNC_ are mentioned

I guess this was the last commit. Thx for digging into and providing leaner sections!

schinagl · 2022-05-09T05:36:32Z

Something gone wrong with the/my merge. It throws Access denied on -Copy Postfix

malxau-msft · 2022-05-09T05:49:52Z

It's missing the initial population of szDestAlt :(

schinagl · 2022-05-09T05:53:36Z

Yes. See #322. Found it when preparing

One Level file '- Copy' pattern

7993f66

schinagl force-pushed the one_level_-_copy_behaviour branch from 26a1b63 to 7993f66 Compare April 11, 2022 20:22

malxau-msft reviewed May 5, 2022

View reviewed changes

Comment thread src/wfcopy.c Outdated

malxau-msft reviewed May 5, 2022

View reviewed changes

schinagl and others added 3 commits May 5, 2022 07:45

Code review 01

d4925c4

Remove unneccessary code 01 - Code review

75e8bdc

Add error handling to the default clause

81f01dc

Also an easy one

6098bd5

Variable declaration in C style, needed for branch targeting W2K & WXP Change message to more a appropriate

The flow of lines is untwisted much better.

c4542e4

The intention to reach DirMoveCopy: is the same Fine, thx

malxau-msft approved these changes May 9, 2022

View reviewed changes

malxau-msft merged commit ffa2c97 into microsoft:master May 9, 2022

schinagl deleted the one_level_-_copy_behaviour branch May 9, 2022 05:04

schinagl mentioned this pull request May 9, 2022

Creation of Symbolic Links, Hardlinks and Junctions #312

Merged

schinagl mentioned this pull request Nov 11, 2022

Symbolic Link, Hardlink and Junction creation is available via menu #352

Merged

Uh oh!

Conversation

schinagl commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Improvements

Comments on the Code

Known Limitations

Dependencies

Uh oh!

schinagl commented Apr 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

malxau-msft May 5, 2022

Choose a reason for hiding this comment

Uh oh!

schinagl May 5, 2022

Choose a reason for hiding this comment

Uh oh!

malxau-msft May 5, 2022

Choose a reason for hiding this comment

Uh oh!

schinagl May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malxau-msft May 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

malxau-msft May 5, 2022

Choose a reason for hiding this comment

Uh oh!

schinagl May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schinagl May 5, 2022

Choose a reason for hiding this comment

Uh oh!

malxau-msft May 5, 2022

Choose a reason for hiding this comment

Uh oh!

schinagl May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Copy files over 'ops' (.exe/.bat/...)

Copy no-op over no-ops

Uh oh!

malxau-msft commented May 7, 2022

Uh oh!

schinagl commented May 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schinagl commented May 8, 2022

Uh oh!

schinagl commented May 8, 2022

Uh oh!

schinagl commented May 8, 2022

Uh oh!

schinagl commented May 8, 2022

Uh oh!

schinagl commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malxau-msft commented May 9, 2022

Uh oh!

schinagl commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

schinagl commented Mar 20, 2022 •

edited

Loading

schinagl May 5, 2022 •

edited

Loading

schinagl May 5, 2022 •

edited

Loading

schinagl May 6, 2022 •

edited

Loading

schinagl commented May 8, 2022 •

edited

Loading

schinagl commented May 9, 2022 •

edited

Loading

schinagl commented May 9, 2022 •

edited

Loading