Context Menu operations on Folder as Workspace tree#556
Context Menu operations on Folder as Workspace tree#556SimLV wants to merge 11 commits intodail8859:masterfrom
Conversation
ad5b0fb to
fe945df
Compare
dail8859
left a comment
There was a problem hiding this comment.
Appreciate the PR! I knew this functionality was definitely needed. I think this is a great first start. I'm sure there will be more added to the menu eventually.
| ui = new Ui::FolderAsWorkspaceDock; | ||
| model = new QFileSystemModel(this); |
There was a problem hiding this comment.
Any particular reason these couldn't be kept in the initializer list?
There was a problem hiding this comment.
There was warning about -Wreorder I don't get how to remove
| #include "ui_FolderAsWorkspaceDock.h" | ||
|
|
||
| #include <QFileSystemModel> | ||
| QString NEW_DIR_TEMPLATE("dir_%1"); |
There was a problem hiding this comment.
I'd have to look deeper how this is used but might make sense to keep it inside the class so this can be translated.
| auto doc = window->currentEditor(); | ||
| QString dstName(parentDir.absoluteFilePath(doc->getName())); | ||
|
|
||
| if (doc->saveAs(dstName) != QFileDevice::NoError) |
There was a problem hiding this comment.
In most cases I prefer the "happy path" to be the first part (e.g. switching this to ==). Just helps with readability since you have to skip over the failure case and look at the else clause to see the natural flow.
Also, this is a bit of a code 'smell' that the FolderAsWorkspace is messing with the lower editor API. Might be possible to use something like window->saveFileAs(...) so that incase there are errors it will be handled the same way as a normal file would if it failed.
There was a problem hiding this comment.
I separate UI questions inside MainWindow and use them here instead.
| void FolderAsWorkspaceDock::on_actionSaveHere_triggered() | ||
| { | ||
| QDir parentDir(model->filePath(lastSelectedItem)); | ||
| auto doc = window->currentEditor(); |
There was a problem hiding this comment.
To keep consistent with the rest of the code base, rename doc to editor.
| QString dstName = NEW_DIR_TEMPLATE.arg(i); | ||
|
|
||
| auto newItem = model->mkdir(lastSelectedItem, dstName); | ||
| if (!newItem.isValid()) { |
| { | ||
| QDir dir(path); | ||
| QString fileName = dir.absoluteFilePath(oldName); | ||
| for(auto &&editor : window->editors()) |
There was a problem hiding this comment.
Might also leverage something in window...might need something new like renameFile to take an editor.
There was a problem hiding this comment.
There is some mess between MainWindow <-> EditorManager and also ScintillaNext as a view vs some nonexistent Content which could be file or i.e. temporary buffer
| void FolderAsWorkspaceDock::on_actionDelete_triggered() | ||
| { | ||
| QString path(model->filePath(lastSelectedItem)); | ||
| QMessageBox::StandardButton reply = QMessageBox::question(this, tr("Delete Item"), |
There was a problem hiding this comment.
Probably use window->moveFileToTrash() here.
There was a problem hiding this comment.
Implemented both options but I have no windows box nearby to test on windows.
| void on_actionSaveHere_triggered(); | ||
| void on_actionNewFolder_triggered(); | ||
| void on_actionRename_triggered(); | ||
| void on_actionDelete_triggered(); |
There was a problem hiding this comment.
Qt doesn't recommend the auto connect slots and I haven't used them elsewhere throughout the codebase, but for now it is fine. It can be cleaned up later.
There was a problem hiding this comment.
M.. why/where? connectSlotsByName has no such notices at any version.
There was a problem hiding this comment.
There was a problem hiding this comment.
It is good scripting usage to automate creation of all these boilerplate as a lua script inside NotepadNext.
For some future version obviously
local luaxml = require 'LuaXML'
local file_name = nn.activeEditor.path:gsub('.ui$')
local cpp_editor = nn.findEditorByPath(file_name .. '.cpp')
local h_editor = nn.findEditorByPath(file_name .. '.h')
assert(cpp_editor and h_editor, 'Please open corresponding .cpp and .h')
local cpp_bookmark = cpp_editor:findNamedBookmark('ctor signals')
local h_bookmark = cpp_editor:findNamedBookmark('slots')
assert(cpp_bookmark and h_bookmark, 'Please set "ctor signals" and "slots" bookmarks')
local items = {}
local tree = xml.load(file_name .. '.ui')
tree:iterate(function(el)
table.insert(items, el.name)
end, 'action', nil, nil, true)
local class_name = find_class_name(cpp_bookmark) -- TODO
for i,name in ipairs(items) do
local method_name = 'on'.. name:gsub('^[a-z]', function(s) return s:upper() end) .. 'Triggered'
local full_method = 'void '..method_name..'()'
if not h_editor:findText(full_method) then
h_bookmark:appendText(full_method .. ';\n');
cpp_bookmark:append('connect(ui->'..name..', &QAction::triggered, this, &'.. class_name.. '::'..method_name..');')
cpp_editor:append(full_method..'\n{\n}\n')
end
endThere was a problem hiding this comment.
Interesting idea, though I don't see Lua as a long-term solution. It was originally just added for debugging/testing purposes
There was a problem hiding this comment.
I see lua as very good "Glue language" to customize and script stuff. I am going to use NotepadNext as very customizable light editor (instead of NotepadQQ and ZeroBraneStudio) on projects where I don't need big IDE like PyCharm
| <number>0</number> | ||
| </property> | ||
| <item> | ||
| <widget class="QMenu" name="menuFile"> |
There was a problem hiding this comment.
Can QtCreator create a QMenu in the ui file? or some other method (e.g. manually)? I'm just interested since I've never seen this done before :)
There was a problem hiding this comment.
It can't. I googled for it but at least Qt 5 can't. For me it looks like minor inconvenience everyone accustomed to.
|
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 |
| if (askMoveToTrash(filePath)) { | ||
| if (editor->moveToTrash()) { | ||
| closeByPath(filePath); | ||
| closeCurrentFile(); |
There was a problem hiding this comment.
Why are you closing current file? One could open one file and delete another one.
There was a problem hiding this comment.
You are right! Good catch! 😄 This is what I get for trying to modify it on little sleep.
Do you need some other rework for this branch from me for now? |
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 :)
Agreed. They are very nice and normally 'expected' features if you are going to be showing a list of files.
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.
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. |
|
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. |
|
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. |
|
Then QFileSystemWatcher for each Editor bound to a file |
+ getPath dropped from ScintillaNext + getFilePath now uses Qt directory separators
|
@dail8859 could you please check and possible merge this PR. |
|
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. |
|
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. |
…rkspace_operations
|
Merged current master and tested |
|
@dail8859: Any news on this PR? |
# Conflicts: # src/NotepadNext/dialogs/MainWindow.cpp
|
Merged current master |


For now: