microsoft / winfile

Original Windows File Manager (winfile) with enhancements
MIT License
6.75k stars 698 forks source link

Have WFSetAttr refresh tree #414

Open malxau-msft opened 6 months ago

malxau-msft commented 6 months ago

This is a fairly isolated approach to update the tree in response to changes to hidden/system attributes for directories. (I'm not sure if it's the best approach, but sending it out for comment anyway.)

Scenarios this change addresses

  1. Copying a hidden directory. At the end of the copy, this will mark the directory as hidden, and this change ensures that the hidden directory will not be listed in the left pane if hidden files are not being displayed.
  2. Updating the right pane for file attribute changes made outside of winfile. This part is the winfile.h change.
  3. Updating the left pane for directory attribute changes made when selecting multiple directories.

Scenarios not addressed by this change

  1. Updating the attributes of a single directory in winfile. This currently uses the shell32 dialog, which means it does not trigger winfile's internal change notification system.
  2. Updating directory attributes outside of winfile. Winfile does not monitor for changes of parent components that are visible in the left pane but not the right pane. Even if it did, the only notification it currently receives is "something changed", and refreshing the tree any time something changes on the drive would not be a good experience.

Next steps

As mentioned above, it's not feasible to monitor changes for the entire drive and refresh, which is implied by using FindFirstChangeNotification. Monitoring for external changes requires something more efficient so winfile can quickly discard irrelevant changes. One fairly incremental change is to use ReadDirectoryChangesW which indicates exactly what changed. I think this can be broken down into three parts:

  1. Replace current use of FindFirstChangeNotification with ReadDirectoryChangesW. Normally this would be hard, but winfile's ChangeFileSystem function is almost a direct translation of the output of ReadDirectoryChangesW. Most of the changes here would be deleting code used to work around the limitations of FindFirstChangeNotification - the FSC_REFRESH logic, the timer, etc.
  2. Add monitoring for the subtree of drive roots. This part is more involved, mainly because monitoring a whole drive can generate a lot of changes so it's important to be efficient about it. This might imply things like not monitoring the same tree twice, which in turn implies reference counting trees subject to monitoring, etc. It looks like now there's a relatively new ReadDirectoryChangesExW which offers another cute optimization by providing file attributes as part of the notification (so changes to non-directories can be immediately ignored with no processing.)
  3. Remove the current explicit calls to ChangeFileSystem and let this be driven through external notifications. Currently winfile is a bit split-brained in that it receives both internal and external notifications, and by applying a delay to external notifications, ensures the internal ones take precedence. But if internal and external are happening at the same time without a delay and performing explicit changes, it'd be relatively easy to create race conditions that leave the UI out of sync with the file system. Previously the internal changes were necessary to create a responsive UI when external changes can only indicate "something changed" and require a rescan, but with ReadDirectoryChangesW the UI could be fairly responsive without needing explicit internal change notifications.
schinagl commented 6 months ago

A few things

schinagl commented 5 months ago

dwFilterAttribs could be declared after wfcomman.c:309 and it is still C89

malxau-msft commented 4 months ago
  • I suggest to extend the case statement in wfcomman.c:331 instead of adding an extra if statement in wfcomman.c:295.

Thanks. I think the reason I couldn't do this is because there's already an FSC_ATTRIBUTES case statement which is shared with six other operations. The logic could be moved into the case statement, but I'd still need either an extra if to ensure the new code only runs for FSC_ATTRIBUTES, duplicate the code that currently runs in FSC_ATTRIBUTES, or use goto. None of them seemed to fit well.

schinagl commented 4 months ago

Then move it in to case FSC_ATTRIBUTES: and use the if.

Someone who reverse engineers this code is happy to find all stuff related to FSC_ATTRIBUTES in one place and not spread around.

Furthermore you have not commented on the 'dwFilterAttribs could be declared after wfcomman.c:309'.

Respecting the principle of locality is a commonly accepted pattern and the above would still be C89 if you want to go to your nmake based vs2005 build for w2k