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

Added context menu to treeview. #303

Merged
merged 3 commits into from
Jul 14, 2021

Conversation

jgmdev
Copy link
Member

@jgmdev jgmdev commented Jun 23, 2021

Thanks to work by @takase1121 we have a nice context menu and on this pull request I did the needed modifications to enable it on the treeview with the following changes:

  • Moved ContextMenu object from plugins.contextmenu to core.contextmenu
    for usage by other plugins.
  • Added the options: Open in System, Rename, New File, New Folder and
    Delete.

Screenshots

2021-06-23_02:56:57

2021-06-23_03:07:15

If anything can be done better or is missing let me know.

@takase1121
Copy link
Member

takase1121 commented Jun 23, 2021

Maybe it's possible to add "remove directory" option too, but we'll need rmdir().

@jgmdev
Copy link
Member Author

jgmdev commented Jun 23, 2021

Maybe it's possible to add "remove directory" option too, but we'll need rmdir().

Done! Added rmdir to common.rmdir(path, recursively)

@takase1121
Copy link
Member

Done! Added rmdir to common.rmdir(path, recursively)

That's great, but unfortunately I found out that deleting an empty folder on Windows using os.remove always return errcode 13 (permission denied).

A Lua version might not cut it. We might need to do it in C.

For Windows, there's SHFileOperation which grants us superpowers of moving the folder into recycle bin (recursive by default).

For Mac, there's this thing

For Linux however... It'll be fun to deal with XDG dirs and entry files at the same time :)

@jgmdev
Copy link
Member Author

jgmdev commented Jun 23, 2021

That's great, but unfortunately I found out that deleting an empty folder on Windows using os.remove always return errcode 13 (permission denied).

Ok, found this https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-removedirectorya and https://docs.microsoft.com/en-us/windows/win32/debug/retrieving-the-last-error-code and smashed it all together into this:

static int f_rmdir(lua_State *L) {
  const char *path = luaL_checkstring(L, 1);

#ifdef _WIN32
  int deleted = RemoveDirectoryA(path);
  if(deleted > 0) {
    lua_pushboolean(L, 1);
  } else {
    DWORD error_code = GetLastError();
    LPVOID message;

    FormatMessage(
      FORMAT_MESSAGE_ALLOCATE_BUFFER |
      FORMAT_MESSAGE_FROM_SYSTEM |
      FORMAT_MESSAGE_IGNORE_INSERTS,
      NULL,
      error_code,
      MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
      (LPTSTR) &message,
      0,
      NULL
    );

    lua_pushboolean(L, 0);
    lua_pushlstring(L, (LPCTSTR)message, lstrlen((LPCTSTR)message));
    LocalFree(message);

    return 2;
  }
#else
  int deleted = remove(path);
  if(deleted < 0) {
    lua_pushboolean(L, 0);
    lua_pushstring(L, strerror(errno));

    return 2;
  } else {
    lua_pushboolean(L, 1);
  }
#endif

  return 1;
}

Since MacOS is posix compliant remove() should work on empty folders so no further changes should be necessary. Pushed the change but haven't tested on windows (no win machine here), Could you test on windows to see if it compiles and works?

@takase1121
Copy link
Member

Since MacOS is posix compliant remove() should work on empty folders so no further changes should be necessary. Pushed the change but haven't tested on windows (no win machine here), Could you test on windows to see if it compiles and works?

very unfortunate that I'm also not using a Windows dev environment (WSL2 + WSLg). I'll try to cross compile and see if it works.

@takase1121
Copy link
Member

I can't get it to cross compile. @jgmdev did you try to use github actions to do it?

@jgmdev
Copy link
Member Author

jgmdev commented Jun 23, 2021

@takase1121 I committed a fix, @redtide tested on a windows VM and the issue was a header (#include <strsafe.h>) that I added when implementing the get error message sample and which braked standard C functionality.

@franko
Copy link
Member

franko commented Jun 24, 2021

I would like to do with PRs only one thing at once.

I propose to do one PR to add context menu only with the commands that already exists and works correctly.

We can therefore do a secode PR to add the rmdir command.

Each PR has to be reviewed separately.

@jgmdev
Copy link
Member Author

jgmdev commented Jun 24, 2021

I would like to do with PRs only one thing at once.

Added a second pull request that adds system.rmdir(path) on #313

Now this pull request holds 1 single commit, and after discussion on #313 it should be merged before this one.

@jgmdev
Copy link
Member Author

jgmdev commented Jun 28, 2021

Updated to use common.rm instead of common.rmdir

@adamharrison
Copy link
Member

OK, I think this is almost good to go.

One last thing; on delete from the treeview, it takes a few seconds for the autoreload plugin to kick in and actually remove the item from the treeview.

Could we either hook the rm function treeview to detect when a visible item is deleted, and just instantly delete it, or, if that leads to too much code, at the very least, have the context menu action specifically remove the item instantly?

@franko
Copy link
Member

franko commented Jul 11, 2021

One last thing; on delete from the treeview, it takes a few seconds for the autoreload plugin to kick in and actually remove the item from the treeview.

Could we either hook the rm function treeview to detect when a visible item is deleted, and just instantly delete it, or, if that leads to too much code, at the very least, have the context menu action specifically remove the item instantly?

Hi, I am back from my two weeks of vacation, time to get back working :-)

I introduced quite some time ago a function for this purpose, core.reschedule_project_scan(), to be called when we need to rescan the project directory. It is not perfect, it just tells the scheduler to run the project scan job at the next frame run. It works with the only caveat that if the scheduler was already running it does nothing and in this case we may have a delay of several seconds for the refresh.

So in short, just call core.reschedule_project_scan(). Longer answer, we may want to improve the function core.reschedule_project_scan() to stop the current scan of project folder if it is already running and start a new job.

@jgmdev
Copy link
Member Author

jgmdev commented Jul 12, 2021

Hi, I am back from my two weeks of vacation, time to get back working :-)

Welcome back!

I introduced quite some time ago a function for this purpose, core.reschedule_project_scan(), to be called when we need to rescan the project directory

That is what I wanted to ask, how to reupdate the cached files and directories residing on a specific location (not the entire project tree), I was inspecting the treeview plugin code and it uses some cache that depends on the core project scan functionality. Then I was trying to understand the core project scan files/directories lua table/list to be able to scan/refresh only certain path but to be honest I didn't investigated the code base enough to properly comprehend the table layout and how it is been used.

If I recall correctly I was first trying to make use of TreeView:invalidate_cache(dirname) but without success since the cached core project directories and files weren't also invalidated. I did pushed a commit to use core.reschedule_project_scan() as pointed out so that should improve things a bit.

I found this threaded C99 header file and maybe could be integrated into lite-xl in the form of two basic functions system.fswatch(path, callback) and system.fsunwatch(path) to better keep track of project dir/file since the small 1 header library uses another thread which would help keep the editor running more snappier on huge code bases (as an example I work on a project that has more than 20,000 files inside of various directories and using the coroutine yielding task switching functionality doesn't cuts it).

@francesco-st
Copy link
Contributor

That is what I wanted to ask, how to reupdate the cached files and directories residing on a specific location (not the entire project tree), I was inspecting the treeview plugin code and it uses some cache that depends on the core project scan functionality. Then I was trying to understand the core project scan files/directories lua table/list to be able to scan/refresh only certain path but to be honest I didn't investigated the code base enough to properly comprehend the table layout and how it is been used.

Well, for your purpose you should not tinker with the code in treeview because it is really meant for the visualization of the directory tree and files. If you really want to update a part of the directory tree I guess you should implement a new function in 'core', one that says, "ok keep the files listing you already have and perform an update by scanning just this subfolder". When done just make sure that treeview update everything by invalidating its cache (caveat, I think it checks the files list using a shallow object comparison between the previous list and the new one).

On the other side I don't recommend doing this because it greatly complicate the architecture for a little gain.

I found this threaded C99 header file and maybe could be integrated into lite-xl in the form of two basic functions system.fswatch(path, callback) and system.fsunwatch(path) to better keep track of project dir/file since the small 1 header library uses another thread which would help keep the editor running more snappier on huge code bases (as an example I work on a project that has more than 20,000 files inside of various directories and using the coroutine yielding task switching functionality doesn't cuts it).

This is the solution we really need! It is a long time I wanted to do this because scanning the directory all the time is really wasteful and inefficient and things get bad when the project is big.

I strongly advice you begin experimenting with septag/dmon in a new branch if you want to and you have time. I would be glad to jump on that and help you when I have some time.

@francesco-st
Copy link
Contributor

Concerning this PR I am favorable to merge when you think you are ready.

I have only a concern because we don't display a confirmation dialog when deleting a directory but this can be added later if we want.

@takase1121
Copy link
Member

Since we may/may not add a few fs related functions, should we move this into a seperate module?

@jgmdev
Copy link
Member Author

jgmdev commented Jul 12, 2021

I have only a concern because we don't display a confirmation dialog

done using NagView

@franko
Copy link
Member

franko commented Jul 12, 2021

I have only a concern because we don't display a confirmation dialog

done using NagView

Great, thank you!

@franko
Copy link
Member

franko commented Jul 12, 2021

Since we may/may not add a few fs related functions, should we move this into a seperate module?

I am not sure I understand, sorry. What we should move into a separate module ? I thought we already moved the rmdir command in the "common" module with a separate PR.

@adamharrison
Copy link
Member

Concerning this PR I am favorable to merge when you think you are ready.

Great. I want this in for 2.0, so ideally sometime in the next couple days, if you're good to go, @jgmdev , just let me know, and I'll merge.

@jgmdev
Copy link
Member Author

jgmdev commented Jul 14, 2021

I'm good to go, mostly every issue was handled, everything else can later be implemented.

@adamharrison adamharrison merged commit d10865b into lite-xl:master Jul 14, 2021
@franko
Copy link
Member

franko commented Jul 14, 2021

I found this threaded C99 header file and maybe could be integrated into lite-xl in the form of two basic functions system.fswatch(path, callback) and system.fsunwatch(path) to better keep track of project dir/file since the small 1 header library uses another thread which would help keep the editor running more snappier on huge code bases (as an example I work on a project that has more than 20,000 files inside of various directories and using the coroutine yielding task switching functionality doesn't cuts it).

The septag/dmon library is now fully integrated in the dmon-integration branch but it needs some polishing and dmon has a serious bug on linux, I created an issue. Otherwise on Windows it works quite well. I am really happy of this new integration because it eliminates the pain point with the periodic rescan of the directory.

@jgmdev
Copy link
Member Author

jgmdev commented Jul 19, 2021

The septag/dmon library is now fully integrated in the dmon-integration branch but it needs some polishing and dmon has a serious bug on linux

This is great, have been testing it and works really fast, no more slow cpu consuming project directory scanning :)

I sent a pull request to fix the directories creation/deletion reporting issue septag/dmon#11 which you may try on the dmon-integration branch.

@franko
Copy link
Member

franko commented Jul 19, 2021

This is great, have been testing it and works really fast, no more slow cpu consuming project directory scanning :)

Thank you. I am also happy of this long-awaited improvement. I just need some more work to make the dmon variant really robust and bug free.

I sent a pull request to fix the directories creation/deletion reporting issue septag/dmon#11 which you may try on the dmon-integration branch.

Thank you so much, your are amazing! I wanted to fix this myself like you did but didn't have time to work on that problem. I will be glad to test your new version on linux.

Just a small comment about your PR for dmon: you should not mix whitespace changes (like those made automatically by lite-xl) with actual code changes. This is considered a bad practice and is actually annoying. You can configure lite-xl to disable whitespace trimming on a per-project basis but I guess you already know about that.

@jgmdev
Copy link
Member Author

jgmdev commented Jul 20, 2021

I am also happy of this long-awaited improvement.

Wow, I just tested your latest commits by rebasing your changes on master and modifying the treeview plugin to remove the reschedule project scan on this branch https://github.com/jgmdev/lite-xl/tree/dmon and it is working beautifully so far, thanks a lot for this work! Maybe this could also make it into 2.0 and if not a subsequent 2.1 :)

@franko
Copy link
Member

franko commented Jul 20, 2021

Wow, I just tested your latest commits by rebasing your changes on master and modifying the treeview plugin to remove the reschedule project scan on this branch https://github.com/jgmdev/lite-xl/tree/dmon and it is working beautifully so far, thanks a lot for this work! Maybe this could also make it into 2.0 and if not a subsequent 2.1 :)

Thank you for fixing dmon itself on linux, you made a great work!

Otherwise, you tested the dmon branch on lite-xl on linux ? The last time I tested it was terribly broken because of the bug in dmon. I will test again with the new version. Otherwise on Windows everything was working smoothly.

On the other side it is delicate to add this feature for the 2.0 release. We may introduce some subtle bugs. It is wiser to take some time and implement this improvement in the 2.1 release or something like that.

@jgmdev
Copy link
Member Author

jgmdev commented Jul 20, 2021

Otherwise, you tested the dmon branch on lite-xl on linux ?

Yes, working perfectly so far with the treeview context menu, will keep testing on the days ahead.

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.

None yet

5 participants