Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Context Menu operations on Folder as Workspace tree #556

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

SimLV
Copy link

@SimLV SimLV commented May 7, 2024

For now:

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

Copy link
Owner

@dail8859 dail8859 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 37 to 38
ui = new Ui::FolderAsWorkspaceDock;
model = new QFileSystemModel(this);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason these couldn't be kept in the initializer list?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was warning about -Wreorder I don't get how to remove

#include "ui_FolderAsWorkspaceDock.h"

#include <QFileSystemModel>
QString NEW_DIR_TEMPLATE("dir_%1");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

auto doc = window->currentEditor();
QString dstName(parentDir.absoluteFilePath(doc->getName()));

if (doc->saveAs(dstName) != QFileDevice::NoError)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep consistent with the rest of the code base, rename doc to editor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

QString dstName = NEW_DIR_TEMPLATE.arg(i);

auto newItem = model->mkdir(lastSelectedItem, dstName);
if (!newItem.isValid()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch this condition as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

{
QDir dir(path);
QString fileName = dir.absoluteFilePath(oldName);
for(auto &&editor : window->editors())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also leverage something in window...might need something new like renameFile to take an editor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved parts into MainWindow

void FolderAsWorkspaceDock::on_actionDelete_triggered()
{
QString path(model->filePath(lastSelectedItem));
QMessageBox::StandardButton reply = QMessageBox::question(this, tr("Delete Item"),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably use window->moveFileToTrash() here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented both options but I have no windows box nearby to test on windows.

Comment on lines 48 to 51
void on_actionSaveHere_triggered();
void on_actionNewFolder_triggered();
void on_actionRename_triggered();
void on_actionDelete_triggered();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M.. why/where? connectSlotsByName has no such notices at any version.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

@SimLV SimLV May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
end

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea, though I don't see Lua as a long-term solution. It was originally just added for debugging/testing purposes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -27,19 +27,77 @@
<property name="bottomMargin">
<number>0</number>
</property>
<item>
<widget class="QMenu" name="menuFile">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't. I googled for it but at least Qt 5 can't. For me it looks like minor inconvenience everyone accustomed to.

@SimLV SimLV requested a review from dail8859 May 8, 2024 18:04
@dail8859
Copy link
Owner

dail8859 commented May 8, 2024

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
Copy link
Owner

dail8859 commented May 9, 2024

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 :)

@@ -1369,7 +1356,10 @@ void MainWindow::moveFileToTrash(ScintillaNext *editor)

if (askMoveToTrash(filePath)) {
if (editor->moveToTrash()) {
closeByPath(filePath);
closeCurrentFile();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you closing current file? One could open one file and delete another one.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! Good catch! 😄 This is what I get for trying to modify it on little sleep.

@SimLV
Copy link
Author

SimLV commented May 9, 2024

  • 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?

@dail8859
Copy link
Owner

dail8859 commented May 9, 2024

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
Copy link
Owner

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.

@SimLV
Copy link
Author

SimLV commented May 10, 2024

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

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

@dail8859
Copy link
Owner

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.

@SimLV
Copy link
Author

SimLV commented May 10, 2024

Then QFileSystemWatcher for each Editor bound to a file

SimLV added 2 commits May 10, 2024 22:57
+ getPath dropped from ScintillaNext
+ getFilePath now uses Qt directory separators
@SimLV
Copy link
Author

SimLV commented May 12, 2024

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

@dail8859
Copy link
Owner

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.

@SimLV SimLV mentioned this pull request Jun 5, 2024
@dail8859
Copy link
Owner

dail8859 commented Jul 8, 2024

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.

@SimLV
Copy link
Author

SimLV commented Sep 14, 2024

Merged current master and tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants