REQ: PackageEvents onFiletreeFileDelete and onFiletreeFileRename ...

[jjvbsag]

I'm (still) writing on a subversion package for Zerobrane and I'm quite far ¹)

But my package misses, when a file is being renamed or deleted in the file tree. So there should be some more package events for this. I suggest

onFiletreeFileDelete(...,pathOfFileDeleted)
onFiletreeFileRename(...,oldPathOfFileRenamed,newPathOfFileRenamed)

and also

onFiletreeFilePreDelete(...,pathOfFileDeleted)
onFiletreeFilePreRename(...,oldPathOfFileRenamed,newPathOfFileRenamed)

the last two should have the option to return false to abort the action.

...Rename should also trigger, when a file is moved to another folder in the file tree.

---

¹) Still discussing with my employer how to release this to public 🍺

[jjvbsag]

One more question: Would it harm ZBS to have style wxTR_MULTIPLE used in the project tree control?

---

Just to give you an impression, how it might look:

[pkulchenko]

But my package misses, when a file is being renamed or deleted in the file tree. So there should be some more package events for this.

@jjvbsag, yes, I can add these events, although I need to do a couple of changes first. Also, I need to think whether these events should be triggered before or after user dialogs (confirmation on delete or overwrite).

One more question: Would it harm ZBS to have style wxTR_MULTIPLE used in the project tree control?

This one is tricky. It seems like GetSelection() is disabled when MULTIPLE style is set, but it can be replaced with GetFocusedItem(), although it was not available in the previous version, so will need to account for that.

Renames and deletes should work, but selection may get wrong after these tree modifications and selection with the cursor may also picked the wrong item when hovering over multiple selection; for example, select 3 items and one having focus; when right-clicking on a selected item without focus, the delete/edit operations will be on selected item, so this needs to be accounted for or fixed. Or maybe Delete and some other operations, like needs to work on all selected files anyway.

I'm curious, why would you need this style? I may be an option to turn it on in some contexts (as it can be changed dynamically), but I like the idea of having MULTIPLE style available, so will think about other aspects that may need to be tweaked to enable this.

Just to give you an impression, how it might look:

This looks great! I'm interested in seeing what you can come up with, as we can probably make it a bit more generic, so it also works with git and perforce.

[pkulchenko]

@jjvbsag, I'm working on a couple of changes that should enable TR_MULTIPLE in the project tree. I think I found a good way to handle edit, delete and other events that used to work with selected items. Should have it completed tomorrow.

[jjvbsag]

Also, I need to think whether these events should be triggered before or after user dialogs (confirmation on delete or overwrite).

The xxxPrexxx Events before and/or as a replacement. E.g. for delete the package would check, if the file has been edited, then make a warning, changes will get lost. I would suggest the following return values

• true action (delete/rename) HAS already been done by the package using the version control system. no further action by ZBS (but ZBS has to update the tree)
• false cancel the action
• nil let ZBS show it's confirmation dialog (e.g if the file (or the project) is not under version control)

The other Events then might not be needed, but that I have to evalute when the are implemented

I'm curious, why would you need this style?

Because the package also extends the Menu on Right-Click in the file tree and it just would be handy to commit multiple files at once by this.

as we can probably make it a bit more generic, so it also works with git and perforce.

Sure. this is (at least for GIT) on the ToDo list. I have separated the "get file status from subversion into a table" from "update controls from status table". So one could replace the first step by "get file status from GIT..." and leave the rest unchanged (well, the status names must be unified then)

[pkulchenko]

@jjvbsag, pushed changes to make the tree to work with multiple selections; it hasn't been enabled yet, but you can enable it with the following patch:

diff --git a/src/editor/filetree.lua b/src/editor/filetree.lua
index a4020e1..c775c40 100644
--- a/src/editor/filetree.lua
+++ b/src/editor/filetree.lua
@@ -815,7 +815,7 @@ end
 -- project
 local projtree = ide:CreateTreeCtrl(ide.frame, wx.wxID_ANY,
   wx.wxDefaultPosition, wx.wxDefaultSize,
-  wx.wxTR_HAS_BUTTONS + wx.wxTR_SINGLE + wx.wxTR_LINES_AT_ROOT
+  wx.wxTR_HAS_BUTTONS + wx.wxTR_MULTIPLE + wx.wxTR_LINES_AT_ROOT
   + wx.wxTR_EDIT_LABELS + wx.wxNO_BORDER)
 projtree:SetFont(ide.font.fNormal)
 filetree.projtreeCtrl = projtree

[jjvbsag]

-  wx.wxTR_HAS_BUTTONS + wx.wxTR_SINGLE + wx.wxTR_LINES_AT_ROOT
+  wx.wxTR_HAS_BUTTONS + wx.wxTR_MULTIPLE + wx.wxTR_LINES_AT_ROOT

@pkulchenko thanks, works great. Now I can start to implement the event handlers.

This is the function, I use to get the elements:

------------------------------------------------------------
-- get items from the tree
-- return {list-of-selected},focused
------------------------------------------------------------
function SVN_PLUGIN:GetSelectedTreeItems()
	local tree=ide:GetProjectTree()
	local function GetText(itemId)
		if itemId:IsOk() then
			return tree:GetItemText(itemId)
		end
	end
	local selected={}
	if tree:HasFlag(wx.wxTR_MULTIPLE) then
		for i,treeItemId in ipairs(tree:GetSelections()) do
			push(selected,GetText(treeItemId))
		end
	else
		push(selected,GetText(tree:GetSelection()))
	end
	local focused=GetText(tree:GetFocusedItem())
	return selected,focused
end

any comments are welcome, feel free to use it

---

By the way. Now with Multiple Selections, the interface for the requested package events should be redefined.

onFiletreeFileDelete(...,pathOfFileDeleted) should get a LIST of files to delete:
onFiletreeFileDelete(...,listOfFilesDeleted)

onFiletreeFileRename(...,oldPathOfFileRenamed,newPathOfFileRenamed)
stay so, if a single file is renamed, but change to
onFiletreeFileMove(...,listOfFilesMoved,destinationDir)
when one or more files are moved to another location.
The same for all the xxxPrexxx events.

I hope, this will be the last iteration :-)

[pkulchenko]

This is the function, I use to get the elements:

Yes, this looks correct.

By the way. Now with Multiple Selections, the interface for the requested package events should be redefined.
onFiletreeFileDelete(...,pathOfFileDeleted) should get a LIST of files to delete:

I thought about it, but I'm not sure that's the right way to deal with this as it's inconsistent with other callbacks. You can always use the same method you implemented in GetSelectedTreeItems if you want to get a complete list of selected files in addition to the focused one.

With the current implementation, only one file will be moved/deleted even when MULTIPLE flag is applied, so the callback doesn't need to be changed. Even if this is modified in the future and move/delete will take the list into account, I think it can be simply accommodated by calling individual events multiple times, so no signature/parameter changes will be required.

[pkulchenko]

@jjvbsag, here is the diff that implements the event handlers we were discussing. I included more than just the handlers in the diff as there was a patch for a wxwidgets bug (v2.9.5) that interfere with the event handlers, so I ended up removing it as the issue no longer happens with wxwidgets 3.0+. Take a look and let me know if this works for you and I'll check it in.

diff --git a/src/editor/filetree.lua b/src/editor/filetree.lua
index 61dd585..e970728 100644
--- a/src/editor/filetree.lua
+++ b/src/editor/filetree.lua
@@ -307,10 +307,9 @@ local function treeSetConnectorsAndIcons(tree)
 
   local empty = ""
   local function renameItem(itemsrc, target)
-    local cache = type(itemsrc) == 'table' and itemsrc or nil
-    local isdir = not cache and tree:IsDirectory(itemsrc) or cache and cache.isdir or false
-    local isnew = not cache and tree:GetItemText(itemsrc) == empty or cache and cache.isnew or false
-    local source = cache and cache.fullname or tree:GetItemFullName(itemsrc)
+    local isdir = tree:IsDirectory(itemsrc)
+    local isnew = tree:GetItemText(itemsrc) == empty
+    local source = tree:GetItemFullName(itemsrc)
     local fn = wx.wxFileName(target)
 
     -- check if the target is the same as the source;
@@ -318,6 +317,10 @@ local function treeSetConnectorsAndIcons(tree)
     -- on case insensitive systems, but need to be allowed in renaming.
     if source == target then return end
 
+    if PackageEventHandle("onFiletreeFilePreRename", tree, itemsrc, source, target) == false then
+      return false
+    end
+
     local docs = {}
     if not isnew then -- find if source is already opened in the editor
       docs = (isdir
@@ -356,7 +359,7 @@ local function treeSetConnectorsAndIcons(tree)
       end
     end
 
-    refreshAncestors(cache and cache.parent or tree:GetItemParent(itemsrc))
+    refreshAncestors(tree:GetItemParent(itemsrc))
     -- load file(s) into the same editor (if any); will also refresh the tree
     if #docs > 0 then
       for _, doc in ipairs(docs) do
@@ -391,6 +394,8 @@ local function treeSetConnectorsAndIcons(tree)
       if doc then LoadFile(doc:GetFilePath(), doc:GetEditor()) end
     end
 
+    PackageEventHandle("onFiletreeFileRename", tree, itemsrc, source, target)
+
     return true
   end
   local function deleteItem(item_id)
@@ -403,6 +408,10 @@ local function treeSetConnectorsAndIcons(tree)
     local isdir = tree:IsDirectory(item_id)
     local source = tree:GetItemFullName(item_id)
 
+    if PackageEventHandle("onFiletreeFilePreDelete", tree, item_id, source) == false then
+      return false
+    end
+
     if isdir and FileDirHasContent(source..pathsep) then return false end
     if wx.wxMessageBox(
       TR("Do you want to delete '%s'?"):format(source),
@@ -423,6 +432,7 @@ local function treeSetConnectorsAndIcons(tree)
       end
     end
     refreshAncestors(tree:GetItemParent(item_id))
+    PackageEventHandle("onFiletreeFileDelete", tree, item_id, source)
     return true
   end
 
@@ -768,23 +778,11 @@ local function treeSetConnectorsAndIcons(tree)
       local target = MergeFullPath(tree:GetItemFullName(parent), label)
       if cancelled or label == empty then refreshAncestors(parent)
       elseif target then
-        -- normally, none of this caching would be needed as `renameItem`
-        -- would be called to check if the item can be renamed;
-        -- however, as it may open a dialog box, on Linux it's causing a crash
-        -- (caused by the same END_LABEL_EDIT even triggered one more time),
-        -- so to protect from that, `renameItem` is called from IDLE event.
-        -- Unfortunately, by that time, the filetree item (`itemsrc`) may
-        -- already have incorrect state (as it's removed from the tree),
-        -- so its properties need to be cached to be used from IDLE event.
-        local cache = {
-          isdir = tree:IsDirectory(itemsrc),
-          isnew = tree:GetItemText(itemsrc) == empty,
-          fullname = tree:GetItemFullName(itemsrc),
-          parent = parent,
-        }
-        ide:DoWhenIdle(function()
-            if not renameItem(cache, target) then refreshAncestors(parent) end
-          end)
+        -- wxwidgets v2.9.5 and earlier crashes when the IDE loses focus
+        -- during renaming a file that has a conflict with an exising one
+        -- (caused by the same END_LABEL_EDIT even triggered one more time).
+        if ide.osname == "Linux" and ide.wxver <= "2.9.5" then return end
+        if not renameItem(itemsrc, target) then refreshAncestors(parent) end
       end
     end)
 
@@ -815,7 +813,7 @@ end
 -- project
 local projtree = ide:CreateTreeCtrl(ide.frame, wx.wxID_ANY,
   wx.wxDefaultPosition, wx.wxDefaultSize,
-  wx.wxTR_HAS_BUTTONS + wx.wxTR_SINGLE + wx.wxTR_LINES_AT_ROOT
+  wx.wxTR_HAS_BUTTONS + wx.wxTR_MULTIPLE + wx.wxTR_LINES_AT_ROOT
   + wx.wxTR_EDIT_LABELS + wx.wxNO_BORDER)
 projtree:SetFont(ide.font.fNormal)
 filetree.projtreeCtrl = projtree

[pkulchenko]

@jjvbsag, let me know if you had a chance to test the changes as I'd like to get the changes in if the work as expected. Thanks.

[jjvbsag]

@pkulchenko Sorry, I'm very busy with other work at the moment. I will try ASAP, maybe tomorrow if possible. But I will for sure give you details, as soon as I have tested.
Thanks for your efforts in any case 👍

[jjvbsag]

Ok, a quick test show, events are called as expected, but evaluation of return values is not complete:

I would suggest the following return values

• true action (delete/rename) HAS already been done by the package using the version control system. no further action by ZBS (but ZBS has to update the tree)
• false cancel the action
• nil let ZBS show it's confirmation dialog (e.g if the file (or the project) is not under version control)

nil and false works as expected, but true does not. After my plugin has asked the user, user said YES and the plugin deleted the file, ZBS still comes with the question
Do you want to delete '/directorypath/filename'?

When onFiletreeFilePreDelete or onFiletreeFilePreRename returns true, ZBS should only refresh the file tree, but don't have further user dialogs.

[pkulchenko]

When onFiletreeFilePreDelete or onFiletreeFilePreRename returns true, ZBS should only refresh the file tree, but don't have further user dialogs.

That's too specific and inconsistent with other cases that return false from Pre handlers. If you need to refresh the tree (as you know that you made modifications), you can call tree:RefreshChildren() to refresh the entire tree or tree:RefreshChildren(tree:GetItemParent(node)) to refresh a particular node down. It also refreshes all expanded sub-nodes under that node.

I'll get the changes in, however I'll stay with the existing API that only handles false value returned, as I believe it's sufficient and consistent with other cases.

[pkulchenko]

@jjvbsag, how is the progress with the subversion package?

[jjvbsag]

@pkulchenko Dear Paul, we are using it and tweaking the last issues¹. But I'm still struggling with my company to get an ok for publishing it on github. We've had some major changes (new owner) in the last year, now we have a new CEO. I will do my best to make this (and other stuff) available and I also fight for a donation to you 👍

Regards Jørgen

---

¹ I know, there is always another issue waiting 😉

[pkulchenko]

Thanks Jørgen; let me know if I can help in any way. I'd be interested to update the package, so it works with other version control systems if possible.

[jjvbsag]

@pkulchenko 🎁 Dear Paul, I can now proudly direct you to zbssvn.

It's still not complete, but already quite usable. I have my doubt, it can be updated to support multiple version control systems, because the "habbit" of e.g. subversion and git is quite different. But for sure it can be seen as a template, how such a plugin may be coded. You are welcome to give any feedback, either to me by mail or via issue tracking on the repository.

[pkulchenko]

@jjvbsag, I think it's a great start; thank you for sharing! I still need to go through all the details, but I got it working for the svn repository I have (after fixing a problem with the paths that my local svn executable didn't like). I will have some suggestions for you on how to update the plugin to make it more in line with the current IDE API (for example, DisplayOutputLn has been replaced with ide:Print).

I do agree on support for multiple version control systems; I'd very much like to have that. I'll have to look at other version control integration, as there may still be enough overlap between git and svn to support both through the same UI.