NotepadNext icon indicating copy to clipboard operation
NotepadNext copied to clipboard

Context Menu operations on Folder as Workspace tree

Open SimLV opened this issue 9 months ago • 12 comments

For now:

  • New Folder (for folders and root)
  • Delete
  • Rename
  • Save here (save active document under a new name)

SimLV avatar May 07 '24 18:05 SimLV

I'll take a crack at this and push any updates. I think I might have been confused a bit and told you wrong in places. I guess I was thinking the "File List" window...which means there was always an editor object associated with it (and thus easier to use the window). But realize now for the Folder as Workspace it is more direct file system modification potentially.

dail8859 avatar May 08 '24 21:05 dail8859

I pushed updates to this branch. I made a quick first round of cleanups...hopefully I didnt break anything that was working previously.

Some things to note:

  • I removed the permeant deletion for now. Think its better to just give the user an option move it to the trash. (if someone really wants it maybe it is a configuration later)
  • I understand the intent of closeByPath()...the difficult part is trying to half use the FaW as an explorer window that also keeps the opened files in sync (renamed, deleted, etc)...which is part of the reason why I didn't tackle this in the first place :) Notepad++ doesn't provide those file level operations so it never has to worry about that. For example: image
  • It might be better to leverage the EditorManager to future proof some things when finding an editor by the file path, because in the future if there are multipe windows it is resposinble for coordinating the editors between them. Plus if an editor is opened multiple times etc.

Sorry...late night brain dump :)

dail8859 avatar May 09 '24 03:05 dail8859

  • I was searching for permanent deletion solution like in file explorers (Option+Delete means permanent delete) but this needs subclassing of QTreeView.
  • "Eat your own dogfood". At least minor file manipulations (new/rename/delete) are useful for me.
  • I am working+thinking on "Run..." stuff but this is more complex

Do you need some other rework for this branch from me for now?

SimLV avatar May 09 '24 05:05 SimLV

I was searching for permanent deletion solution like in file explorers (Option+Delete means permanent delete) but this needs subclassing of QTreeView.

Yeah I think if something isn't obvious it is good to err on the side of caution. The user needs to accept the prompt anyways so best to move it to the trash. Again this could be an app setting or something in the future but unless there is a real "need" I don't think it is needed. If you need to delete something "permanently" I think it would be best not to use a text editor's file management window :)

"Eat your own dogfood". At least minor file manipulations (new/rename/delete) are useful for me.

Agreed. They are very nice and normally 'expected' features if you are going to be showing a list of files.

I am working+thinking on "Run..." stuff but this is more complex

I'm not saying this is needed for now. I was just showing an example of what Notepad++ does. This stuff can always be added in later if there is a real use-case for it.

Do you need some other rework for this branch from me for now?

No, I don't believe so currently. I need to find some more time to just sit down with it and work through some of this.

dail8859 avatar May 09 '24 11:05 dail8859

So I had some time to step back and think about this. There is definitely more complexity that has to be handled to implement it correctly.

If you rename a folder, now you have to check every file in that folder as a possible candidate that is opened for editing because it's file path could have changed.

Same with deleting a folder. You now have to check every file.

I know the application doesn't gracefully handle when files change outside of the application yet, but there could be race conditions that the application sees the file change on disk and notify the user the file is missing/edited before the FaW logic could tell the application it is gone/edited.

Also there's a case that you delete the file, but the file changes weren't saved, then I guess you just close the file anyways? (not looking for a direct answer, just thinking out loud).

Most of these cases are possible to handle but could run into cases that a folder could contain hundreds or thousands of files and make the entire situation worse.


Remembering now, all this was why long ago I never added a context menu since it is desirable to have these kinds of file operations but hard to handle gracefully.

dail8859 avatar May 10 '24 02:05 dail8859

Some of such stuff could be fixed using timer and check for all opened files once a second. Going to fix:

  • [x] Check files on Rename folder
    • all Editors will be renamed
  • [x] Check files on Delete folder
    • saved Editors will be closed
    • unsaved Editors will be kept open

SimLV avatar May 10 '24 15:05 SimLV

Some of such stuff could be fixed using timer and check for all opened files once a second.

Honestly that definitely doesn't not sound like a good idea and timers are usually just band aid for race conditions.

dail8859 avatar May 10 '24 17:05 dail8859

Then QFileSystemWatcher for each Editor bound to a file

SimLV avatar May 10 '24 19:05 SimLV

@dail8859 could you please check and possible merge this PR. I think further improvements could be in other PRs.

SimLV avatar May 12 '24 07:05 SimLV

I want to find some more time to review this again but not sure when that will be yet to identify any additional issues that need addressed or noted for future PRs.

I'll be the first to admit I'm quite particular about code that gets merged and want to make sure. I do appreciate the time and effort you and others contribute so I'll do my best to set aside some time for this.

dail8859 avatar May 12 '24 15:05 dail8859

I did pick this back up recently and started hacking away at it. In the end I might simplify it down for now to get alot of the changes merged in and can tackle some of the technically challenging functionality later to narrow down testing it.

dail8859 avatar Jul 08 '24 21:07 dail8859

Merged current master and tested

SimLV avatar Sep 14 '24 07:09 SimLV